Refactor resolve_ident_in_module to use scopes #142547
Conversation
rustbot
added
S-waiting-on-review
| } | ||
|
|
||
| // --- From now on we either have a glob resolution or no resolution. --- | ||
| let break_result = self.visit_module_scopes(ns, |this, scope| match scope { |
There was a problem hiding this comment.
I wonder if it'd be worth making a ModuleScopeVisitor trait or something like that? It seems a little weird to me to loop through all the scope types and then match on which scope we're in. I feel like there might be a nicer way to factor that.
(Just a suggestion to consider, you don't need to change this just because I said so.)
There was a problem hiding this comment.
The current setup is copied from the larger visit_scopes, in that case a match is certainly more convenient.
|
I gave this a quick read and it seems cleaner than before! |
rust-cloud-vms
bot
force-pushed
the
refactor-resolve-ident-module
branch
from
f6807eb to
f9a6e87
Compare
| #[derive(Debug, Copy, Clone, PartialEq)] | ||
| enum ModuleScope { | ||
| NonGlobal, | ||
| Global, |
There was a problem hiding this comment.
| Global, | |
| Globs, |
"Glob imports", not "global imports".
| @@ -221,6 +223,27 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { | |||
| None | |||
| } | |||
|
|
|||
| #[instrument(skip(self, visitor), level = "debug")] | |||
| pub(crate) fn visit_module_scopes<T>( | |||
There was a problem hiding this comment.
| pub(crate) fn visit_module_scopes<T>( | |
| fn visit_module_scopes<T>( |
| // However, it causes resolution/expansion to stuck too often (#53144), so, to make | ||
| // progress, we have to ignore those potential unresolved invocations from other modules | ||
| // and prohibit access to macro-expanded `macro_export` macros instead (unless restricted | ||
| // shadowing is enabled, see `macro_expanded_macro_export_errors`). |
There was a problem hiding this comment.
Could you make sure none of the comments is lost, like in this case?
|
|
||
| let check_usable = |this: &mut Self, binding: NameBinding<'ra>| { | ||
| let usable = this.is_accessible_from(binding.vis, parent_scope.module); | ||
| debug!(?usable); |
There was a problem hiding this comment.
Could you remove the added debug statements some time before merge?
| } | ||
|
|
||
| // --- From now on we either have a glob resolution or no resolution. --- | ||
| let break_result = self.visit_module_scopes(ns, |this, scope| match scope { |
There was a problem hiding this comment.
The current setup is copied from the larger visit_scopes, in that case a match is certainly more convenient.
| ModuleScope::Global => { | ||
| // If we are here, any primary `resolution.binding` is either a glob, None, | ||
| // or should be ignored. | ||
| let binding = [resolution.binding, resolution.shadowed_glob] |
There was a problem hiding this comment.
This is a sign showing that the conversion to scopes is not fully done.
Ideally resolution should contain resolution.non_glob_binding and resolution.glob_binding instead (or there should be two resolution maps for non-globs and globs), but that's a larger refactoring.
| } | ||
| } | ||
|
|
||
| // Forbid expanded shadowing to avoid time travel. |
There was a problem hiding this comment.
This also indicates that the conversion to scopes wasn't done properly.
Ideally MoreExpandedVsOuter should be reported here in a similar way to how it's done in early_resolve_ident_in_lexical_scope.
| @@ -130,6 +130,13 @@ enum Scope<'ra> { | |||
| BuiltinTypes, | |||
| } | |||
|
|
|||
| /// Scopes used for resolving an `Ident` in a `Module`. | |||
| #[derive(Debug, Copy, Clone, PartialEq)] | |||
| enum ModuleScope { | |||
There was a problem hiding this comment.
Instead of this, I think enum Scope above should also get two variants ModuleNonGlobs and ModuleGlobs instead of one Module.
Then finding any resolution inside a module would be done using a new ScopeSet variant including just two scopes, but other ScopeSet would just iterate over ModuleNonGlobs and ModuleGlobs as a part of the common loop.
This would also be a much larger refactoring.
| } | ||
| }); | ||
|
|
||
| match break_result { |
There was a problem hiding this comment.
if let Some(result) = break_result {
return result;
}
|
I think this is a small step in the right direction, but it probably doesn't move us much closer to implementing the open namespace proposal, splitting modules into scopes properly is a large refactoring that only partially intersects with open namespaces. Also not sure if landing just this part of the refactoring is beneficial. |
petrochenkov
added
S-waiting-on-author
Refactoring that should help with introducing name resolution for namespaced crates.
r? @petrochenkov