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

[WIP] Generic Associated Types Name Resolution #46146

Closed
wants to merge 12 commits into from
Closed

[WIP] Generic Associated Types Name Resolution #46146

wants to merge 12 commits into from

Conversation

sunjay
Copy link
Member

@sunjay sunjay commented Nov 21, 2017

This PR builds off of the to-be-merged PR for parsing generic associated types. In fact, the only new commit is the latest one which makes changes to src/librustc_resolve/lib.rs. You should look only at that file when reviewing this PR. Once the other PR is merged and this PR is rebased, the remaining files will disappear.

This PR is a work in progress and only meant to give me a way to show @nikomatsakis my progress and ask questions. I am still waiting for the build to finish locally on my computer and then I will update this PR as I find and fix errors.

Tracking Issue: #44265

r? @nikomatsakis

Notes & Questions for Reviewers

  • Something to check: Am I using the right RibKind variants? I don't actually fully understand the differences from the docs in the code so it's quite possible that I didn't choose the right one.

  • From the instructions by @nikomatsakis on the Tracking Issue:

    Now that generics are in place on every trait/impl item, I think we probably want to remove the handling for type and extract the method handling so that it occurs at a higher level. For the items (e.g., const) where there are no generics, then the newly introduced rib ought to be empty and hence harmless (I hope).

    I had to modify this a bit because I chose different RibKinds for Methods and Types. If it turns out the RibKind should be the same in all cases, I can lift out that call to with_type_parameter_rib and add the HasTypeParameters rib only once.

  • By the end of this, the run-pass tests should actually pass! 🎉

  • It's possible that I still need to add more tests before this should be merged

@@ -1895,7 +1895,9 @@ impl<'a> Resolver<'a> {
});
}
TraitItemKind::Type(..) => {
this.with_type_parameter_rib(NoTypeParameters, |this| {
let type_parameters = HasTypeParameters(&trait_item.generics,
ItemRibKind);
Copy link
Member Author

Choose a reason for hiding this comment

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

Is ItemRibKind the right RibKind?

// We also need a new scope for the associated type
// specific type parameters.
let type_parameters =
HasTypeParameters(&impl_item.generics, ItemRibKind);
Copy link
Member Author

Choose a reason for hiding this comment

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

Is ItemRibKind the right RibKind? Is this the right way to visit these types? I copied what was done for methods a few lines above this.

@petrochenkov petrochenkov self-assigned this Nov 21, 2017
@kennytm kennytm added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Nov 21, 2017
@bors
Copy link
Contributor

bors commented Nov 21, 2017

☔ The latest upstream changes (presumably #45771) made this pull request unmergeable. Please resolve the merge conflicts.

…ld which was never used. Lifting the HasTypeParameters rib to all trait item kinds and all impl item kinds
@sunjay
Copy link
Member Author

sunjay commented Nov 22, 2017

This has been completed and merged into the other PR I had open for parsing (#45904).

@sunjay sunjay closed this Nov 22, 2017
@sunjay sunjay deleted the gat-resolve branch November 22, 2017 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants