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::peekable #7097

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

hudem1
Copy link
Contributor

@hudem1 hudem1 commented Jan 15, 2025

I suggest the addition of Iterator::peekable to be able to use peek() on iterators.

Example

let xs = array![1, 2, 3];

let mut iter = xs.into_iter().peekable();

// peek() lets us see one step into the future
assert_eq!(iter.peek(), Option::Some(1));
assert_eq!(iter.next(), Option::Some(1));
assert_eq!(iter.next(), Option::Some(2));

// The iterator does not advance even if we `peek` multiple times
assert_eq!(iter.peek(), Option::Some(3));
assert_eq!(iter.peek(), Option::Some(3));
assert_eq!(iter.next(), Option::Some(3));

// After the iterator is finished, so is `peek()`
assert_eq!(iter.peek(), Option::None);
assert_eq!(iter.next(), Option::None);

I set my PR as a draft because i have 1 issue and 1 question, possibly related:

  1. When I run my test, it says Error: Compilation failed without any diagnostics.. It does not display the reason of the compilation error and the test file doesn't show any particular error from the LS. I do get an LS error in the iterator.cairo file saying Trait has no implementation in context: core::iter::traits::iterator::Iterator::<T>., which does not make much sense to me as T obviously implements Iterator as we are in the Iterator<T> trait.

  2. I am unsure about how to use the custom trait for the peek method. I exported it in the adapters.cairo and iter.cairo files to be able to import it in my test file, but I am not sure it is the correct way to use it.

Not complete: still some compilation issue
@reviewable-StarkWare
Copy link

This change is Reviewable

@hudem1 hudem1 marked this pull request as draft January 15, 2025 21:18
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.

it is required to be added to the prelude in that case, but lets postpone that.

Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hudem1)


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


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

    let xs = array![1, 2, 3];

    let mut iter = xs.into_iter().peekable();

Suggestion:

fn test_iter_adapter_peekable() {
    let mut iter = (1..4).into_iter().peekable();

Copy link
Contributor

@gilbens-starkware gilbens-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hudem1)


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

    #[must_use]
    fn peekable(self: T) -> Peekable<T, Self::Item> {
        peekable_iterator(self)

We should find the impl we in, but meanwhile, maybe this will work:

Suggestion:

peekable_iterator::<T, Self>(self)

Copy link
Contributor Author

@hudem1 hudem1 left a comment

Choose a reason for hiding this comment

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

Ok.

Reviewable status: 2 of 5 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware and @orizi)


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

Previously, gilbens-starkware (Gil Ben-Shachar) wrote…

We should find the impl we in, but meanwhile, maybe this will work:

Thank you for the suggestion. I tried but it doesn't work, it says Unknown impl over the Self, and running the test still gives the same error Error: Compilation failed without any diagnostics..


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

    let xs = array![1, 2, 3];

    let mut iter = xs.into_iter().peekable();

Done.

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: 2 of 5 files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware and @hudem1)


corelib/src/iter/adapters/peekable.cairo line 17 at r2 (raw file):

pub fn peekable_iterator<I, impl IterI: Iterator<I>>(iter: I) -> Peekable<I, IterI::Item> {
    Peekable { iter, peeked: Option::None }

remove this - it would remove the requirement in the trait - that you don't actually need.

Suggestion:

pub fn peekable_iterator<I>(iter: I) -> Peekable<I, IterI::Item> {
    Peekable { iter, peeked: Option::None }

corelib/src/iter/adapters/peekable.cairo line 21 at r2 (raw file):

impl PeekableIterator<
    I, impl IterI: Iterator<I>, +Drop<I>, +Drop<IterI::Item>,

Suggestion:

    I, impl IterI: Iterator<I>, +Destruct<I>, +Destruct<IterI::Item>,

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.

4 participants