HIR - Move Generics from MethodSig to TraitItem and ImplItem #44852
Conversation
…nt to support generics in each variant of TraitItem and ImplItem
|
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. |
| FnKind::Method(_, sig, ..) => { | ||
| visitor.visit_generics(&sig.generics); | ||
| } | ||
| FnKind::Method(..) | |
There was a problem hiding this comment.
Note that this is a removal. If you trace the code up to the usages of walk_fn, I don't see where else this is used. I think it should be alright, but it's worth noting the consequences of this change.
There was a problem hiding this comment.
I'll try to do a look to get a feel for whether there will be impact -- but as we said before, really this call is just moving into visit_trait_item and visit_impl_item, so it's not like it's a pure removal.
| impl_m_span | ||
| } | ||
| } | ||
| _ => bug!("{:?} is not a method", impl_m), |
There was a problem hiding this comment.
Is it still a bug if this isn't a method? If so, we should add the match back in. The match was taken out because technically all impl items have generics now, not just methods.
There was a problem hiding this comment.
I don't think it has to be a bug -- this code is currently only used for methods, I believe, but I expect we'll be using it or something similar for all trait/impl items before we are done.
| @@ -257,19 +257,9 @@ fn type_param_predicates<'a, 'tcx>(tcx: TyCtxt<'a, 'tcx, 'tcx>, | |||
|
|
|||
| let item_node_id = tcx.hir.as_local_node_id(item_def_id).unwrap(); | |||
| let ast_generics = match tcx.hir.get(item_node_id) { | |||
| NodeTraitItem(item) => { | |||
There was a problem hiding this comment.
Note the complete replacement of these match statements. All TraitItems and ImplItems contain generics now.
There was a problem hiding this comment.
Yep. I am happy about how the code is getting more uniform.
|
This looks good. I'll do a sweep of the visitor impls to look for complications, but since the Travis checks succeed, it suggests that everything should be ok. |
|
This looks great, I think however that to help our poor ol' bors, you should just push these commits into #44766 and close this PR. |
|
@nikomatsakis Done! |
This is a continuation of the work from #44766. Once that PR gets merged, the commits and changes from that PR should disappear from this one. GitHub actually allows you to filter commits when you are viewing the files changed. There is a dropdown in the top left corner under the tabs when you click on Files Changed. Select the commits starting at ad16f9c and going to 037aa16.
This is the next step in the implementation of #44265 (comment)
Note: I did a search for
sig.genericsin the codebase and I anticipate that this will break some clippy and rustfmt code.