Join GitHub today
GitHub is home to over 31 million developers working together to host and review code, manage projects, and build software together.
Sign upMove Generics from MethodSig to TraitItem and ImplItem #44766
Conversation
rust-highfive
assigned
nikomatsakis
Sep 22, 2017
This comment has been minimized.
This comment has been minimized.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
sunjay
reviewed
Sep 22, 2017
| @@ -1544,15 +1544,15 @@ impl<'a> LoweringContext<'a> { | |||
| } | |||
| TraitItemKind::Method(ref sig, None) => { | |||
| let names = this.lower_fn_args_to_names(&sig.decl); | |||
| hir::TraitItemKind::Method(this.lower_method_sig(sig), | |||
| hir::TraitItemKind::Method(this.lower_method_sig(&i.generics, sig), | |||
This comment has been minimized.
This comment has been minimized.
sunjay
Sep 22, 2017
Author
Member
We should consider updating the HIR to match AST if that is desireable. I have made everything fit for now, but there is now an incongruencey between the structure of the HIR and AST. That's what forced me to add this extra paramater for generics all throughout the code.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Sep 22, 2017
Contributor
I think we will want to update the HIR. I had originally thought we might do that in a separate PR, but I'm not opposed to doing it in the same PR either. It should though be possible to get this code to work without doing it, might be easier for testing and things to start that way (then maybe do the HIR change in a follow-up commit).
This comment has been minimized.
This comment has been minimized.
sunjay
Sep 25, 2017
Author
Member
I think it'll be easier to the the HIR in a follow-up PR. I'll get started on that as soon as this is ready to be merged.
sunjay
reviewed
Sep 22, 2017
| @@ -708,7 +708,6 @@ impl<'a, 'tcx> Visitor<'tcx> for Resolver<'a> { | |||
| ItemRibKind | |||
| } | |||
| FnKind::Method(_, sig, _, _) => { | |||
| self.visit_generics(&sig.generics); | |||
This comment has been minimized.
This comment has been minimized.
sunjay
Sep 22, 2017
Author
Member
I'm worried about whether this change has any side-effects we don't want. I don't think I found a place to add this back in after I deleted it.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Sep 22, 2017
•
Contributor
Indeed, we will need to add this back somewhere =) I think the problem is that the FnKind enum has to change. Methods should operate more analogously with ItemFn -- so we should add a &'a Generics to the Method variant, and then when we construct FnKind::Method (here and here) we can add the data from the trait or impl item respectively. Then we can restore the call to visit_generics that occurs here, I suppose.
This was wrong.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Sep 22, 2017
Contributor
Well, let me read a bit more into this actually. I think what I said is not wrong but a few more tweaks are likely needed.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Sep 22, 2017
Contributor
Yeah, so I think it's fine to remove the call here -- it is being moved into the walk_trait_item and walk_impl_item code, effectively.
This comment has been minimized.
This comment has been minimized.
sunjay
Sep 25, 2017
Author
Member
Is this change really okay? If ItemFn visits generics, shouldn't Method visit them too?
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Sep 25, 2017
Contributor
Nope. It happens as part of visit_trait_item or visit_impl_item, I think.
sunjay
reviewed
Sep 22, 2017
| @@ -2056,6 +2056,7 @@ impl<'a> Resolver<'a> { | |||
| this.with_current_self_type(self_type, |this| { | |||
| for impl_item in impl_items { | |||
| this.check_proc_macro_attrs(&impl_item.attrs); | |||
| this.visit_generics(&impl_item.generics); | |||
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
petrochenkov
Sep 22, 2017
Contributor
@sunjay
You have to visit generics inside of with_type_parameter_rib here and for trait items.
This was done previously for methods in this piece of code below:
this.with_type_parameter_rib(type_parameters, |this| {
visit::walk_impl_item(this, impl_item); // We visited method signature, `FnKind::Method` and its generics here previously
});
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Sep 22, 2017
Contributor
Yes, this change is not quite right. =)
The way that resolve works, it has these "ribs" that are in scope, those are the sets of names that can be used. When you visit the generics, you are then resolving the names that appear within. So when you visit the generics here, we are visiting without any names in scope.
What we want to do then is to move up the code that is specific to methods below, and execute it for all impl-items:
let type_parameters =
HasTypeParameters(&impl_item.generics,
MethodRibKind(!sig.decl.has_self()));
this.with_type_parameter_rib(type_parameters, |this| {...}in that case, we can visit_generics here, and you can .. probably forget what I said about modifying FnKind::Method above.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Sep 22, 2017
Contributor
Actually, I don't think we want to call visit_generics here. It will happen as part of the walk_impl_item that we see below.
This comment has been minimized.
This comment has been minimized.
sunjay
Sep 25, 2017
Author
Member
@nikomatsakis I don't think I can actually move the HasTypeParameters code above because it uses sig which is only in scope for methods. After this refactoring, I may have to come back and add HasTypeParameters for TraitItemKind::Type in this match statement.
sunjay
reviewed
Sep 22, 2017
| let TyParam {ident, bounds, default, ..} = self.parse_ty_param(vec![])?; | ||
| self.expect(&token::Semi)?; | ||
| (ident, TraitItemKind::Type(bounds, default)) | ||
| (ident, TraitItemKind::Type(bounds, default), ast::Generics::default()) |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Sep 22, 2017
Contributor
uh... I'm not sure. =) I wasn't aware that it implemented default. But it's probably right.
sunjay
reviewed
Sep 22, 2017
| @@ -4919,7 +4920,8 @@ impl<'a> Parser<'a> { | |||
|
|
|||
| /// Parse a method or a macro invocation in a trait impl. | |||
| fn parse_impl_method(&mut self, vis: &Visibility, at_end: &mut bool) | |||
| -> PResult<'a, (Ident, Vec<ast::Attribute>, ast::ImplItemKind)> { | |||
| -> PResult<'a, (Ident, Vec<ast::Attribute>, ast::Generics, | |||
| ast::ImplItemKind)> { | |||
This comment has been minimized.
This comment has been minimized.
sunjay
Sep 22, 2017
Author
Member
I had to make this return type even larger since Generics is outside MethodSig now. There was a lot of code relying on that structure.
This comment has been minimized.
This comment has been minimized.
sunjay
reviewed
Sep 22, 2017
| @@ -558,13 +557,13 @@ pub fn walk_fn<'a, V>(visitor: &mut V, kind: FnKind<'a>, declaration: &'a FnDecl | |||
| pub fn walk_trait_item<'a, V: Visitor<'a>>(visitor: &mut V, trait_item: &'a TraitItem) { | |||
| visitor.visit_ident(trait_item.span, trait_item.ident); | |||
| walk_list!(visitor, visit_attribute, &trait_item.attrs); | |||
| visitor.visit_generics(&trait_item.generics); | |||
This comment has been minimized.
This comment has been minimized.
sunjay
Sep 22, 2017
Author
Member
I had to guess here and some other places that this was the right place to add this code. Would be great if you could check.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@nikomatsakis I am currently experiencing a compiler panic when I try to compile this. Given that I am only compiling the compiler and not actually running it, I don't understand where I could have introduced the bug. I would love to know what's going on and would appreciate it if you could give me an idea of how to troubleshoot. The error message I'm receiving is below. Once this is fixed I would like to run the tests and fix any other errors that are found before this gets merged.
|
nikomatsakis
reviewed
Sep 22, 2017
| @@ -585,6 +584,7 @@ pub fn walk_impl_item<'a, V: Visitor<'a>>(visitor: &mut V, impl_item: &'a ImplIt | |||
| visitor.visit_vis(&impl_item.vis); | |||
| visitor.visit_ident(impl_item.span, impl_item.ident); | |||
| walk_list!(visitor, visit_attribute, &impl_item.attrs); | |||
| visitor.visit_generics(&impl_item.generics); | |||
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Sep 22, 2017
Contributor
...i.e., here, when resolve invokes walk_impl_item, it will walk the generics here
nikomatsakis
reviewed
Sep 22, 2017
| @@ -1845,6 +1844,7 @@ impl<'a> Resolver<'a> { | |||
|
|
|||
| for trait_item in trait_items { | |||
| this.check_proc_macro_attrs(&trait_item.attrs); | |||
| this.visit_generics(&trait_item.generics); | |||
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Sep 22, 2017
Contributor
we should not be invoking visit_generics here. IT will happen when we invoke walk_trait_item below. Instead, we should be inserting the type-parameters rib:
let type_parameters = HasTypeParameters(&trait_item.generics, MethodRibKind(!sig.decl.has_self()));
this.with_type_parameter_rib(type_parameters, |this| { /* old code goes here */ })
shepmaster
added
the
S-waiting-on-author
label
Sep 22, 2017
sunjay
reviewed
Sep 25, 2017
| @@ -1861,7 +1860,7 @@ impl<'a> Resolver<'a> { | |||
| } | |||
| TraitItemKind::Method(ref sig, _) => { | |||
| let type_parameters = | |||
| HasTypeParameters(&sig.generics, | |||
| HasTypeParameters(&trait_item.generics, | |||
This comment has been minimized.
This comment has been minimized.
sunjay
Sep 25, 2017
Author
Member
I think we'll need to add this type parameter rib to each variant in this match statement because each HasTypeParamters will need a different rib kind. Since nothing other than methods has generics right now, would it work to save that change until we are actually implementing associated type generics?
(this applies to both trait items and impl items)
This comment has been minimized.
This comment has been minimized.
nikomatsakis
Sep 25, 2017
Contributor
Well, it depends. I would think we just want to rename MethodRibKind to something else like TraitOrImplItemRibKind, but it might be important to distinguish those cases for error messages? I kinda' doubt it, but have to look around.
nikomatsakis
approved these changes
Sep 25, 2017
|
At first glance, this is looking good! |
This comment has been minimized.
This comment has been minimized.
|
@nikomatsakis The build passed! |
nikomatsakis
approved these changes
Sep 25, 2017
This comment has been minimized.
This comment has been minimized.
|
@bors r+ |
This comment has been minimized.
This comment has been minimized.
|
|
sunjay
referenced this pull request
Sep 25, 2017
Closed
HIR - Move Generics from MethodSig to TraitItem and ImplItem #44852
This comment has been minimized.
This comment has been minimized.
|
@bors r+ |
This comment has been minimized.
This comment has been minimized.
|
|
arielb1
added
S-waiting-on-bors
and removed
S-waiting-on-author
labels
Sep 26, 2017
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Sep 27, 2017
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
OK, so this failed building rustfmt, which is being built because of the RLS. I'm not really sure what needs to happen here. Can we set toolstate to "broken" for the RLS? Do we have to (a) open a PR fixing rustfmt, (b) redirect RLS in its cargo.toml to use that PR, then (c) land this patch, then (d) land that PR, then (e) redirect RLS back? cc @nrc @rust-lang/dev-tools @rust-lang/compiler -- sorry for the broad cc here but I'm actually unclear on who precisely I ought to cc! |
This comment has been minimized.
This comment has been minimized.
|
Sure yeah I can write out how this works, sorry we don't have some great instructions for this yet! If it works out here, though, maybe we can write them down in
Eventually we'll be able to set the tools to "broken" temporarily to skip a lot of these steps, but unfortunately we're not there yet for the rls/rustfmt. :( Lemme know if there's any questions though! |
shepmaster
added
S-waiting-on-author
and removed
S-waiting-on-bors
labels
Sep 29, 2017
sunjay
added some commits
Sep 25, 2017
This comment has been minimized.
This comment has been minimized.
Sounds good! That's much easier. Now that I've cleared this PR and marked the tools as broken, it should be ready for final review and approval from @nikomatsakis. :) |
This comment has been minimized.
This comment has been minimized.
|
@alexcrichton Any idea why the build is failing? |
This comment has been minimized.
This comment has been minimized.
|
Looks like an incremental test is failing. |
kennytm
added
S-waiting-on-author
and removed
S-waiting-on-review
labels
Oct 18, 2017
kennytm
added a commit
to kennytm/rust
that referenced
this pull request
Oct 18, 2017
This comment has been minimized.
This comment has been minimized.
|
@bors r+ |
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Oct 23, 2017
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
|
|
This comment has been minimized.
This comment has been minimized.
bors
added a commit
that referenced
this pull request
Oct 24, 2017
This comment has been minimized.
This comment has been minimized.
|
|
sunjay commentedSep 22, 2017
•
edited
As part of
rust-impl-period/WG-compiler-traits, we want to "lift"GenericsfromMethodSigintoTraitItemandImplItem. This is in preparation for adding associated type generics. (#44265 (comment))Currently this change is only made in the AST. In the future, it may also impact the HIR. (Still discussing)
To understand this PR, it's probably best to start from the changes to
ast.rsand then work your way to the other files to understand the far reaching effects of this change.r? @nikomatsakis