feat(corelib): core::iter::chain by hudem1 · Pull Request #7064 · starkware-libs/cairo

hudem1 · 2025-01-12T21:00:35Z

pub fn chain<A, B>(a: A, b: B) -> Chain<A, B>

Converts the arguments to iterators and links them together, in a chain.

Example

let a = array![1, 2, 3];
let b = array![4, 5, 6];

let mut iter = a.into_iter().chain(b)

assert_eq!(iter.next(), Option::Some(1));
assert_eq!(iter.next(), Option::Some(2));
assert_eq!(iter.next(), Option::Some(3));
assert_eq!(iter.next(), Option::Some(4));
assert_eq!(iter.next(), Option::Some(5));
assert_eq!(iter.next(), Option::Some(6));
assert_eq!(iter.next(), Option::None);

reviewable-StarkWare · 2025-01-12T21:00:42Z

This change is

orizi

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

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

#[test]
fn test_iterator_chain() {

add test chaining different types.

corelib/src/iter/adapters/chain_adapter.cairo line 40 at r1 (raw file):

/// assert_eq!(iter.next(), None);
/// ```
pub fn chain<

shouldn't this be a method?

corelib/src/iter/adapters/chain_adapter.cairo line 44 at r1 (raw file):

    B,
    impl IntoIterA: IntoIterator<A>,
    impl IntoIterB: IntoIterator<B>[IntoIter: IntoIterA::IntoIter],

this seems to test the Iterators are of the same type - not their Items.

Code quote:

    impl IntoIterA: IntoIterator<A>,
    impl IntoIterB: IntoIterator<B>[IntoIter: IntoIterA::IntoIter],

orizi

Reviewed all commit messages.
Reviewable status: 1 of 11 files reviewed, 4 unresolved discussions (waiting on @hudem1)

a discussion (no related file):
definitely do not edit the IntoIterator trait for this support.

orizi

Reviewable status: 1 of 11 files reviewed, 5 unresolved discussions (waiting on @hudem1)

corelib/src/iter/adapters/chain_adapter.cairo line 49 at r2 (raw file):

    B,
    impl IntoIterA: IntoIterator<A>,
    impl IntoIterB: IntoIterator<B>[Item: IntoIterA::Item],

Suggestion:

    impl IntoIterA: IntoIterator<A>,
    impl IntoIterB: IntoIterator<B>,
    +TypeEqual<IntoIterA::IntoIter::Item, IntoIterB::IntoIter::Item>

hudem1 · 2025-01-13T16:22:20Z

Reviewed all commit messages.
Reviewable status: 1 of 11 files reviewed, 4 unresolved discussions (waiting on @hudem1)

Previously, Orizi wrote...

a discussion (no related file): definitely do not edit the IntoIterator trait for this support.

Oh okay! I tried to do like Rust and thought IntoIterator trait might not have been fully finished. Sorry about that, I will revert the commit related to that and thank you for your suggestion after! I wasn't aware of this syntax.

hudem1 · 2025-01-13T16:25:58Z

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

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

Previously, orizi wrote...

add test chaining different types.

Done.

hudem1 · 2025-01-13T16:33:35Z

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

corelib/src/iter/adapters/chain_adapter.cairo line 40 at r1 (raw file):

Previously, orizi wrote...

/// assert_eq!(iter.next(), None);
/// ```
pub fn chain<

shouldn't this be a method?

I tried to do like the PR core::iter::zip #7050. In this PR, the zip is not created as a method to Iterator<T>. Am I missing something maybe ? Should I then go and add the method chain to Iterator<T> ?

orizi

Reviewable status: 1 of 11 files reviewed, 5 unresolved discussions (waiting on @hudem1)

corelib/src/iter/adapters/chain_adapter.cairo line 40 at r1 (raw file):

Previously, hudem1 wrote…

Previously, orizi wrote...
/// assert_eq!(iter.next(), None);
/// ```
pub fn chain<
shouldn't this be a method?
I tried to do like the PR core::iter::zip #7050. In this PR, the zip is not created as a method to Iterator<T>. Am I missing something maybe ? Should I then go and add the method chain to Iterator<T> ?

i guess it should be the same answer for both.

the question is - why is it that way in rust, and should we keep the same - or do we have specific reasons to diverge.

This reverts commit 3353f49.

julio4 · 2025-01-13T17:40:33Z

corelib/src/iter/adapters/chain_adapter.cairo line 40 at r1 (raw file):

Previously, orizi wrote…

i guess it should be the same answer for both.

the question is - why is it that way in rust, and should we keep the same - or do we have specific reasons to diverge.

So iter::adapters by definition takes iterator(s) to return a new iterator.
For most cases when it performs a transformation over a single iterator it makes sense to only have a method, i.e map: iter.map();
But when it concerns multiple iterators we can optionally export the adapters as a function as well, as the syntax is a bit more readable, i.e. zip: consider zip(iterA, iterB) vs iterA.zip(iterB). The first one is clearly understandable for example in for loops, the later one should still be possible for chaining iterator adapters (iterA.map().zip(iterB))

I think chain should be both a function and method of Iterator

julio4

Reviewable status: 1 of 11 files reviewed, 5 unresolved discussions (waiting on @hudem1 and @orizi)

corelib/src/iter/adapters/chain_adapter.cairo line 49 at r2 (raw file):

    B,
    impl IntoIterA: IntoIterator<A>,
    impl IntoIterB: IntoIterator<B>[Item: IntoIterA::Item],

I believe we can't access directly the associated item without the trait impl, but something like this can work:

+TypeEqual<IntoIterA::Iterator::<IntoIterA::IntoIter>::Item, IntoIterB::Iterator::<IntoIterB::IntoIter>::Item>

I tried to implement chain as well before but I was stuck on this, I was not aware of this TypeEqual syntax, good to know!

hudem1

Reviewable status: 1 of 11 files reviewed, 5 unresolved discussions (waiting on @julio4 and @orizi)

corelib/src/iter/adapters/chain_adapter.cairo line 49 at r2 (raw file):

Previously, julio4 (Julio) wrote…

I believe we can't access directly the associated item without the trait impl, but something like this can work:
+TypeEqual<IntoIterA::Iterator::<IntoIterA::IntoIter>::Item, IntoIterB::Iterator::<IntoIterB::IntoIter>::Item>
I tried to implement chain as well before but I was stuck on this, I was not aware of this TypeEqual syntax, good to know!

Yes indeed, I tried orizi's suggestion and there was a slight Path problem.

Thank you for your suggestion @julio4! I tried and believe we can omit ::<IntoIterA::IntoIter> and ::<IntoIterB::IntoIter>, and using the code below seems to work:

Code snippet:

+TypeEqual<IntoIterA::Iterator::Item, IntoIterB::Iterator::Item>

orizi

Reviewable status: 1 of 11 files reviewed, 4 unresolved discussions (waiting on @hudem1 and @julio4)

corelib/src/iter/adapters/chain_adapter.cairo line 40 at r1 (raw file):

Previously, julio4 (Julio) wrote…

So iter::adapters by definition takes iterator(s) to return a new iterator.
For most cases when it performs a transformation over a single iterator it makes sense to only have a method, i.e map: iter.map();
But when it concerns multiple iterators we can optionally export the adapters as a function as well, as the syntax is a bit more readable, i.e. zip: consider zip(iterA, iterB) vs iterA.zip(iterB). The first one is clearly understandable for example in for loops, the later one should still be possible for chaining iterator adapters (iterA.map().zip(iterB))

I think chain should be both a function and method of Iterator

i think it shouldn't be a function, in zip as well - as a place holder for when we have proper inline macros.

chain! and zip! for an arbitrary number of arguments are much better than binary chain and zip.

julio4

Reviewable status: 1 of 11 files reviewed, 4 unresolved discussions (waiting on @hudem1 and @orizi)

corelib/src/iter/adapters/chain_adapter.cairo line 40 at r1 (raw file):

Previously, orizi wrote…

i think it shouldn't be a function, in zip as well - as a place holder for when we have proper inline macros.

chain! and zip! for an arbitrary number of arguments are much better than binary chain and zip.

For arbitrary number of arguments it would be better I agree.
Is the inlining macro system not good enough to be able to do this now?
Should we keep these function or remove them for now?

orizi

Reviewable status: 1 of 11 files reviewed, 4 unresolved discussions (waiting on @hudem1 and @julio4)

corelib/src/iter/adapters/chain_adapter.cairo line 40 at r1 (raw file):

Previously, julio4 (Julio) wrote…

For arbitrary number of arguments it would be better I agree.
Is the inlining macro system not good enough to be able to do this now?
Should we keep these function or remove them for now?

not good enough now - but i hope will be better soon enough.

i think we should remove now, as we can always add, but removing after any release is much harder.

Not complete yet: an error persists in the Iterator::chain method

hudem1 · 2025-01-13T22:14:48Z

I still got an error in the Iterator::chain method that I need to solve before the PR being ready. (And I also need to rebase).

By the way, if you got an idea as to why the error appears, I am interested!

orizi

Reviewed 1 of 5 files at r1, 9 of 11 files at r3, all commit messages.
Reviewable status: 10 of 12 files reviewed, 6 unresolved discussions (waiting on @hudem1 and @julio4)

corelib/src/iter/traits/collect.cairo line 95 at r3 (raw file):

/// Trying to convert an already existing iterator into an iterator
/// just returns the iterator itself

seems really unrelated to PR.

Code quote:

/// Trying to convert an already existing iterator into an iterator
/// just returns the iterator itself

corelib/src/iter/adapters/chain_adapter.cairo line 8 at r3 (raw file):

pub struct Chain<A, B> {
    // These are "fused" with `Option` so we don't need separate state to track which part is
    // already exhausted, and we may also get niche layout for `None`.

WDYM?

Code quote:

and we may also get niche layout for `None`.

corelib/src/iter/adapters/chain_adapter.cairo line 12 at r3 (raw file):

    // Only the "first" iterator is actually set `None` when exhausted.
    a: Option<A>,
    b: Option<B>,

if the second one is never None - let us:

Suggestion:

    // These are "fused" with `Option` so we don't need separate state to track which part is
    // already exhausted, and we may also get niche layout for `None`.
    //
    // Only the "first" iterator is actually set `None` when exhausted.
    a: Option<A>,
    b: B,

corelib/src/iter/adapters/chain_adapter.cairo line 49 at r3 (raw file):

        self.b = Option::Some(second_container);

        value

Suggestion:

        // First iterate over first container values.
        if let Option::Some(first) = self.a {
            if let Option::Some(value) = first.next() {
                self.a = Option::Some(first);
                return Option::Some(value)
            } else {
                self.a = Option::None;
            }
        }

        // Then iterate over second container values.
        self.b.next()

corelib/src/iter.cairo line 3 at r3 (raw file):

mod adapters;
mod traits;
pub use adapters::{Chain, chained_iterator};

this is only for usage in the trait - right?
if so:

Suggestion:

pub use adapters::Chain;
pub(crate) use adapters::chained_iterator;

orizi

this is possibly is just the LS - unless you get this error when compiling as well now.

Reviewable status: 10 of 12 files reviewed, 5 unresolved discussions (waiting on @hudem1 and @julio4)

julio4

Reviewed 1 of 11 files at r3.
Reviewable status: 11 of 12 files reviewed, 6 unresolved discussions (waiting on @hudem1 and @orizi)

corelib/src/iter/adapters/chain_adapter.cairo line 0 at r3 (raw file):
After removing fn chain you can rename the module to only chain instead of chain_adapter

orizi

Reviewed 1 of 11 files at r3.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @hudem1)

orizi

please rebase - there is still some problematic issue with using an Iterator directly instead of an IntoIter as an arg - but for the time being should mostly work.

Reviewable status: 12 of 13 files reviewed, all discussions resolved (waiting on @hudem1)

orizi

@hudem1 please rebase and check - it is ok not having tests with direct extrainto_iter() - it is a known bug (and it is actually nice not needing the extra "clipply" style warning for removing it) - but it seems to not really hinder us for the time being.

Reviewable status: 12 of 13 files reviewed, all discussions resolved (waiting on @hudem1)

hudem1 · 2025-02-13T14:27:11Z

@hudem1 please rebase and check - it is ok not having tests with direct extrainto_iter() - it is a known bug (and it is actually nice not needing the extra "clipply" style warning for removing it) - but it seems to not really hinder us for the time being.

Reviewable status: 12 of 13 files reviewed, all discussions resolved (waiting on @hudem1)

Hi !

Sorry my late reply. I'll be more careful to reply faster!

I merged main into my branch, and removed the unnecessary into_iter if i understood correctly what you meant.

When i run the tests, i still have the following errors:

for 1st test test_iterator_chain_different_types:

for 2nd test test_iterator_chain_same_types:

It seems like the condition +TypeEqual<Self::Item, IntoIterU::Iterator::Item> in call to iterator::chain does not work well ?

orizi

no need to appologize :) great work here.

thanks for the update - will continue looking into it now.

indeed meant removing the not required .into_iter() from the test.

Reviewed 8 of 9 files at r7, all commit messages.
Reviewable status: 12 of 13 files reviewed, 3 unresolved discussions (waiting on @hudem1)

corelib/src/test/iter_test.cairo line 2 at r7 (raw file):

use core::iter::PeekableTrait;
use core::ops::Range;

corelib/src/test/iter_test.cairo line 148 at r7 (raw file):

    let b: Range<u8> = 0..5;

    let mut iter = a.into_iter().chain(b);

Suggestion:

    let a: Array<u8> = array![7, 8, 9];
    let mut iter = a.into_iter().chain(0..5_u8);

corelib/src/iter/traits/iterator.cairo line 2 at r7 (raw file):

use crate::iter::adapters::{
    Chain, Enumerate, Filter, Map, Peekable, Zip, chained_iterator, enumerated_iterator, filter_iterator, mapped_iterator,

run ./scripts/cairo_fmt.sh.

hudem1

Thank you :)
Sure, let me know if there is anything else to modify!

Reviewable status: 11 of 13 files reviewed, 3 unresolved discussions (waiting on @orizi)

corelib/src/iter/traits/iterator.cairo line 2 at r7 (raw file):

Previously, orizi wrote…

run ./scripts/cairo_fmt.sh.

Done

corelib/src/test/iter_test.cairo line 2 at r7 (raw file):

use core::iter::PeekableTrait;
use core::ops::Range;

Done

corelib/src/test/iter_test.cairo line 148 at r7 (raw file):

    let b: Range<u8> = 0..5;

    let mut iter = a.into_iter().chain(b);

Done. I actually inlined a as well and in the other test as well

orizi

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

orizi

you have other bugs - can you first makes sure the corelib itself compiles?
run ./scripts/cairo_test.sh.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @hudem1)

corelib/src/iter/traits/iterator.cairo line 722 at r7 (raw file):

    /// assert_eq!(iter.next(), Option::None);
    /// ```
    fn chain<U, impl IntoIterU: IntoIterator<U>, +TypeEqual<Self::Item, IntoIterU::Iterator::Item>>(

probably the cause for the actual failure.

Suggestion:

    fn chain<U, impl IntoIterU: IntoIterator<U>, +TypeEqual<Self::Item, IntoIterU::Iterator::Item>, +Destruct<T>>(

orizi

any updates?

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hudem1)

orizi

please update - or tomorrow i will need to redo the PR.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hudem1)

hudem1 · 2025-02-23T23:46:34Z

please update - or tomorrow i will need to redo the PR.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @hudem1)

Oh no, late reply a 2nd time ! :(
I actually started an internship recently, and now my email inbox gets flooded by pipeline runs notifications, for every single pipeline that runs. This combined with advertisements I was already getting everyday makes me miss the important emails I wanna see like this project. I have to unsubscribe from all those to not miss anything. I'll make sure to do that before making new contributions ! :)

Anyway, it now compiles and tests pass, great!! Thank you for the fix!

orizi

all good - thanks!

Reviewed 3 of 3 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @hudem1)

orizi

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @hudem1)

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

gilbens-starkware

Reviewed 4 of 13 files at r6, 6 of 9 files at r7, 3 of 3 files at r10, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @hudem1)

TomerStarkware

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @hudem1)

feat(corelib): core::iter::chain

f0d91b9

orizi requested changes Jan 13, 2025

View reviewed changes

hudem1 added 3 commits January 13, 2025 17:03

feat(corelib): Add type Item to IntoIterator trait

3353f49

feat(corelib): Add doc to IntoIterator<T> impl where T is an iterator

a69ddd4

feat(corelib): Add test chaining different iterator types

7f84162

orizi requested changes Jan 13, 2025

View reviewed changes

Revert "feat(corelib): Add type Item to IntoIterator trait"

c1e26ba

This reverts commit 3353f49.

julio4 reviewed Jan 13, 2025

View reviewed changes

hudem1 commented Jan 13, 2025

View reviewed changes

orizi requested changes Jan 13, 2025

View reviewed changes

julio4 reviewed Jan 13, 2025

View reviewed changes

orizi requested changes Jan 13, 2025

View reviewed changes

hudem1 added 3 commits January 13, 2025 21:57

feat(corelib): Update doc for chain() - use TypeEqual for iterator items

b69355c

feat(corelib): Remove chain standalone function

98d07d5

feat(corelib): Add Iterator::chain method - Update tests

ff51f82

Not complete yet: an error persists in the Iterator::chain method

orizi requested changes Jan 14, 2025

View reviewed changes

orizi reviewed Jan 14, 2025

View reviewed changes

julio4 reviewed Jan 14, 2025

View reviewed changes

orizi reviewed Jan 14, 2025

View reviewed changes

hudem1 added 3 commits January 14, 2025 17:06

feat(corelib): Remove doc unrelated to PR

999329b

feat(corelib): Clean Iterator<Chain>::next - Rename adapter file

88cd023

feat(corelib): Make chained_iterator visible to crate only

137030a

orizi reviewed Feb 5, 2025

View reviewed changes

orizi reviewed Feb 12, 2025

View reviewed changes

hudem1 added 2 commits February 13, 2025 14:56

Merge branch 'main' into feat/iter_adapter_chain

ddb23e3

clean: Remove extra into_iter call

d06375b

orizi requested changes Feb 13, 2025

View reviewed changes

fix: Lint and inlining

c58ef8e

hudem1 commented Feb 13, 2025

View reviewed changes

orizi reviewed Feb 13, 2025

View reviewed changes

orizi requested changes Feb 14, 2025

View reviewed changes

orizi reviewed Feb 20, 2025

View reviewed changes

orizi reviewed Feb 23, 2025

View reviewed changes

fix: Add +Destruct<T> on iter::chain

95d5551

hudem1 added 3 commits February 24, 2025 00:48

Merge branch 'main' into feat/iter_adapter_chain

ae1a67a

lint

b7495bd

fix: Remove duplicate import

2524a5a

orizi approved these changes Feb 24, 2025

View reviewed changes

orizi added this pull request to the merge queue Feb 24, 2025

orizi removed this pull request from the merge queue due to a manual request Feb 24, 2025

gilbens-starkware approved these changes Feb 25, 2025

View reviewed changes

TomerStarkware approved these changes Feb 25, 2025

View reviewed changes

gilbens-starkware added this pull request to the merge queue Feb 25, 2025

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Feb 25, 2025

gilbens-starkware added this pull request to the merge queue Feb 25, 2025

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Feb 25, 2025

orizi added this pull request to the merge queue Feb 25, 2025

Merged via the queue into starkware-libs:main with commit fc40831 Feb 25, 2025

Conversation

hudem1 commented Jan 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example

Uh oh!

reviewable-StarkWare commented Jan 12, 2025

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

hudem1 commented Jan 13, 2025

Uh oh!

hudem1 commented Jan 13, 2025

Uh oh!

hudem1 commented Jan 13, 2025

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

julio4 commented Jan 13, 2025

Uh oh!

julio4 left a comment

Choose a reason for hiding this comment

Uh oh!

hudem1 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

julio4 left a comment

Choose a reason for hiding this comment

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

hudem1 commented Jan 13, 2025

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

julio4 left a comment

Choose a reason for hiding this comment

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

hudem1 commented Feb 13, 2025

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

hudem1 left a comment

Choose a reason for hiding this comment

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

orizi left a comment

Choose a reason for hiding this comment

Uh oh!

hudem1 commented Jan 12, 2025 •

edited

Loading

hudem1 left a comment •

edited

Loading

hudem1 commented Feb 23, 2025 •

edited

Loading