Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(corelib): Iterator::last #7140

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

MagisterDaIlis
Copy link

Keeping this as draft, as I'm getting some issues regarding impl inference:

error: Cannot infer negative impl in `core::option::DestructOption::<Iterator::Item, +Destruct<Self::Item>, _>` as it contains the unresolved type `core::option::Option::<Iterator::Item>`
 --> corelib/src/iter/traits/iterator.cairo:112:15
        Self::fold(ref self, Option::None, some)

@reviewable-StarkWare
Copy link

This change is Reviewable

@MagisterDaIlis MagisterDaIlis changed the title feat(corelib): IteratorTrait::last feat(corelib): Iterator::last Jan 22, 2025
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @MagisterDaIlis)


corelib/src/test/iter_test.cairo line 19 at r1 (raw file):

    let mut a = ArrayTrait::<u8>::new().into_iter();
    assert_eq!(a.last(), Option::None);

Suggestion:

    assert_eq!(array![1_u8, 2, 3].into_iter().last(), Option::Some(3));
    assert_eq!(array![].into_iter().last().last(), Option::<u8>::None);

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @MagisterDaIlis)


corelib/src/iter/traits/iterator.cairo line 110 at r1 (raw file):

        let some = |_acc: Option<Self::Item>, x: Self::Item| {
            Option::Some(x)
        };

to support destruct:

Suggestion:

        let some = |acc: Option<Self::Item>, x: Self::Item| {
            // Hack to use `Destruct<T>` directly, instead of `Destruct<Option<T>>`.
            match acc {
                Some(_) | None => {}
            };
            Option::Some(x)
        };

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please respond in https://reviewable.io/reviews/starkware-libs/cairo/7140#-

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @MagisterDaIlis)


corelib/src/test/iter_test.cairo line 19 at r1 (raw file):

    let mut a = ArrayTrait::<u8>::new().into_iter();
    assert_eq!(a.last(), Option::None);

Not done yet.

Copy link
Author

@MagisterDaIlis MagisterDaIlis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @orizi)


corelib/src/iter/traits/iterator.cairo line 110 at r1 (raw file):

Previously, orizi wrote…

to support destruct:

unfortunately, doesn't help: the error still occurs

error: Cannot infer negative impl in `core::option::DestructOption::<Iterator::Item, +Destruct<Self::Item>, _>` as it contains the unresolved type `core::option::Option::<Iterator::Item>`
 --> corelib/src/iter/traits/iterator.cairo:116:15
        Self::fold(ref self, Option::None, some)

corelib/src/test/iter_test.cairo line 19 at r1 (raw file):

Previously, orizi wrote…

Not done yet.

second form is not working.

error[E0002]: Method `last` could not be called on type `core::option::Option::<?37>`.
Candidate `core::iter::traits::iterator::Iterator::last` inference failed with: Trait has no implementation in context: core::iter::traits::iterator::Iterator::<core::option::Option::<?37>>.
 --> corelib/src/test/iter_test.cairo:16:44
    assert_eq!(array![].into_iter().last().last(), Option::<u8>::None);

Copy link
Author

@MagisterDaIlis MagisterDaIlis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @orizi)


corelib/src/test/iter_test.cairo line 19 at r1 (raw file):

Previously, MagisterDaIlis wrote…

second form is not working.

error[E0002]: Method `last` could not be called on type `core::option::Option::<?37>`.
Candidate `core::iter::traits::iterator::Iterator::last` inference failed with: Trait has no implementation in context: core::iter::traits::iterator::Iterator::<core::option::Option::<?37>>.
 --> corelib/src/test/iter_test.cairo:16:44
    assert_eq!(array![].into_iter().last().last(), Option::<u8>::None);

Done. actually, my bad, last was repeated.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @MagisterDaIlis)


corelib/src/iter/traits/iterator.cairo line 110 at r1 (raw file):

Previously, MagisterDaIlis wrote…

unfortunately, doesn't help: the error still occurs

error: Cannot infer negative impl in `core::option::DestructOption::<Iterator::Item, +Destruct<Self::Item>, _>` as it contains the unresolved type `core::option::Option::<Iterator::Item>`
 --> corelib/src/iter/traits/iterator.cairo:116:15
        Self::fold(ref self, Option::None, some)

is this the entire error? or are you truncating some parts?


corelib/src/test/iter_test.cairo line 19 at r1 (raw file):

Previously, MagisterDaIlis wrote…

second form is not working.

error[E0002]: Method `last` could not be called on type `core::option::Option::<?37>`.
Candidate `core::iter::traits::iterator::Iterator::last` inference failed with: Trait has no implementation in context: core::iter::traits::iterator::Iterator::<core::option::Option::<?37>>.
 --> corelib/src/test/iter_test.cairo:16:44
    assert_eq!(array![].into_iter().last().last(), Option::<u8>::None);

i called .last() twice in my comment. (and you seem to have copied it)

Copy link
Author

@MagisterDaIlis MagisterDaIlis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/iter/traits/iterator.cairo line 110 at r1 (raw file):

Previously, orizi wrote…

is this the entire error? or are you truncating some parts?

yes, this is the entire error.

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @MagisterDaIlis)


corelib/src/iter/traits/iterator.cairo line 110 at r1 (raw file):

Previously, MagisterDaIlis wrote…

yes, this is the entire error.

oh - it is because Self::fold requires it...
Replied with a suggestion without fold.


corelib/src/iter/traits/iterator.cairo line 116 at r3 (raw file):

        };

        Self::fold(ref self, Option::None, some)

Suggestion:

        let mut self = self;
        let next = Self::next(ref self)?;
        Option::Some(Self::last(self).unwrap_or(next))

Copy link
Author

@MagisterDaIlis MagisterDaIlis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @orizi)


corelib/src/iter/traits/iterator.cairo line 116 at r3 (raw file):

        };

        Self::fold(ref self, Option::None, some)

Done. Very clever!

@MagisterDaIlis MagisterDaIlis marked this pull request as ready for review January 23, 2025 10:06
Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 1 of 1 files at r3, 1 of 1 files at r4, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @MagisterDaIlis)


a discussion (no related file):
@gilbens-starkware @ilyalesokhin-starkware for 2nd eye.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants