feat(corelib): IntoIter for Span-convertible @C

[cairoIover]

Implementes IntoIter for all Containers-snapshots convertible to span

Enables:
@array![].into_iter()
(@(array![].span())).into_iter

Supercedes the implementation specific to @[T; SIZE]: @[].into_iter()

[reviewable-StarkWare]

This change is 

[orizi]

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

---

a discussion (no related file):
this sounds more like a job for .iter() sort of function - and not into_iter(), i'm not entirely sure.

[cairoIover]

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

---

a discussion (no related file):

Previously, orizi wrote…

this sounds more like a job for .iter() sort of function - and not into_iter(), i'm not entirely sure.

I initially was about to implement iter but then realized that (@C).into_iter() should also be available and easier to implement.

I do believe that in rust, doing the following is strictly equivalent:

    let vec = vec![1, 2, 3];

    // iter() - borrows the elements
    for x in vec.iter() {
        println!("{:?}", x); // x is of type &i32
    }
    println!("{:?}", vec); // vec is still valid here

    // into_iter on &vec
    let vec = vec![1, 2, 3];
    let vec_iter = (&vec).into_iter();
    for x in vec_iter {
        println!("{:?}", x); // x is of type &i32
    }
    println!("{:?}", vec); // vec is still valid here

[cairoIover]

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

---

a discussion (no related file):

Previously, cairoIover wrote…

I initially was about to implement iter but then realized that (@C).into_iter() should also be available and easier to implement.

I do believe that in rust, doing the following is strictly equivalent:

    let vec = vec![1, 2, 3];

    // iter() - borrows the elements
    for x in vec.iter() {
        println!("{:?}", x); // x is of type &i32
    }
    println!("{:?}", vec); // vec is still valid here

    // into_iter on &vec
    let vec = vec![1, 2, 3];
    let vec_iter = (&vec).into_iter();
    for x in vec_iter {
        println!("{:?}", x); // x is of type &i32
    }
    println!("{:?}", vec); // vec is still valid here

so I could write a follow up PR about implementing iter on Span and Array. But the behavior implemented in this one should be supported as well

[orizi]

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

---

a discussion (no related file):

Previously, cairoIover wrote…

so I could write a follow up PR about implementing iter on Span and Array. But the behavior implemented in this one should be supported as well

i don't think so actually - @ of something should probably not provide any into_iter.
we probably need a .into_iter() for [T; N]specifically and possibly remove the iterator for @[T; N] although, this specific case does sounds reasonable for me to have an into_iter same as &[T] &[T; N] and [T; N] in rust.

[cairoIover]

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

---

a discussion (no related file):

Previously, orizi wrote…

i don't think so actually - @ of something should probably not provide any into_iter.
we probably need a .into_iter() for [T; N]specifically and possibly remove the iterator for @[T; N] although, this specific case does sounds reasonable for me to have an into_iter same as &[T] &[T; N] and [T; N] in rust.

Why should @ of something not provide an into_iter? It does not seem unreasonable to me to have snapshot-based iterations to prevent ownership from being moved

[orizi]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @dean-starkware, @gilbens-starkware, @ilyalesokhin-starkware, and @TomerStarkware)

---

a discussion (no related file):

Previously, cairoIover wrote…

Why should @ of something not provide an into_iter? It does not seem unreasonable to me to have snapshot-based iterations to prevent ownership from being moved

I suspect your are correct, but I would like to think about it for a few.
Tagging a few more people for extra brain power @gilbens-starkware @dean-starkware @TomerStarkware @ilyalesokhin-starkware

[dean-starkware]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @dean-starkware, @gilbens-starkware, @ilyalesokhin-starkware, @orizi, and @TomerStarkware)

---

a discussion (no related file):

Previously, orizi wrote…

I suspect your are correct, but I would like to think about it for a few.
Tagging a few more people for extra brain power @gilbens-starkware @dean-starkware @TomerStarkware @ilyalesokhin-starkware

Users might associate .into_iter() with consuming ownership, which seems counterintuitive for @-based snapshots. Would it make sense to implement .iter() first, and revisit .into_iter() later if ownership-transfer use cases arise?

[orizi]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @dean-starkware, @gilbens-starkware, @ilyalesokhin-starkware, and @TomerStarkware)

---

a discussion (no related file):

Previously, dean-starkware wrote…

Users might associate .into_iter() with consuming ownership, which seems counterintuitive for @-based snapshots. Would it make sense to implement .iter() first, and revisit .into_iter() later if ownership-transfer use cases arise?

i think the idea is for things such as:

for x in @some_arr {
...
}

to work - which they won't without this or anything similar.

[dean-starkware]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @dean-starkware, @gilbens-starkware, @ilyalesokhin-starkware, @orizi, and @TomerStarkware)

---

a discussion (no related file):

Previously, orizi wrote…

i think the idea is for things such as:

for x in @some_arr {
...
}

to work - which they won't without this or anything similar.

If the goal is to allow seamless iteration in for loops, implementing .into_iter() here makes perfect sense.
That said, we should make sure to clearly document the behavior, so users understand that .into_iter() on @ does not imply ownership transfer but rather works like .iter() under the hood.
Also, I'm not so sure this is too bad: (instead of what you suggested, which is indeed more ergonomic)
for x in @some_arr.iter() { ... } // Manual call to iter().

[orizi]

Reviewable status: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @dean-starkware, @gilbens-starkware, @ilyalesokhin-starkware, and @TomerStarkware)

---

a discussion (no related file):

Previously, dean-starkware wrote…

If the goal is to allow seamless iteration in for loops, implementing .into_iter() here makes perfect sense.
That said, we should make sure to clearly document the behavior, so users understand that .into_iter() on @ does not imply ownership transfer but rather works like .iter() under the hood.
Also, I'm not so sure this is too bad: (instead of what you suggested, which is indeed more ergonomic)
for x in @some_arr.iter() { ... } // Manual call to iter().

any copyable object can be consumed and than used - "it just happens" that all snapshots are copyable.
so .into_iter() consumes a snapshot - this is still totally valid.

for x in @arr.iter() - should probably not work - as no reason for a snapshot of an iterator to be iterable.

[tdelabro]

for x in @arr.iter() - should probably not work - as no reason for a snapshot of an iterator to be iterable.

Not sure about that.
This code is currently valid:

    fn get_snap(ref ar: Array<u32>) -> @Array<u32> {
        @ar
    }
    
    
     let mut x = array![1, 2, 3];
     let y = get_snap(ref x);

     println!("{:?}", y);

So it is possible to take a snapshot of a reference.
Given that, there is not reason that you cannot iter upon the snapshot of an iterator, which would be @arr.iter()

But I'm not sure what you are referring to by iter(). So far it doesn't exist in the corelib

In rust iter() (and iter_mut()) are not part of any trait. They are convenient methods impl on some types. By convention, and convention only, iter always returns an iterator over &T and iter_mut over &mut T.
Given that, in Cairo, ref only makes sense in the context of function calls I don't know how it fits here. I don't think it does.

[orizi]

Reviewed 2 of 3 files at r2, all commit messages.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @cairoIover, @dean-starkware, @gilbens-starkware, @ilyalesokhin-starkware, and @TomerStarkware)

---

corelib/src/test/array_test.cairo line 211 at r2 (raw file):

    assert_eq!(fixed_arr.span()[1], @11);
}

in any case no need to changed this test same as the next one.

add another test that tests the exact changed feature.

additionally remove the last weird assertion added in all the tests.

Code quote:

fn test_snapshot_fixed_size_array_iterator() {
    let fixed_arr = [10_usize, 11, 12, 13];
    let mut iter = (@fixed_arr).into_iter();
    assert_eq!(iter.next(), Option::Some(@10));
    assert_eq!(iter.next(), Option::Some(@11));
    assert_eq!(iter.next(), Option::Some(@12));
    assert_eq!(iter.next(), Option::Some(@13));
    assert!(iter.next().is_none());

    assert_eq!(fixed_arr.span()[1], @11);
}

[orizi]

Reviewed 1 of 3 files at r1, 1 of 3 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @cairoIover, @gilbens-starkware, @ilyalesokhin-starkware, and @TomerStarkware)

[cairoIover]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @ilyalesokhin-starkware, @orizi, and @TomerStarkware)

---

corelib/src/test/array_test.cairo line 211 at r2 (raw file):

Previously, orizi wrote…

in any case no need to changed this test same as the next one.

add another test that tests the exact changed feature.

additionally remove the last weird assertion added in all the tests.

Done.

By the way - on
d6f8fbd the test_snapshot_span_into_iter was working fine. Following #6991, trait inference fails:

error[E0002]: Method `into_iter` could not be called on type `@core::array::Span::<?0>`.
Candidate `core::iter::traits::collect::IntoIterator::into_iter` inference failed with: Trait has no implementation in context: core::iter::traits::collect::IntoIterator::<@core::array::Span::<?0>>.
 --> corelib/src/test/array_test.cairo:234:33
    let mut span_iter = (@span).into_iter();

[cairoIover]

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

---

corelib/src/test/array_test.cairo line 211 at r2 (raw file):

Previously, cairoIover wrote…

Done.

By the way - on
d6f8fbd the test_snapshot_span_into_iter was working fine. Following #6991, trait inference fails:

error[E0002]: Method `into_iter` could not be called on type `@core::array::Span::<?0>`.
Candidate `core::iter::traits::collect::IntoIterator::into_iter` inference failed with: Trait has no implementation in context: core::iter::traits::collect::IntoIterator::<@core::array::Span::<?0>>.
 --> corelib/src/test/array_test.cairo:234:33
    let mut span_iter = (@span).into_iter();

sorry I got confused - regardless the commit, inference fails if SnapshotSpanIntoIterator is in array.cairo, but not if in collect.cairo next to IntoIterator

[orizi]

Reviewed 2 of 3 files at r3, all commit messages.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @cairoIover, @gilbens-starkware, @ilyalesokhin-starkware, and @TomerStarkware)

---

corelib/src/test/array_test.cairo line 239 at r3 (raw file):

        i += 1;
    }
}

Suggestion:

fn test_snapshot_array_into_iter() {
    let mut iter = (@array![1, 2, 3, 4, 5]).into_iter();
    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!(iter.next().is_none());
}

#[test]
fn test_snapshot_span_into_iter() {
    let mut iter = (@(array![1, 2, 3, 4, 5].span())).into_iter();
    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!(iter.next().is_none());
}

[cairoIover]

Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware, @ilyalesokhin-starkware, @orizi, and @TomerStarkware)

---

corelib/src/test/array_test.cairo line 239 at r3 (raw file):

        i += 1;
    }
}

Done.

[orizi]

Reviewed all commit messages.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @cairoIover, @gilbens-starkware, @ilyalesokhin-starkware, and @TomerStarkware)

---

corelib/src/test/array_test.cairo line 211 at r2 (raw file):

Previously, cairoIover wrote…

sorry I got confused - regardless the commit, inference fails if SnapshotSpanIntoIterator is in array.cairo, but not if in collect.cairo next to IntoIterator

so we should probably have a few specific implementations - as this seems to be out of scope if implemented in collect.

[cairoIover]

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @array, @gilbens-starkware, @ilyalesokhin-starkware, @orizi, and @TomerStarkware)

---

corelib/src/test/array_test.cairo line 211 at r2 (raw file):

Previously, orizi wrote…

so we should probably have a few specific implementations - as this seems to be out of scope if implemented in collect.

Why is this not being picked up automatically when implemented next to Iterator in collect? From my understanding of the trait/impl system it should work. Am I missing something ?

I can restore the @[T;SIZE] and add a @array implementation if that's fine.

[orizi]

Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @array, @gilbens-starkware, @ilyalesokhin-starkware, @snap, and @TomerStarkware)

---

corelib/src/test/array_test.cairo line 211 at r2 (raw file):

Previously, cairoIover wrote…

Why is this not being picked up automatically when implemented next to Iterator in collect? From my understanding of the trait/impl system it should work. Am I missing something ?

I can restore the @[T;SIZE] and add a @array implementation if that's fine.

more so - i'd rather have three impls - one for @array, one for @snap and one for @[].

[cairoIover]

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @array, @gilbens-starkware, @ilyalesokhin-starkware, @orizi, @snap, and @TomerStarkware)

---

corelib/src/test/array_test.cairo line 211 at r2 (raw file):

Previously, orizi wrote…

more so - i'd rather have three impls - one for @array, one for @snap and one for @[].

Done.

[orizi]

Reviewed all commit messages.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @cairoIover, @gilbens-starkware, @ilyalesokhin-starkware, and @TomerStarkware)

---

corelib/src/iter/traits/collect.cairo line 119 at r5 (raw file):

    fn into_iter(self: @Array<T>) -> Self::IntoIter {
        self.span().into_iter()
    }

these need to be near Span and Array definition.

Code quote:

impl SnapshotSpanIntoIterator<T> of IntoIterator<@Span<T>> {
    type IntoIter = crate::array::SpanIter<T>;
    fn into_iter(self: @Span<T>) -> Self::IntoIter {
        (*self).into_iter()
    }
}

impl SnapshotArrayIntoIterator<T> of IntoIterator<@Array<T>> {
    type IntoIter = crate::array::SpanIter<T>;
    fn into_iter(self: @Array<T>) -> Self::IntoIter {
        self.span().into_iter()
    }

[cairoIover]

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

---

corelib/src/iter/traits/collect.cairo line 119 at r5 (raw file):

Previously, orizi wrote…

these need to be near Span and Array definition.

When located there, there's still the problem about trait inference. The implementations of IntoIterator are not found.

[cairoIover]

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

---

corelib/src/iter/traits/collect.cairo line 119 at r5 (raw file):

error[E0002]: Method into_iter could not be called on type @core::array::Array::<?0>.
Candidate core::iter::traits::collect::IntoIterator::into_iter inference failed with: Trait has no implementation in context: core::iter::traits::collect::IntoIterator::<@core::array::Array::<?0>>.
--> corelib/src/test/array_test.cairo:219:45
let mut iter = (@array![1, 2, 3, 4, 5]).into_iter();
^^^^^^^^^

error[E0002]: Method into_iter could not be called on type @core::array::Span::<?0>.
Candidate core::iter::traits::collect::IntoIterator::into_iter inference failed with: Trait has no implementation in context: core::iter::traits::collect::IntoIterator::<@core::array::Span::<?0>>.
--> corelib/src/test/array_test.cairo:230:54
let mut iter = (@(array![1, 2, 3, 4, 5].span())).into_iter();

[orizi]

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @ilyalesokhin-starkware, and @TomerStarkware)

---

corelib/src/iter/traits/collect.cairo line 119 at r5 (raw file):

Previously, cairoIover wrote…

error[E0002]: Method into_iter could not be called on type @core::array::Array::<?0>.
Candidate core::iter::traits::collect::IntoIterator::into_iter inference failed with: Trait has no implementation in context: core::iter::traits::collect::IntoIterator::<@core::array::Array::<?0>>.
--> corelib/src/test/array_test.cairo:219:45
let mut iter = (@array![1, 2, 3, 4, 5]).into_iter();
^^^^^^^^^

error[E0002]: Method into_iter could not be called on type @core::array::Span::<?0>.
Candidate core::iter::traits::collect::IntoIterator::into_iter inference failed with: Trait has no implementation in context: core::iter::traits::collect::IntoIterator::<@core::array::Span::<?0>>.
--> corelib/src/test/array_test.cairo:230:54
let mut iter = (@(array![1, 2, 3, 4, 5].span())).into_iter();

so now for the annoying solution -
SnapshotIntoIter trait here - with impl for IntoIterator here, while implementing it.
unless @gilbens-starkware have a better idea.

[cairoIover]

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @ilyalesokhin-starkware, @orizi, @TomerStarkware, and @x)

---

corelib/src/iter/traits/collect.cairo line 119 at r5 (raw file):

Previously, orizi wrote…

so now for the annoying solution -
SnapshotIntoIter trait here - with impl for IntoIterator here, while implementing it.
unless @gilbens-starkware have a better idea.

Is it not going to cause conflicts if I have two traits with the same methods?

calling x.into_iter() will not know whether to call (@x).into_iter() (pass it by snapshot) or x.into_iter()

[orizi]

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @ilyalesokhin-starkware, @t, @TomerStarkware, and @x)

---

corelib/src/iter/traits/collect.cairo line 119 at r5 (raw file):

Previously, cairoIover wrote…

Is it not going to cause conflicts if I have two traits with the same methods?

calling x.into_iter() will not know whether to call (@x).into_iter() (pass it by snapshot) or x.into_iter()

it should be just a helper - should probably not have the same method name (and if it does, it shouldn't have self) - the idea is - since we have an issue with finding methods for types, where the relevant trait is of the form MyTrait<@T> and not MyTrait<T> we need this sort of hack unfortunatly.

I'll be checking how easy will it be for us to just add these as well to the lookup context (so actual compiler update)

[orizi]

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @ilyalesokhin-starkware, @t, @TomerStarkware, and @x)

---

corelib/src/iter/traits/collect.cairo line 119 at r5 (raw file):

Previously, orizi wrote…

it should be just a helper - should probably not have the same method name (and if it does, it shouldn't have self) - the idea is - since we have an issue with finding methods for types, where the relevant trait is of the form MyTrait<@T> and not MyTrait<T> we need this sort of hack unfortunatly.

I'll be checking how easy will it be for us to just add these as well to the lookup context (so actual compiler update)

fast attempt #7024

[cairoIover]

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @gilbens-starkware, @ilyalesokhin-starkware, @orizi, @TomerStarkware, and @x)

---

corelib/src/iter/traits/collect.cairo line 119 at r5 (raw file):

Previously, orizi wrote…

fast attempt #7024

Works after #7024 !

-> back to having SnapshotIteratorSpanBased in collect.cairo and a SnapshotSpanIntoIterator in array.cairo

[orizi]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @cairoIover, @gilbens-starkware, @ilyalesokhin-starkware, and @TomerStarkware)

---

corelib/src/iter/traits/collect.cairo line 99 at r6 (raw file):

}

impl SnapshotIteratorSpanBased<C, T, +Into<@C, Span<T>>> of IntoIterator<@C> {

I don't want to commit to everything that has a Into<_, Span> to have an into-iterator.

as i said earlier - have a specific implementation for the things you wanted.
one for [T; N] (so just revert here).
one for @array and one for @span.

[cairoIover]

Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @array, @gilbens-starkware, @ilyalesokhin-starkware, @orizi, @span, and @TomerStarkware)

---

corelib/src/iter/traits/collect.cairo line 99 at r6 (raw file):

Previously, orizi wrote…

I don't want to commit to everything that has a Into<_, Span> to have an into-iterator.

as i said earlier - have a specific implementation for the things you wanted.
one for [T; N] (so just revert here).
one for @array and one for @span.

Done. sorry I got confused.

[orizi]

Reviewed 1 of 1 files at r4, 2 of 2 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware, @ilyalesokhin-starkware, and @TomerStarkware)

[orizi]

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