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): Extend trait #7132

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

Conversation

julio4
Copy link
Contributor

@julio4 julio4 commented Jan 21, 2025

  • Define the Extend trait: Extend a collection with the contents of an iterator.
  • Implement ArrayExtend

Examples

let mut arr = array![1_u32, 2, 3];
arr.extend((4..6_u32));
assert_eq!(arr, array![1, 2, 3, 4, 5]);

Known Issue

DIrectly calling into_iter in the argument cause panic.

let mut arr = array![1, 2];
arr.extend(array![3, 4, 5].into_iter()); // fails

@reviewable-StarkWare
Copy link

This change is Reviewable

@julio4 julio4 force-pushed the feat/extend_trait branch from 4c56ebb to 0ee7864 Compare January 23, 2025 22:23
@julio4 julio4 changed the title feat(corelib): Extend trait (wip) feat(corelib): Extend trait Jan 23, 2025
@julio4 julio4 marked this pull request as ready for review January 23, 2025 22:23
@julio4 julio4 force-pushed the feat/extend_trait branch from 0ee7864 to ac16d9b Compare January 27, 2025 20:44
@julio4 julio4 force-pushed the feat/extend_trait branch from ac16d9b to 610d9b2 Compare February 4, 2025 17:55
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.

@orizi for visibility

Reviewable status: 0 of 5 files reviewed, all discussions resolved

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.

this now should work (if you rebase).
though - if IntoIterator is called from the outside it would probably not work for the time being (with a diagnostic).
but adding additional type info should work.

Reviewed 3 of 5 files at r3, all commit messages.
Reviewable status: 3 of 5 files reviewed, 1 unresolved discussion (waiting on @julio4)


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

    arr.extend((4..6_u32).into_iter());
    assert_eq!(arr, array![1, 2, 3, 4, 5]);
}

Suggestion:

#[test]
fn test_array_extend() {
    let mut arr = array![1_u32, 2, 3];
    arr.extend((4..6_u32).into_iter());
    assert_eq!(arr, array![1, 2, 3, 4, 5]);
}

@julio4 julio4 force-pushed the feat/extend_trait branch from 610d9b2 to 973557b Compare February 5, 2025 12:40
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.

I'm curious to know why calling IntoIterator from the outside is failing.
IntoIterator::into_iter() always return an Iterator, and all Iterators implement IntoIterator.
I'm not sure how to add additional type infos

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


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

    arr.extend((4..6_u32).into_iter());
    assert_eq!(arr, array![1, 2, 3, 4, 5]);
}

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


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

fn test_array_extend() {
    let mut arr = array![1_u32, 2, 3];
    arr.extend((4..6_u32).into_iter());

Suggestion:

    arr.extend(4..6_u32);

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

// fn test_array_extend_panic() {
//     let mut arr: Array<u32> = array![1, 2, 3];
//     arr.extend((4..6_u32).into_iter());

remove the commented test.

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.

it has something to do with the fact that we have an impl::impl::something sort of usage - it causes some sort of an internal mix which is difficult to solve.

adding extra info would just work with an additional variable with a specific type before passing it as a param.

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

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


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

Previously, orizi wrote…

remove the commented test.

Done.


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

fn test_array_extend() {
    let mut arr = array![1_u32, 2, 3];
    arr.extend((4..6_u32).into_iter());

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


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

fn test_array_extend() {
    let mut arr = array![1_u32, 2, 3];
    arr.extend((4..6_u32));

Suggestion:

    arr.extend(4..6_u32);

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: all files reviewed, 1 unresolved discussion (waiting on @julio4)


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

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.

3 participants