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

Expose indexing as a language interface #4370

Merged
merged 22 commits into from
Oct 29, 2024

Conversation

brymer-meneses
Copy link
Contributor

@brymer-meneses brymer-meneses commented Oct 4, 2024

This PR makes it so that types can implement the IndexWith interface so that they can provide their custom indexing behavior.

@brymer-meneses brymer-meneses changed the title Expose indexing as a language interface for classes Expose indexing as a language interface Oct 7, 2024
@brymer-meneses
Copy link
Contributor Author

brymer-meneses commented Oct 7, 2024

I think this should be ready for review. The bulk of the test happens at toolchain/check/testdata/operators/overloaded/index.carbon, most of the testdata changes are just importing Core//prelude/operators/index.

Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Thanks! Most of my comments are for crashes on bad inputs. Checking with zygoloid I think we're also getting close to having associated type support, but since it's not ready yet, this seems good for short-term support -- we should just have some TODOs.

toolchain/check/handle_index.cpp Outdated Show resolved Hide resolved
toolchain/check/handle_index.cpp Outdated Show resolved Hide resolved
toolchain/check/handle_index.cpp Outdated Show resolved Hide resolved
toolchain/check/handle_index.cpp Show resolved Hide resolved
toolchain/check/handle_index.cpp Outdated Show resolved Hide resolved
toolchain/check/handle_index.cpp Outdated Show resolved Hide resolved
@brymer-meneses brymer-meneses force-pushed the index-with branch 2 times, most recently from e2dc087 to 8ea0a6b Compare October 28, 2024 23:43
@jonmeow
Copy link
Contributor

jonmeow commented Oct 29, 2024

Thanks for the changes! It looks though like there are still test failures on toolchain/check/testdata/class/fail_redeclaration_introducer.carbon -- where did that file come from? It appears to be unrelated to the indexing change, should it be removed?

@brymer-meneses
Copy link
Contributor Author

I'm also not sure, perhaps i messed up syncing to trunk again--sorry about that!

Copy link
Contributor

@jonmeow jonmeow left a comment

Choose a reason for hiding this comment

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

Thanks!

@brymer-meneses
Copy link
Contributor Author

Not really sure why are the tests failing :<<

auto-merge was automatically disabled October 29, 2024 18:40

Head branch was pushed to by a user without write access

@brymer-meneses
Copy link
Contributor Author

My bad, forgot to add fail_ on tests that are expected to fail

@jonmeow jonmeow added this pull request to the merge queue Oct 29, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Oct 29, 2024
@jonmeow jonmeow added this pull request to the merge queue Oct 29, 2024
Merged via the queue into carbon-language:trunk with commit 89eed42 Oct 29, 2024
8 checks passed
@brymer-meneses brymer-meneses deleted the index-with branch October 29, 2024 23:34
github-merge-queue bot pushed a commit that referenced this pull request Oct 30, 2024
This is in particular to avoid churn from changes such as #4370. I think
the import list can be helpful (particularly to understand what the
library is aware of), but it's a different trade-off for the prelude
package due to the implicit imports.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants