feat(corelib): storage vectors iterators

[julio4]

Add new iterator trait IntoIterRange<T> in the vec module to turn a collection of values into an iterator over a specific range, with into_iter_range(range: Range<u64>) or into_iter_full_range().

Add IntoIterRange implementations for storage vectors to enable iterator over vectors in storage.

1. VecIntoIterRange and MutableVecIntoIterRange for direct iteration over StoragePath<Vec<T>> and StoragePath<Mutable<Vec<T>>>
2. PathableVecIntoIterRange and PathableMutableVecIntoIterRange for iteration over types implementing StorageAsPath into storage vectors (such as Vecs in Starknet contract storage structs)

You need to bring the trait in the current scope:

use starknet::storage::IntoIterRange;

Before:

for i in 0..vec.len() {
    let entry = vec.at(i);
}

After:

for entry in vec.into_iter_full_range() {
    // entry: StoragePath<T> or StoragePath<Mutable<T>>
}

Examples

use starknet::storage::{
    IntoIterRange, MutableVecTrait, StoragePathEntry, StoragePointerReadAccess,
    StoragePointerWriteAccess,
};

#[starknet::contract]
mod contract_with_vec {
    use starknet::storage::Vec;
    #[storage]
    pub struct Storage {
        pub simple: Vec<u32>,
    }
}

fn vec_iter() {
    let mut mut_state = contract_with_vec::contract_state_for_testing();
    for i in 0..9_usize {
        mut_state.simple.append().write(i);
    };

    let state = @contract_with_vec::contract_state_for_testing();
    let mut i = 0;
    for entry in state.simple.into_iter_full_range() {
        assert_eq!(entry.read(), i);
        i += 1;
    };
    assert_eq!(i, 9);

    let mut i = 2;
    for entry in state.simple.into_iter_range(2..5) {
        assert_eq!(entry.read(), i);
        i += 1;
    };
    assert_eq!(i, 5);
}

[reviewable-StarkWare]

This change is 

[orizi]

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

---

a discussion (no related file):
i'm not sure this is actually a good idea for the time being - as this sort of hides the fact that the result vector may be very very large.

the original code at least very specifically needs to specifiy the fact that iteration over a whole section of a vector is required.
Maybe iterating over a specific range of the storage is a bit more sensible? - so both (mostly) easy but still very explicit?

[julio4]

Previously, orizi wrote…

i'm not sure this is actually a good idea for the time being - as this sort of hides the fact that the result vector may be very very large.

the original code at least very specifically needs to specifiy the fact that iteration over a whole section of a vector is required.
Maybe iterating over a specific range of the storage is a bit more sensible? - so both (mostly) easy but still very explicit?

True. But I would argue that the cognitive difference between 0..vec.len() and vec.into_iter() is not that big.
Also iterators provide benefits not only loop syntax sugar, they enable interesting functional patterns and composition.

Maybe we could make size-bounded operations more clear and explicit in the code, for example by forcing iterator instanciation with explicit bound with new patterns like vec.into_iter().take(vec.len())/vec.into_bounded_iter(vec.len())?

Thoughts?

[orizi]

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

---

a discussion (no related file):

Previously, julio4 (Julio) wrote…

True. But I would argue that the cognitive difference between 0..vec.len() and vec.into_iter() is not that big.
Also iterators provide benefits not only loop syntax sugar, they enable interesting functional patterns and composition.

Maybe we could make size-bounded operations more clear and explicit in the code, for example by forcing iterator instanciation with explicit bound with new patterns like vec.into_iter().take(vec.len())/vec.into_bounded_iter(vec.len())?

Thoughts?

do note that for x in vec would work as well - whish isn't as clear as .into_vec to most people probably.

the vec.into_bounded_iter(vec.len()) sound pretty close to the idea i suggested - vec.iter_range(4..89) and vec.into_full_iter_range() style thing.

So - not providing IntoIterator just specific functions providing iterators sound much better to my mind.

[julio4]

Previously, orizi wrote…

do note that for x in vec would work as well - whish isn't as clear as .into_vec to most people probably.

the vec.into_bounded_iter(vec.len()) sound pretty close to the idea i suggested - vec.iter_range(4..89) and vec.into_full_iter_range() style thing.

So - not providing IntoIterator just specific functions providing iterators sound much better to my mind.

Will do, this makes sense.

[julio4]

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

---

a discussion (no related file):

Previously, julio4 (Julio) wrote…

Will do, this makes sense.

Done.

[orizi]

Reviewed 2 of 6 files at r2, all commit messages.
Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @julio4)

---

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

---

corelib/src/iter/traits/iterator.cairo line 22 at r2 (raw file):

