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

[stdlib] List __getitem__ returns auto-dereferenced ref #2847

Closed
wants to merge 3 commits into from

Conversation

mikowals
Copy link
Contributor

With this, List.__getitem__() no longer makes copies when returning a value. I also added a test to show that setting an individual field using sugared my_list[0].value = 1 no longer produces extra copies. Passing that test is why __getitem__ receives a Reference[Self, ...] argument rather than just self.

I removed uses of __get_ref in list.mojo and test_list.mojo other than leaving one test to show that __get_ref was still functioning correctly until it is removed. I left usage of List.__get_ref in other modules untouched. From a few quick efforts to eliminate __get_ref it was leading to some lifetime casting decisions that seemed complex and numerous enough they should be handled separately.

This supersedes #2544 which I will close. It should close #2432 and #2540 as __refitem__ and it related difficulties are no longer relevant.

@mikowals mikowals requested a review from a team as a code owner May 27, 2024 04:12
@mikowals mikowals changed the title List __getitem__ returns auto-dereferenced ref [stdlib] List __getitem__ returns auto-dereferenced ref May 27, 2024
@JoeLoser
Copy link
Collaborator

!sync

@modularbot modularbot added the imported-internally Signals that a given pull request has been imported internally. label May 27, 2024
@JoeLoser
Copy link
Collaborator

Unfortunately this is hitting some compiler crashes internally. I'll file a GitHub issue, but until it's solved, I don't see an easy workaround for this really.

@JoeLoser JoeLoser added the blocked Blocked by another issue that must be resolved first label May 27, 2024
@mikowals mikowals force-pushed the list-getitem-no-copy branch 3 times, most recently from fe95cea to 3db296c Compare June 3, 2024 21:52
@JoeLoser
Copy link
Collaborator

!sync

Signed-off-by: Michael Kowalski <1331470+mikowals@users.noreply.github.com>
Signed-off-by: Michael Kowalski <1331470+mikowals@users.noreply.github.com>
Signed-off-by: Michael Kowalski <1331470+mikowals@users.noreply.github.com>
@ConnorGray
Copy link
Collaborator

Hey @mikowals I just wanted to share an update on this PR. @lattner personally had a look at the bug holding this up🙂

He mentioned the error message we were seeing could use some improvement, but realized that the issue was an ambiguity between whether the compiler should call List.__getitem__ or List.__setitem__ at a particular callsite.

He was able to fix this by creating a patch that updated List.__getitem__ to return a ref [_] T and additionally removed List.__setitem__ entirely. That patch was merged yesterday evening, and should be visible during the next nightly release.

The other callsites updated and tests you added in this PR are still awesome to have, so I'm going rebase this PR and merge those changes in :)

Thank you again for pushing on this improvement and your patience while it was investigated and fixed 🙂

If you're interested in continuing work in this vein ⛏️ , there's still a lot of low-hanging fruit in terms of places in the stdlib that need to be updated to use reference—we'd love to work with you on any future contributions that interest you🙂

@ConnorGray
Copy link
Collaborator

!sync

@modularbot
Copy link
Collaborator

✅🟣 This contribution has been merged 🟣✅

Your pull request has been merged to the internal upstream Mojo sources. It will be reflected here in the Mojo repository on the nightly branch during the next Mojo nightly release, typically within the next 24-48 hours.

We use Copybara to merge external contributions, click here to learn more.

@modularbot modularbot added the merged-internally Indicates that this pull request has been merged internally label Jun 25, 2024
modularbot added a commit that referenced this pull request Jun 26, 2024
…40663)

[External] [stdlib] Add tests for `List.__getitem__` returns auto-dereferenced ref

`List.__getitem__()` no longer makes copies when returning a
value. I also added a test to show that setting an individual field
using sugared `my_list[0].value = 1` no longer produces extra copies.
Passing that test is why `__getitem__` receives a `Reference[Self, ...]`
argument rather than just `self`.

I removed uses of `__get_ref` in `list.mojo` and `test_list.mojo` other
than leaving one test to show that `__get_ref` was still functioning
correctly until it is removed. I left usage of `List.__get_ref` in other
modules untouched. From a few quick efforts to eliminate `__get_ref` it
was leading to some lifetime casting decisions that seemed complex and
numerous enough they should be handled separately.

ORIGINAL_AUTHOR=Michael Kowalski
<1331470+mikowals@users.noreply.github.com>
PUBLIC_PR_LINK=#2847

Co-authored-by: Michael Kowalski <1331470+mikowals@users.noreply.github.com>
Closes #2847
MODULAR_ORIG_COMMIT_REV_ID: 3a03e1669aad93bd512210b373c05df0e2adb313
@modularbot modularbot added the merged-externally Merged externally in public mojo repo label Jun 26, 2024
@modularbot
Copy link
Collaborator

Landed in 7c00fc9! Thank you for your contribution 🎉

@lattner
Copy link
Collaborator

lattner commented Jun 26, 2024

Amazing, thank you @mikowals !!

modularbot added a commit that referenced this pull request Sep 13, 2024
…40663)

[External] [stdlib] Add tests for `List.__getitem__` returns auto-dereferenced ref

`List.__getitem__()` no longer makes copies when returning a
value. I also added a test to show that setting an individual field
using sugared `my_list[0].value = 1` no longer produces extra copies.
Passing that test is why `__getitem__` receives a `Reference[Self, ...]`
argument rather than just `self`.

I removed uses of `__get_ref` in `list.mojo` and `test_list.mojo` other
than leaving one test to show that `__get_ref` was still functioning
correctly until it is removed. I left usage of `List.__get_ref` in other
modules untouched. From a few quick efforts to eliminate `__get_ref` it
was leading to some lifetime casting decisions that seemed complex and
numerous enough they should be handled separately.

ORIGINAL_AUTHOR=Michael Kowalski
<1331470+mikowals@users.noreply.github.com>
PUBLIC_PR_LINK=#2847

Co-authored-by: Michael Kowalski <1331470+mikowals@users.noreply.github.com>
Closes #2847
MODULAR_ORIG_COMMIT_REV_ID: 3a03e1669aad93bd512210b373c05df0e2adb313
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Blocked by another issue that must be resolved first imported-internally Signals that a given pull request has been imported internally. merged-externally Merged externally in public mojo repo merged-internally Indicates that this pull request has been merged internally
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants