refactor(resolve): splitting glob and non-glob name bindings #112659
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -9,7 +9,7 @@ use crate::{ | |||||
| Resolver, Segment, | ||||||
| }; | ||||||
| use crate::{Finalize, Module, ModuleOrUniformRoot, ParentScope, PerNS, ScopeSet}; | ||||||
| use crate::{NameBinding, NameBindingKind, PathResult, Res}; | ||||||
|
|
||||||
| use rustc_ast::NodeId; | ||||||
| use rustc_data_structures::fx::FxHashSet; | ||||||
|
|
@@ -34,8 +34,6 @@ use smallvec::SmallVec; | |||||
| use std::cell::Cell; | ||||||
| use std::{mem, ptr}; | ||||||
|
|
||||||
| /// Contains data for specific kinds of imports. | ||||||
| #[derive(Clone)] | ||||||
| pub(crate) enum ImportKind<'a> { | ||||||
|
|
@@ -212,25 +210,32 @@ pub(crate) struct NameResolution<'a> { | |||||
| /// Single imports that may define the name in the namespace. | ||||||
| /// Imports are arena-allocated, so it's ok to use pointers as keys. | ||||||
| pub single_imports: FxHashSet<Interned<'a, Import<'a>>>, | ||||||
| non_glob_binding: Option<&'a NameBinding<'a>>, | ||||||
| glob_binding: Option<&'a NameBinding<'a>>, | ||||||
| } | ||||||
|
|
||||||
| impl<'a> NameResolution<'a> { | ||||||
| pub(crate) fn binding(&self) -> Option<&'a NameBinding<'a>> { | ||||||
| self.non_glob_binding() | ||||||
| .or_else(|| if self.single_imports.is_empty() { self.glob_binding() } else { None }) | ||||||
|
There was a problem hiding this comment. This logic is likely incorrect and is a source of issues like #56593 (because it doesn't account for macros), but let's keep it as is in this PR. I'd just want to rename it to something more explanatory, like There was a problem hiding this comment. Yes, I think that issue #56593 is interesting and could be the next task to work on. |
||||||
| } | ||||||
|
|
||||||
| pub(crate) fn non_glob_binding(&self) -> Option<&'a NameBinding<'a>> { | ||||||
| self.non_glob_binding.and_then(|binding| { | ||||||
|
There was a problem hiding this comment.
|
||||||
| assert!(!binding.is_glob_import()); | ||||||
|
There was a problem hiding this comment. I'm not sure these asserts are useful, there's literally one place where these fields are assigned and the glob-ness is checked there. |
||||||
| Some(binding) | ||||||
| }) | ||||||
| } | ||||||
|
|
||||||
| pub(crate) fn glob_binding(&self) -> Option<&'a NameBinding<'a>> { | ||||||
| self.glob_binding.and_then(|binding| { | ||||||
| assert!(binding.is_glob_import()); | ||||||
| Some(binding) | ||||||
| }) | ||||||
| } | ||||||
|
|
||||||
| pub(crate) fn available_binding(&self) -> Option<&'a NameBinding<'a>> { | ||||||
|
There was a problem hiding this comment.
Suggested change
Assuming the other method is renamed to |
||||||
| self.non_glob_binding().or(self.glob_binding()) | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -305,65 +310,41 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { | |||||
| self.check_reserved_macro_name(key.ident, res); | ||||||
| self.set_binding_parent_module(binding, module); | ||||||
| self.update_resolution(module, key, |this, resolution| { | ||||||
| if let Some(old_binding) = resolution.available_binding() && res == Res::Err && old_binding.res() != Res::Err { | ||||||
|
There was a problem hiding this comment. Could you keep the old code structure here when possible? |
||||||
| // Do not override real bindings with `Res::Err`s from error recovery. | ||||||
| Ok(()) | ||||||
| } else if binding.is_glob_import() { | ||||||
| if let Some(old_binding) = resolution.glob_binding() { | ||||||
| if binding.res() != old_binding.res() { | ||||||
| resolution.glob_binding = Some(this.ambiguity( | ||||||
| AmbiguityKind::GlobVsGlob, | ||||||
| old_binding, | ||||||
| binding, | ||||||
| )); | ||||||
| } else if !old_binding.vis.is_at_least(binding.vis, this.tcx) { | ||||||
| resolution.glob_binding = Some(binding); | ||||||
| } | ||||||
| } else { | ||||||
| resolution.glob_binding = Some(binding); | ||||||
| } | ||||||
|
|
||||||
| if let Some(old_binding) = resolution.non_glob_binding() { | ||||||
| if binding.res() != old_binding.res() && key.ns == MacroNS && old_binding.expansion != LocalExpnId::ROOT { | ||||||
| resolution.non_glob_binding = Some(this.ambiguity( | ||||||
| AmbiguityKind::GlobVsExpanded, | ||||||
| old_binding, | ||||||
| binding, | ||||||
| )); | ||||||
| } | ||||||
| } | ||||||
| Ok(()) | ||||||
| } else if let Some(old_binding) = resolution.non_glob_binding() { | ||||||
| Err(old_binding) | ||||||
| } else { | ||||||
| resolution.non_glob_binding = Some(binding); | ||||||
| Ok(()) | ||||||
| } | ||||||
| }) | ||||||
| } | ||||||
|
|
||||||
| fn ambiguity( | ||||||
|
|
@@ -549,11 +530,11 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { | |||||
| for (key, resolution) in self.resolutions(module).borrow().iter() { | ||||||
| let resolution = resolution.borrow(); | ||||||
|
|
||||||
| if let Some(glob_binding) = resolution.glob_binding() { | ||||||
| if let NameBindingKind::Import { import, .. } = glob_binding.kind | ||||||
| && let Some((amb_binding, _)) = glob_binding.ambiguity | ||||||
| && glob_binding.res() != Res::Err | ||||||
| && exported_ambiguities.contains(&Interned::new_unchecked(glob_binding)) | ||||||
| { | ||||||
| self.lint_buffer.buffer_lint_with_diagnostic( | ||||||
| AMBIGUOUS_GLOB_REEXPORTS, | ||||||
|
|
@@ -569,7 +550,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { | |||||
| ); | ||||||
| } | ||||||
|
|
||||||
| if let Some(binding) = resolution.non_glob_binding() { | ||||||
| let binding_id = match binding.kind { | ||||||
| NameBindingKind::Res(res) => { | ||||||
| Some(self.def_id_to_node_id[res.def_id().expect_local()]) | ||||||
|
|
@@ -769,9 +750,8 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { | |||||
| .emit(); | ||||||
| } | ||||||
| let key = BindingKey::new(target, ns); | ||||||
| let mut resolution = this.resolution(parent, key).borrow_mut(); | ||||||
| resolution.single_imports.remove(&Interned::new_unchecked(import)); | ||||||
|
There was a problem hiding this comment.
There was a problem hiding this comment. It is a cleanup, because it seems will never meet other logic in match resolution.binding() {
_ if old_binding.is_some() => return t,
None => return t,
// xxxxxx
}Of course, It should revert it back because it is irrelevant. |
||||||
| } | ||||||
| } | ||||||
| } | ||||||
|
|
@@ -1025,29 +1005,25 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { | |||||
| let resolutions = resolutions.as_ref().into_iter().flat_map(|r| r.iter()); | ||||||
| let names = resolutions | ||||||
| .filter_map(|(BindingKey { ident: i, .. }, resolution)| { | ||||||
| let resolution = resolution.borrow(); | ||||||
| if i.name == ident.name { | ||||||
| None | ||||||
| } else if let Some(name_binding) = resolution.available_binding() { | ||||||
| match name_binding.kind { | ||||||
| NameBindingKind::Import { binding, .. } => { | ||||||
| match binding.kind { | ||||||
| // Never suggest the name that has binding error | ||||||
| // i.e., the name that cannot be previously resolved | ||||||
| NameBindingKind::Res(Res::Err) => None, | ||||||
| _ => Some(i.name), | ||||||
| } | ||||||
| } | ||||||
| _ => Some(i.name), | ||||||
| } | ||||||
| } else if resolution.single_imports.is_empty() { | ||||||
| None | ||||||
| } else { | ||||||
| Some(i.name) | ||||||
| } | ||||||
| }) | ||||||
| .collect::<Vec<Symbol>>(); | ||||||
|
|
||||||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,21 @@ | ||
| // check-pass | ||
|
|
||
| mod a { | ||
| pub trait P {} | ||
| } | ||
| pub use a::*; | ||
|
|
||
| mod b { | ||
| #[derive(Clone)] | ||
| pub enum P { | ||
| A | ||
| } | ||
| } | ||
| pub use b::P; | ||
|
|
||
| mod c { | ||
| use crate::*; | ||
| pub struct _S(Vec<P>); | ||
| } | ||
|
|
||
| fn main() {} | ||
|
There was a problem hiding this comment. Is there some specific issue for this test? |
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: unrelated change, could be moved to a separate cleanup commit.