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): snapshot fixed-size array iterator #6944

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

cairolover
Copy link
Contributor

Adds IntoIter and Iterator implementations for the fixed-size array [T; SIZE] type.

I am encountering issues where the trait implementation cannot be found unless

  1. explicitly imported
  2. if I define the implementation next to the definition of the Iterator trait.

tracking bug in #6943

@reviewable-StarkWare
Copy link

This change is Reviewable

@@ -1,5 +1,6 @@
use crate::iter::{IntoIterator, Iterator};
use crate::test::test_utils::assert_eq;
use crate::array::{EmptyFixedSizeArrayIntoIterator, FixedSizeArrayIntoIterator};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

demonstrates the requirement for an explicit import

Copy link
Collaborator

Choose a reason for hiding this comment

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

it isn't required - or at least it really shouldn't.

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: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @cairolover)


a discussion (no related file):
the return value for fixed sized span should probably not be a snapshot - probably means we need a more explicit definition for this.


corelib/src/array.cairo line 725 at r1 (raw file):

        crate::array::EmptyFixedSizeArrayImpl::<T>::span(@self).into_iter()
    }
}

and move to near Iterator trait defintion (as it is for a language construct defintion), so it won't require an explicit import.

Suggestion:

impl FixedSizeArrayIntoIterator<
    T, const SIZE: usize, +Drop<T>,
> of crate::iter::IntoIterator<[T; SIZE]> {
    type IntoIter = crate::array::SpanIter<T>;
    fn into_iter(self: [T; SIZE]) -> Self::IntoIter {
        self.span().into_iter()
    }
}

@@ -1,5 +1,6 @@
use crate::iter::{IntoIterator, Iterator};
use crate::test::test_utils::assert_eq;
use crate::array::{EmptyFixedSizeArrayIntoIterator, FixedSizeArrayIntoIterator};
Copy link
Collaborator

Choose a reason for hiding this comment

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

it isn't required - or at least it really shouldn't.

Copy link
Contributor Author

@cairolover cairolover 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: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @orizi)


corelib/src/array.cairo line 725 at r1 (raw file):

Previously, orizi wrote…

and move to near Iterator trait defintion (as it is for a language construct defintion), so it won't require an explicit import.

This unfortunately does not work as I'm getting:

Cannot infer negative impl in \`core::array::FixedSizeArrayToSpan::<T, SIZE, \_>\` as it contains the unresolved type \`\[T; SIZE\]\`

hence why I was using two impls, for the empty-sized and non-empty FixedSizeArray

Copy link
Contributor Author

@cairolover cairolover 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: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @orizi)


a discussion (no related file):

Previously, orizi wrote…

the return value for fixed sized span should probably not be a snapshot - probably means we need a more explicit definition for this.

can you expand on this? I'm not sure to understand

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: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @cairolover)


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

        self.span().into_iter()
    }
}

This probably additionally requires a Desnap iterator of iterator - since iteration on flat-size-array should be denapped.

with the additional @ the type is now more correct.

Suggestion:

impl FixedSizeArrayIntoIterator<
    T, const SIZE: usize, +Drop<T>,
> of crate::iter::IntoIterator<@[T; SIZE]> {
    type IntoIter = crate::array::SpanIter<T>;
    fn into_iter(self: [T; SIZE]) -> Self::IntoIter {
        self.span().into_iter()
    }
}

Copy link
Contributor Author

@cairolover cairolover 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: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @orizi)


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

Previously, orizi wrote…

This probably additionally requires a Desnap iterator of iterator - since iteration on flat-size-array should be denapped.

with the additional @ the type is now more correct.

Still getting the issue about:

Cannot infer negative impl in `core::array::FixedSizeArrayToSpan::<T, SIZE, _>` as it contains the unresolved type `[T; SIZE]`

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: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @cairolover)


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

Previously, cairolover (cairolover) wrote…

Still getting the issue about:

Cannot infer negative impl in `core::array::FixedSizeArrayToSpan::<T, SIZE, _>` as it contains the unresolved type `[T; SIZE]`

request the impl for ToSpan by adding:
+core::array::ToSpanTrait<[T; SIZE>`

Copy link
Contributor Author

@cairolover cairolover 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: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @orizi)


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

Previously, orizi wrote…

request the impl for ToSpan by adding:
+core::array::ToSpanTrait<[T; SIZE>`

Done.

Works fine now. Although, it requires explicit cast of a [T; usize] into a @[T; usize] when using iter methods

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 3 files at r4, all commit messages.
Reviewable status: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @cairolover)


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

Previously, cairolover (cairolover) wrote…

Done.

Works fine now. Although, it requires explicit cast of a [T; usize] into a @[T; usize] when using iter methods

to be expected - as i said earlier - this would probably require us having a desnap of an iterator - so that we'd be able to have the value of the iterator properly work.


corelib/src/test/language_features/for_test.cairo line 68 at r4 (raw file):

    assert_eq!(sum, 46);
}

no need here - this is a test for the for concept - this adds nothing specific for the test.


corelib/src/test/array_test.cairo line 221 at r4 (raw file):

        i += 1;
    }
}

Suggestion:

#[test]
fn test_empty_fixed_size_array_iterator() {
    let mut input: [usize; 0] = [];
    assert_eq!((@input).into_iter().next(), Option::None);
}

Copy link
Contributor Author

@cairolover cairolover 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: 1 of 4 files reviewed, 3 unresolved discussions (waiting on @orizi)


corelib/src/test/language_features/for_test.cairo line 68 at r4 (raw file):

Previously, orizi wrote…

no need here - this is a test for the for concept - this adds nothing specific for the test.

Done.


corelib/src/test/array_test.cairo line 221 at r4 (raw file):

        i += 1;
    }
}

Done.
I can't inline this in a single line as the value passed to next() must be an explicitly defined mutable variable. It would be nice to make this possible at the compiler level

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 4 files at r2, 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @cairolover)


a discussion (no related file):

Previously, cairolover (cairolover) wrote…

can you expand on this? I'm not sure to understand

if i'm iterating over [1, 4 , 7] i should get 1 4 and 7 - not @1 @4 and @7


corelib/src/test/array_test.cairo line 210 at r5 (raw file):

        i += 1;
    }
}

Suggestion:

    let mut iter = (@[10_usize, 11, 12, 13]).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());
}

Copy link
Contributor Author

@cairolover cairolover 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: 3 of 4 files reviewed, 2 unresolved discussions (waiting on @orizi)


corelib/src/test/array_test.cairo line 210 at r5 (raw file):

        i += 1;
    }
}

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 1 of 1 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @cairolover)


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

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:

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


a discussion (no related file):

Previously, orizi wrote…

@gilbens-starkware for 2nd eye.

.

@cairolover cairolover changed the title feat(corelib): fixed-size array iterator feat(corelib): snapshot fixed-size array iterator Dec 29, 2024
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:

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

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