/// Turn a collection of values into an iterator over a specific range.
pub trait IntoIterRange<T, I> {

not sure this trait is required - why not add directly to the VecTrait.

Code quote:

/// Turn a collection of values into an iterator over a specific range.
pub trait IntoIterRange<T, I> {

[julio4]

corelib/src/iter/traits/iterator.cairo line 22 at r2 (raw file):

Previously, orizi wrote…

not sure this trait is required - why not add directly to the VecTrait.

I thought it could be potentially implemented by other types. If you think it's useless I can move it in VecTrait, or a separate VecIterTrait

[orizi]

Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @julio4)

---

corelib/src/iter/traits/iterator.cairo line 22 at r2 (raw file):

Previously, julio4 (Julio) wrote…

I thought it could be potentially implemented by other types. If you think it's useless I can move it in VecTrait, or a separate VecIterTrait

i think this is more relvant specifically for storage stuff - since in the usual case probably some combination of into_iter().skip(10).take(20) makes more sense.

[julio4]

corelib/src/iter/traits/iterator.cairo line 22 at r2 (raw file):

Previously, orizi wrote…

i think this is more relvant specifically for storage stuff - since in the usual case probably some combination of into_iter().skip(10).take(20) makes more sense.

Should I move the IntoIterRange trait to the vec module and rename it to VecIntoIterRange, or should I extend VecTrait and MutableVecTrait directly with the new iterators methods?

[orizi]

Reviewable status: 2 of 6 files reviewed, 3 unresolved discussions (waiting on @julio4)

---

corelib/src/iter/traits/iterator.cairo line 22 at r2 (raw file):

Previously, julio4 (Julio) wrote…

Should I move the IntoIterRange trait to the vec module and rename it to VecIntoIterRange, or should I extend VecTrait and MutableVecTrait directly with the new iterators methods?

oh - adding one trait and using it for both is probably nicer and opens more options later - so i like it all and all.

[gilbens-starkware]

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

---

corelib/src/iter/traits/iterator.cairo line 22 at r2 (raw file):

Previously, orizi wrote…

oh - adding one trait and using it for both is probably nicer and opens more options later - so i like it all and all.

Notice that this means that the impls will not be automatically found (is not near the trait nor the generic type) and thus it needs to be whereever it is used.

[julio4]

corelib/src/iter/traits/iterator.cairo line 22 at r2 (raw file):

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

Notice that this means that the impls will not be automatically found (is not near the trait nor the generic type) and thus it needs to be whereever it is used.

I also think it is nicer but yes the trait need to be used. Is there any pattern to "bring" automatically the trait in the scope of VecTrait?

[gilbens-starkware]

Reviewable status: 2 of 6 files reviewed, 4 unresolved discussions (waiting on @julio4 and @orizi)

---

corelib/src/iter/traits/iterator.cairo line 22 at r2 (raw file):

Previously, julio4 (Julio) wrote…

I also think it is nicer but yes the trait need to be used. Is there any pattern to "bring" automatically the trait in the scope of VecTrait?

Importing another trait is fine in this case. The problem is if we need to import the impls as well, but your code seems fine in this regard, does it work if you remove the imported impls from the tests (marked in another comment)?

---

crates/cairo-lang-starknet/cairo_level_tests/collections_test.cairo line 4 at r2 (raw file):

    MutableVecTrait, PathableMutableVecIntoIterRange, PathableVecIntoIterRange, StoragePathEntry,
    StoragePointerReadAccess, StoragePointerWriteAccess,
};

Suggestion:

use starknet::storage::{
    MutableVecTrait, StoragePathEntry,
    StoragePointerReadAccess, StoragePointerWriteAccess,
};
use core::iter::IntoIterRange;

[julio4]

corelib/src/iter/traits/iterator.cairo line 22 at r2 (raw file):
When not explicitly importing impls there was an error of duplicated impl.
By making VecIntoIter impl less generic it works correctly as it can resolved the correct impl:

impl VecIntoIterRange<
    T, impl VecTraitImpl: VecTrait<StoragePath<Vec<T>>>,
> of IntoIterRange<StoragePath<Vec<T>>> { //... }

But then I have to edit PathableVecIntoIterRange to be able to delegate, and I didn't found a way to correctly bound Vec<T> to the StorageAsPath<Vec<T>>::Value. I tried something like this but it's not working:

impl PathableVecIntoIterRange<
    T,
    impl PathImpl: StorageAsPath<Vec<T>>,
    impl VecTraitImpl: VecTrait<StoragePath<PathImpl::Value>>,
> of IntoIterRange<Vec<T>> {
    type IntoIter = VecIter<StoragePath<PathImpl::Value>, VecTraitImpl>;
    #[inline]
    fn into_iter_range(self: Vec<T>, range: Range<u64>) -> Self::IntoIter {
        VecIntoIterRange::<T, VecTraitImpl>::into_iter_range(self.as_path(), range)
    }
    #[inline]
    fn into_iter_full_range(self: Vec<T>) -> Self::IntoIter {
        VecIntoIterRange::<T, VecTraitImpl>::into_iter_full_range(self.as_path())
    }
}

In the end, it was more easy to just duplicate the implementation and not delegate, that's what I did and updated the PR. But if there's any way to correctly do trait bounds I would be curious to know!

[julio4]

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

---

crates/cairo-lang-starknet/cairo_level_tests/collections_test.cairo line 4 at r2 (raw file):

    MutableVecTrait, PathableMutableVecIntoIterRange, PathableVecIntoIterRange, StoragePathEntry,
    StoragePointerReadAccess, StoragePointerWriteAccess,
};

See other comment, :done:

[orizi]

Reviewed 3 of 5 files at r3, all commit messages.
Reviewable status: 4 of 6 files reviewed, 3 unresolved discussions (waiting on @gilbens-starkware and @julio4)

[gilbens-starkware]

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

---

corelib/src/iter/traits/iterator.cairo line 22 at r2 (raw file):

Previously, julio4 (Julio) wrote…

When not explicitly importing impls there was an error of duplicated impl.
By making VecIntoIter impl less generic it works correctly as it can resolved the correct impl:

impl VecIntoIterRange<
    T, impl VecTraitImpl: VecTrait<StoragePath<Vec<T>>>,
> of IntoIterRange<StoragePath<Vec<T>>> { //... }

But then I have to edit PathableVecIntoIterRange to be able to delegate, and I didn't found a way to correctly bound Vec<T> to the StorageAsPath<Vec<T>>::Value. I tried something like this but it's not working:

impl PathableVecIntoIterRange<
    T,
    impl PathImpl: StorageAsPath<Vec<T>>,
    impl VecTraitImpl: VecTrait<StoragePath<PathImpl::Value>>,
> of IntoIterRange<Vec<T>> {
    type IntoIter = VecIter<StoragePath<PathImpl::Value>, VecTraitImpl>;
    #[inline]
    fn into_iter_range(self: Vec<T>, range: Range<u64>) -> Self::IntoIter {
        VecIntoIterRange::<T, VecTraitImpl>::into_iter_range(self.as_path(), range)
    }
    #[inline]
    fn into_iter_full_range(self: Vec<T>) -> Self::IntoIter {
        VecIntoIterRange::<T, VecTraitImpl>::into_iter_full_range(self.as_path())
    }
}

In the end, it was more easy to just duplicate the implementation and not delegate, that's what I did and updated the PR. But if there's any way to correctly do trait bounds I would be curious to know!

That's good.
But in order to implement this trait for future storage object i'd move this trait to storage.cairo and import the impls there too (in order for them to be automatically found).

[julio4]

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

---

corelib/src/iter/traits/iterator.cairo line 22 at r2 (raw file):

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

That's good.
But in order to implement this trait for future storage object i'd move this trait to storage.cairo and import the impls there too (in order for them to be automatically found).

I moved IntoIterRange to starknet::storage

[gilbens-starkware]

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

[orizi]

Reviewed 3 of 5 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @julio4)

---

corelib/src/starknet/storage/vec.cairo line 445 at r6 (raw file):

    fn into_iter_full_range(self: T) -> Self::IntoIter {
        let vec = self.as_path();
        VecIter { current_index: (0..vec.len()).into_iter(), vec }

we currently range check every access - so no need for the length read.

alternatively - check ranges here - and use update instead of get in the iterator implementation.
(probably better)

Suggestion:

        VecIter { current_index: (0..core::num::traits::Bounded::MAX).into_iter(), vec }

[julio4]

Reviewable status: 4 of 6 files reviewed, 1 unresolved discussion (waiting on @gilbens-starkware and @orizi)

---

corelib/src/starknet/storage/vec.cairo line 445 at r6 (raw file):

Previously, orizi wrote…

we currently range check every access - so no need for the length read.

alternatively - check ranges here - and use update instead of get in the iterator implementation.
(probably better)

I'm not sure I understand what you mean by using update?
get return None on out of bound, so I agree we don't need to actually call len there.

Then, is there even a need for a range? We could simply use cur: u64, to not have to create a RangeIterator struct.

Also range end are exclusive, so technically it can only iterate until core::num::traits::Bounded::MAX - 1 if not mistaken, not a big issue but still maybe not exactly the specification.