resolve: Make disambiguators for underscore bindings module-local #144013
Conversation
rustbot
added
S-waiting-on-review
| @@ -531,7 +531,7 @@ impl<'ra, 'tcx> ResolverExpand for Resolver<'ra, 'tcx> { | |||
| target_trait.for_each_child(self, |this, ident, ns, _binding| { | |||
| // FIXME: Adjust hygiene for idents from globs, like for glob imports. | |||
| if let Some(overriding_keys) = this.impl_binding_keys.get(&impl_def_id) | |||
| && overriding_keys.contains(&BindingKey::new(ident.normalize_to_macros_2_0(), ns)) | |||
| && overriding_keys.contains(&BindingKey::new(ident, ns)) | |||
There was a problem hiding this comment.
Drive-by cleanup, the normalization already happens inside BindingKey::new.
| /// `_` in the expanded AST that introduced this binding. | ||
| /// When we add an underscore binding (with ident `_`) to some module, this field has | ||
| /// a non-zero value that uniquely identifies this binding in that module. | ||
| /// For non-undersore bindings this field is zero. |
There was a problem hiding this comment.
| /// For non-undersore bindings this field is zero. | |
| /// For non-underscore bindings this field is zero. |
| let key = BindingKey::new_disambiguated(ident, ns, || { | ||
| (self.r.impl_binding_keys.len() + 1).try_into().unwrap() | ||
| }); |
There was a problem hiding this comment.
Is this needed because _ names for bindings can be handled in the resolver, but will error later? May be worth a comment.
There was a problem hiding this comment.
It's probably unnecessary, because it only affect erroneous cases, but impl_binding_keys is a
/// Simplified analogue of module `resolutions`
, i.e. a set of definitions, so it was a bit weird to use the "lookup" keys BindingKey::new.
Perhaps it makes sense to rename the methods on BindingKey to be more semantic, like new -> lookup and new_disambiguated -> definition.
There was a problem hiding this comment.
oh I agree this is fine and "more correct" to use, just may be surprising to a future reader of the code, so a comment would be useful.
I like the idea of renaming new to lookup, but have no preference for the new_disambiguated method.
petrochenkov
added
S-waiting-on-author
Disambiguators attached to underscore name bindings (like
const _: u8 = something;) do not need to be globally unique, they just need to be unique inside the module in which they live, because the bindings in a module are basically kept asMap<BindingKey, SomeData>.Also, the specific values of the disambiguators are not important, so a glob import of
const _may have a different disambiguator than the originalconst _itself.So in this PR the disambiguator is just set to the current number of bindings in the module.
This removes one more piece of global mutable state from resolver and unblocks #143884.