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): IntoIterator doc/tests, iter::traits::collect module #6991

Merged
merged 6 commits into from
Jan 6, 2025

Conversation

julio4
Copy link
Contributor

@julio4 julio4 commented Jan 4, 2025

  • Add IntoIterator documentation
  • Add IntoIterator tests
  • Moved IntoIterator from crate::iter::traits::iterator in new module crate::iter::traits::collect. This is to keep the definitions/impls of similar traits for converting collections to/from iterators at the same place (I also implemented FromIterator that depends on this PR), similar to rust standard lib.
  • Changed IntoIterator::into_iter return to Self::IntoIter for consistency

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Contributor

@enitrat enitrat left a comment

Choose a reason for hiding this comment

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

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


corelib/src/iter/traits/collect.cairo line 10 at r1 (raw file):

/// with Cairo's `for` loop syntax](crate::iter#for-loops-and-intoiterator).
///
/// See also: [`FromIterator`].

IntoIterator ?

Code quote:

See also: [`FromIterator`].

corelib/src/iter/traits/collect.cairo line 67 at r1 (raw file):

/// // ... and then turn it into an Iterator:
/// let mut n = 0;
/// for i in c.into_iter() {

do you need an explicit into_iter ?


corelib/src/iter/traits/collect.cairo line 71 at r1 (raw file):

///     n += 1;
/// };
/// ```

would be nice to add the doc about using IntoIterator as a trait bound. same example applies

Copy link
Contributor Author

@julio4 julio4 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: 8 of 10 files reviewed, 3 unresolved discussions (waiting on @enitrat)


corelib/src/iter/traits/collect.cairo line 10 at r1 (raw file):

Previously, enitrat (Mathieu) wrote…

IntoIterator ?

Sorry, didn't push the implementation of FromIterator yet, just need to add tests. I'll remove this for now


corelib/src/iter/traits/collect.cairo line 67 at r1 (raw file):

Previously, enitrat (Mathieu) wrote…

do you need an explicit into_iter ?

No not in for loops, good catch


corelib/src/iter/traits/collect.cairo line 71 at r1 (raw file):
Do you mean the collect_as_strings function? It's a bit more tricky to implement now as map and collect is not yet merged in main. The current implementation might be a bit complex/verbose for an example due to IntoIterator/Iterator Item bounds:

fn collect_as_strings<
    T,
    impl TIntoIterator: IntoIterator<T>,
    +core::fmt::Debug<TIntoIterator::Iterator::<TIntoIterator::IntoIter>::Item>,
    +Drop<TIntoIterator::Iterator::<TIntoIterator::IntoIter>::Item>,
    +Drop<TIntoIterator::IntoIter>,
>(
    collection: T,
) -> Array<ByteArray> {
    let mut res = array![];
    for item in collection {
        res.append(format!("{item:?}"));
    };
    res
}

#[test]
fn test_into_iter_trait_bound() {
    let arr = array![0, 1, 2];
    let res = collect_as_strings(arr);
    assert_eq!(res, array!["0", "1", "2"]);
}

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: 8 of 10 files reviewed, 4 unresolved discussions (waiting on @enitrat and @julio4)


corelib/src/iter/traits/collect.cairo line 10 at r1 (raw file):

Previously, julio4 (Julio) wrote…

Sorry, didn't push the implementation of FromIterator yet, just need to add tests. I'll remove this for now

more so - please do not add it in this PR - it is already large enough as it is.


corelib/src/array.cairo line 845 at r2 (raw file):

    type IntoIter = ArrayIter<T>;
    #[inline]
    fn into_iter(self: Array<T>) -> Self::IntoIter {

unrelated - revert.

Code quote:

    type IntoIter = SpanIter<T>;
    #[inline]
    fn into_iter(self: Span<T>) -> Self::IntoIter {
        SpanIter { span: self }
    }
}

impl ArrayIntoIterator<T> of crate::iter::IntoIterator<Array<T>> {
    type IntoIter = ArrayIter<T>;
    #[inline]
    fn into_iter(self: Array<T>) -> Self::IntoIter {

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: 8 of 10 files reviewed, 5 unresolved discussions (waiting on @enitrat and @julio4)


corelib/src/ops/range.cairo line 139 at r2 (raw file):

> of IntoIterator<Range<T>> {
    type IntoIter = internal::IntRange<T>;
    #[inline]

unrelated - revert.

Code quote:

    type IntoIter = RangeIterator<T>;
    fn into_iter(self: Range<T>) -> Self::IntoIter {
        let start = self.start;
        let end = self.end;
    T, +Copy<T>, +Drop<T>, +SierraIntRangeSupport<T>,
> of IntoIterator<Range<T>> {
    type IntoIter = internal::IntRange<T>;
    #[inline]

@julio4 julio4 force-pushed the feat/into_iter_collect_mod branch from bf9f9b2 to fae5018 Compare January 5, 2025 14:51
Copy link
Contributor Author

@julio4 julio4 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: 8 of 10 files reviewed, 5 unresolved discussions (waiting on @enitrat and @orizi)


corelib/src/array.cairo line 845 at r2 (raw file):

Previously, orizi wrote…

unrelated - revert.

Done.


corelib/src/iter/traits/collect.cairo line 10 at r1 (raw file):

Previously, orizi wrote…

more so - please do not add it in this PR - it is already large enough as it is.

Yes, will open after this one is merged!


corelib/src/ops/range.cairo line 139 at r2 (raw file):

Previously, orizi wrote…

unrelated - revert.

Done.

@julio4 julio4 force-pushed the feat/into_iter_collect_mod branch from fae5018 to 3408ad8 Compare January 5, 2025 14:52
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 1 of 10 files at r1, 2 of 2 files at r3, all commit messages.
Reviewable status: 8 of 10 files reviewed, 6 unresolved discussions (waiting on @enitrat and @julio4)


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

    /// ```
    /// let v = array![1, 2, 3];
    /// let mut iter = v.into_iter();

consider doing these more often

Suggestion:

    /// let mut iter = array![1, 2, 3].into_iter();

corelib/src/test/iter_test.cairo line 1 at r3 (raw file):

use crate::iter::IntoIterator;

in prelude now.


corelib/src/test/iter_test.cairo line 1 at r3 (raw file):

use crate::iter::IntoIterator;

i don't understand what is tested here.

if it is the for it is in for_test.cairo
if it is the array it is tested in array_test.cairo


corelib/src/test/iter_test.cairo line 11 at r3 (raw file):

    assert_eq!(Option::Some(2), iter.next());
    assert_eq!(Option::Some(3), iter.next());
    assert_eq!(Option::None, iter.next());

Suggestion:

    let mut iter = array![1, 2, 3].into_iter();
    assert_eq!(Option::Some(1), iter.next());
    assert_eq!(Option::Some(2), iter.next());
    assert_eq!(Option::Some(3), iter.next());
    assert_eq!(Option::None, iter.next());

Copy link
Contributor

@enitrat enitrat 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: 8 of 10 files reviewed, 3 unresolved discussions (waiting on @julio4)


corelib/src/iter/traits/collect.cairo line 71 at r1 (raw file):

Previously, julio4 (Julio) wrote…

Do you mean the collect_as_strings function? It's a bit more tricky to implement now as map and collect is not yet merged in main. The current implementation might be a bit complex/verbose for an example due to IntoIterator/Iterator Item bounds:

fn collect_as_strings<
    T,
    impl TIntoIterator: IntoIterator<T>,
    +core::fmt::Debug<TIntoIterator::Iterator::<TIntoIterator::IntoIter>::Item>,
    +Drop<TIntoIterator::Iterator::<TIntoIterator::IntoIter>::Item>,
    +Drop<TIntoIterator::IntoIter>,
>(
    collection: T,
) -> Array<ByteArray> {
    let mut res = array![];
    for item in collection {
        res.append(format!("{item:?}"));
    };
    res
}

#[test]
fn test_into_iter_trait_bound() {
    let arr = array![0, 1, 2];
    let res = collect_as_strings(arr);
    assert_eq!(res, array!["0", "1", "2"]);
}

ok, i'll note this on the side

Copy link
Contributor Author

@julio4 julio4 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: 8 of 10 files reviewed, 3 unresolved discussions (waiting on @enitrat and @orizi)


corelib/src/test/iter_test.cairo line 1 at r3 (raw file):

Previously, orizi wrote…

in prelude now.

Done.


corelib/src/test/iter_test.cairo line 1 at r3 (raw file):

Previously, orizi wrote…

i don't understand what is tested here.

if it is the for it is in for_test.cairo
if it is the array it is tested in array_test.cairo

I wanted to test a custom implementation of IntoIterator, same as shown in the documentation.
We can remove it


corelib/src/test/iter_test.cairo line 11 at r3 (raw file):

    assert_eq!(Option::Some(2), iter.next());
    assert_eq!(Option::Some(3), iter.next());
    assert_eq!(Option::None, iter.next());

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.

Reviewable status: 8 of 10 files reviewed, 3 unresolved discussions (waiting on @enitrat and @julio4)


corelib/src/test/iter_test.cairo line 1 at r3 (raw file):

Previously, julio4 (Julio) wrote…

I wanted to test a custom implementation of IntoIterator, same as shown in the documentation.
We can remove it

but all implementations are custom i just don't see what this adds.

more so for the first test - which is just a duplication of array iterator tests.

Copy link
Contributor Author

@julio4 julio4 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: 7 of 10 files reviewed, 3 unresolved discussions (waiting on @enitrat and @orizi)


corelib/src/test/iter_test.cairo line 1 at r3 (raw file):

Previously, orizi wrote…

but all implementations are custom i just don't see what this adds.

more so for the first test - which is just a duplication of array iterator tests.

I see, so we'll add tests only for specific features on Iterator not for testing default trait impls.
I removed iter_tests.

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: 7 of 10 files reviewed, 4 unresolved discussions (waiting on @enitrat and @julio4)


a discussion (no related file):
Can you explain the logic of IntoIterator being in collect module?

@julio4
Copy link
Contributor Author

julio4 commented Jan 6, 2025

Previously, orizi wrote…

Can you explain the logic of IntoIterator being in collect module?

I followed the rust standard library.
I agree that collect is not the perfect name, as it's more like traits that allow conversion of type from/to Iterator.
But because Cairo is strongly following rust it's easier to keep track of what is implemented or not when the modules organization is similar.

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 4 of 10 files at r1, 1 of 2 files at r4, 2 of 2 files at r5.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @julio4)

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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @julio4)


a discussion (no related file):
@gilbens-starkware

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.

:lgtm:

:lgtm:

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

@orizi orizi added this pull request to the merge queue Jan 6, 2025
Merged via the queue into starkware-libs:main with commit a12d7d0 Jan 6, 2025
47 checks passed
@julio4 julio4 deleted the feat/into_iter_collect_mod branch January 6, 2025 20:11
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.

5 participants