Skip to content

Refactor NameResolution to use non_glob_binding and glob_binding #142547

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed

Refactor NameResolution to use non_glob_binding and glob_binding #142547

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a1d845b
refactor resolve_ident_in_module to use scopes
b-naber Jun 15, 2025
e9ed7ed
replace binding and shadowed_glob on NameResolution with non_glob_bin…
b-naber Jun 21, 2025
f836ab1
address review
b-naber Jun 23, 2025
c0f41d4
bless tests
b-naber Jun 23, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion compiler/rustc_resolve/src/check_unused.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ impl Resolver<'_, '_> {
for (_key, resolution) in self.resolutions(*module).borrow().iter() {
let resolution = resolution.borrow();

if let Some(binding) = resolution.binding
if let Some(binding) = resolution.late_binding()
&& let NameBindingKind::Import { import, .. } = binding.kind
&& let ImportKind::Single { id, .. } = import.kind
{
Expand Down
4 changes: 2 additions & 2 deletions compiler/rustc_resolve/src/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1442,7 +1442,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
|(key, name_resolution)| {
if key.ns == TypeNS
&& key.ident == ident
&& let Some(binding) = name_resolution.borrow().binding
&& let Some(binding) = name_resolution.borrow().late_binding()
{
match binding.res() {
// No disambiguation needed if the identically named item we
Expand Down Expand Up @@ -1496,7 +1496,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
return None;
};
for (_, resolution) in this.resolutions(m).borrow().iter() {
let Some(binding) = resolution.borrow().binding else {
let Some(binding) = resolution.borrow().late_binding() else {
continue;
};
let Res::Def(DefKind::Macro(MacroKind::Derive | MacroKind::Attr), def_id) =
Expand Down
414 changes: 261 additions & 153 deletions compiler/rustc_resolve/src/ident.rs
View file
Open in desktop

Large diffs are not rendered by default.

151 changes: 77 additions & 74 deletions compiler/rustc_resolve/src/imports.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ use rustc_span::edit_distance::find_best_match_for_name;
use rustc_span::hygiene::LocalExpnId;
use rustc_span::{Ident, Span, Symbol, kw, sym};
use smallvec::SmallVec;
use tracing::debug;
use tracing::{debug, instrument};

use crate::Determinacy::{self, *};
use crate::Namespace::*;
Expand Down Expand Up @@ -235,21 +235,20 @@ pub(crate) struct NameResolution<'ra> {
/// 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: FxIndexSet<Import<'ra>>,
/// The least shadowable known binding for this name, or None if there are no known bindings.
pub binding: Option<NameBinding<'ra>>,
pub shadowed_glob: Option<NameBinding<'ra>>,
/// The least shadowable known non-glob binding for this name, or None if there are no known bindings.
pub non_glob_binding: Option<NameBinding<'ra>>,
pub glob_binding: Option<NameBinding<'ra>>,
}

impl<'ra> NameResolution<'ra> {
/// Returns the binding for the name if it is known or None if it not known.
pub(crate) fn binding(&self) -> Option<NameBinding<'ra>> {
self.binding.and_then(|binding| {
if !binding.is_glob_import() || self.single_imports.is_empty() {
Some(binding)
} else {
None
}
})
self.non_glob_binding
.or_else(|| if self.single_imports.is_empty() { self.glob_binding } else { None })
}

pub(crate) fn late_binding(&self) -> Option<NameBinding<'ra>> {
self.non_glob_binding.or_else(|| self.glob_binding)
}
}

Expand Down Expand Up @@ -331,77 +330,79 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
self.check_reserved_macro_name(key.ident, res);
self.set_binding_parent_module(binding, module);
self.update_resolution(module, key, warn_ambiguity, |this, resolution| {
if let Some(old_binding) = resolution.binding {
if res == Res::Err && old_binding.res() != Res::Err {
if let Some(old_non_glob_binding) = resolution.non_glob_binding {
if res == Res::Err && old_non_glob_binding.res() != Res::Err {
// Do not override real bindings with `Res::Err`s from error recovery.
return Ok(());
}
match (old_binding.is_glob_import(), binding.is_glob_import()) {
(true, true) => {
// FIXME: remove `!binding.is_ambiguity_recursive()` after delete the warning ambiguity.
if !binding.is_ambiguity_recursive()
&& let NameBindingKind::Import { import: old_import, .. } =
old_binding.kind
&& let NameBindingKind::Import { import, .. } = binding.kind
&& old_import == import
{
// We should replace the `old_binding` with `binding` regardless
// of whether they has same resolution or not when they are
// imported from the same glob-import statement.
resolution.binding = Some(binding);
} else if res != old_binding.res() {
resolution.binding = Some(this.new_ambiguity_binding(
AmbiguityKind::GlobVsGlob,
old_binding,
binding,
warn_ambiguity,
));
} else if !old_binding.vis.is_at_least(binding.vis, this.tcx) {
// We are glob-importing the same item but with greater visibility.
resolution.binding = Some(binding);
} else if binding.is_ambiguity_recursive() {
resolution.binding = Some(this.new_warn_ambiguity_binding(binding));
}

if binding.is_glob_import() {
let (glob_binding, nonglob_binding) = (binding, old_non_glob_binding);

if key.ns == MacroNS
&& nonglob_binding.expansion != LocalExpnId::ROOT
&& glob_binding.res() != nonglob_binding.res()
{
resolution.non_glob_binding = Some(this.new_ambiguity_binding(
AmbiguityKind::GlobVsExpanded,
nonglob_binding,
glob_binding,
false,
));
}
(old_glob @ true, false) | (old_glob @ false, true) => {
let (glob_binding, nonglob_binding) =
if old_glob { (old_binding, binding) } else { (binding, old_binding) };
if key.ns == MacroNS
&& nonglob_binding.expansion != LocalExpnId::ROOT
&& glob_binding.res() != nonglob_binding.res()
{
resolution.binding = Some(this.new_ambiguity_binding(
AmbiguityKind::GlobVsExpanded,
nonglob_binding,

if let Some(old_glob) = resolution.glob_binding {
if glob_binding.res() != old_glob.res() {
resolution.glob_binding = Some(this.new_ambiguity_binding(
AmbiguityKind::GlobVsGlob,
old_glob,
glob_binding,
false,
warn_ambiguity,
));
} else {
resolution.binding = Some(nonglob_binding);
}

if let Some(old_shadowed_glob) = resolution.shadowed_glob {
assert!(old_shadowed_glob.is_glob_import());
if glob_binding.res() != old_shadowed_glob.res() {
resolution.shadowed_glob = Some(this.new_ambiguity_binding(
AmbiguityKind::GlobVsGlob,
old_shadowed_glob,
glob_binding,
false,
));
} else if !old_shadowed_glob.vis.is_at_least(binding.vis, this.tcx) {
resolution.shadowed_glob = Some(glob_binding);
}
} else {
resolution.shadowed_glob = Some(glob_binding);
} else if !old_glob.vis.is_at_least(glob_binding.vis, this.tcx) {
resolution.glob_binding = Some(glob_binding);
}
} else {
resolution.glob_binding = Some(glob_binding);
}
(false, false) => {
return Err(old_binding);
} else {
return Err(old_non_glob_binding);
}
} else if let Some(old_glob_binding) = resolution.glob_binding {
if binding.is_glob_import() {
// FIXME: remove `!binding.is_ambiguity_recursive()` after delete the warning ambiguity.
if !binding.is_ambiguity_recursive()
&& let NameBindingKind::Import { import: old_import, .. } =
old_glob_binding.kind
&& let NameBindingKind::Import { import, .. } = binding.kind
&& old_import == import
{
// We should replace the `old_binding` with `binding` regardless
// of whether they has same resolution or not when they are
// imported from the same glob-import statement.
resolution.glob_binding = Some(binding);
} else if res != old_glob_binding.res() {
resolution.glob_binding = Some(this.new_ambiguity_binding(
AmbiguityKind::GlobVsGlob,
old_glob_binding,
binding,
warn_ambiguity,
));
} else if !old_glob_binding.vis.is_at_least(binding.vis, this.tcx) {
// We are glob-importing the same item but with greater visibility.
resolution.glob_binding = Some(binding);
} else if binding.is_ambiguity_recursive() {
resolution.glob_binding = Some(this.new_warn_ambiguity_binding(binding));
}
} else {
resolution.non_glob_binding = Some(binding);
}
} else {
resolution.binding = Some(binding);
if binding.is_glob_import() {
resolution.glob_binding = Some(binding);
} else {
resolution.non_glob_binding = Some(binding);
}
}

Ok(())
Expand Down Expand Up @@ -611,15 +612,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
self.throw_unresolved_import_error(errors, glob_error);
}

#[instrument(skip(self), level = "debug")]
pub(crate) fn check_hidden_glob_reexports(
&mut self,
exported_ambiguities: FxHashSet<NameBinding<'ra>>,
) {
for module in self.arenas.local_modules().iter() {
for (key, resolution) in self.resolutions(*module).borrow().iter() {
let resolution = resolution.borrow();
debug!(?resolution);

let Some(binding) = resolution.binding else { continue };
let Some(binding) = resolution.late_binding() else { continue };

if let NameBindingKind::Import { import, .. } = binding.kind
&& let Some((amb_binding, _)) = binding.ambiguity
Expand All @@ -639,7 +642,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
);
}

if let Some(glob_binding) = resolution.shadowed_glob {
if let Some(glob_binding) = resolution.glob_binding {
if binding.res() != Res::Err
&& glob_binding.res() != Res::Err
&& let NameBindingKind::Import { import: glob_import, .. } =
Expand Down Expand Up @@ -1162,7 +1165,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
return None;
} // Never suggest the same name
match *resolution.borrow() {
NameResolution { binding: Some(name_binding), .. } => {
NameResolution { non_glob_binding: Some(name_binding), .. } => {
match name_binding.kind {
NameBindingKind::Import { binding, .. } => {
match binding.kind {
Expand Down
6 changes: 4 additions & 2 deletions compiler/rustc_resolve/src/late.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3441,7 +3441,8 @@ impl<'a, 'ast, 'ra, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
};
ident.span.normalize_to_macros_2_0_and_adjust(module.expansion);
let key = BindingKey::new(ident, ns);
let mut binding = self.r.resolution(module, key).try_borrow().ok().and_then(|r| r.binding);
let mut binding =
self.r.resolution(module, key).try_borrow().ok().and_then(|r| r.late_binding());
debug!(?binding);
if binding.is_none() {
// We could not find the trait item in the correct namespace.
Expand All @@ -3452,7 +3453,8 @@ impl<'a, 'ast, 'ra, 'tcx> LateResolutionVisitor<'a, 'ast, 'ra, 'tcx> {
_ => ns,
};
let key = BindingKey::new(ident, ns);
binding = self.r.resolution(module, key).try_borrow().ok().and_then(|r| r.binding);
binding =
self.r.resolution(module, key).try_borrow().ok().and_then(|r| r.late_binding());
debug!(?binding);
}

Expand Down
29 changes: 17 additions & 12 deletions compiler/rustc_resolve/src/late/diagnostics.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -881,8 +881,10 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> {
fn lookup_doc_alias_name(&mut self, path: &[Segment], ns: Namespace) -> Option<(DefId, Ident)> {
let find_doc_alias_name = |r: &mut Resolver<'ra, '_>, m: Module<'ra>, item_name: Symbol| {
for resolution in r.resolutions(m).borrow().values() {
let Some(did) =
resolution.borrow().binding.and_then(|binding| binding.res().opt_def_id())
let Some(did) = resolution
.borrow()
.late_binding()
.and_then(|binding| binding.res().opt_def_id())
else {
continue;
};
Expand Down Expand Up @@ -1457,15 +1459,16 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> {
self.resolve_path(mod_path, None, None)
{
let resolutions = self.r.resolutions(module).borrow();
let targets: Vec<_> =
resolutions
.iter()
.filter_map(|(key, resolution)| {
resolution.borrow().binding.map(|binding| binding.res()).and_then(
|res| if filter_fn(res) { Some((key, res)) } else { None },
)
})
.collect();
let targets: Vec<_> = resolutions
.iter()
.filter_map(|(key, resolution)| {
resolution
.borrow()
.late_binding()
.map(|binding| binding.res())
.and_then(|res| if filter_fn(res) { Some((key, res)) } else { None })
})
.collect();
if let [target] = targets.as_slice() {
return Some(TypoSuggestion::single_item_from_ident(target.0.ident, target.1));
}
Expand Down Expand Up @@ -2298,7 +2301,9 @@ impl<'ast, 'ra, 'tcx> LateResolutionVisitor<'_, 'ast, 'ra, 'tcx> {
let targets = resolutions
.borrow()
.iter()
.filter_map(|(key, res)| res.borrow().binding.map(|binding| (key, binding.res())))
.filter_map(|(key, res)| {
res.borrow().late_binding().map(|binding| (key, binding.res()))
})
.filter(|(_, res)| match (kind, res) {
(AssocItemKind::Const(..), Res::Def(DefKind::AssocConst, _)) => true,
(AssocItemKind::Fn(_), Res::Def(DefKind::AssocFn, _)) => true,
Expand Down
10 changes: 9 additions & 1 deletion compiler/rustc_resolve/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,13 @@ enum Scope<'ra> {
BuiltinTypes,
}

/// Scopes used for resolving an `Ident` in a `Module`.
#[derive(Debug, Copy, Clone, PartialEq)]
enum ModuleScope {
Copy link
Contributor

@petrochenkov petrochenkov Jun 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@b-naber b-naber Jun 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm while I do think it would be more consistent to have those two scopes in the Scope enum, it would also add more complexity to it. I kind of like that the resolution of an Ident in a module is isolated with the current resolve_ident_in_module, and using a separate ModuleScope for that method simplifies things imo.

Copy link
Contributor

@petrochenkov petrochenkov Jun 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic in early_resolve_ident_in_lexical_scope should see the explicit resolution and the glob resolution in a module as two separate steps.
So either Scope::Module should be split into two scopes, or resolve_ident_in_module_unadjusted should somehow produce the both resolutions so that early_resolve_ident_in_lexical_scope could process them as if they were from two separate scopes.

Copy link
Contributor Author

@b-naber b-naber Jun 23, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm open to try to implement this, but I still don't understand why this is necessary. Can you explain in more detail, please?

Copy link
Contributor

@petrochenkov petrochenkov Jun 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much the same answer as in #142547 (comment).
Sorry, I'll try to produce some more details later.

NonGlobal,
Globs,
}

/// Names from different contexts may want to visit different subsets of all specific scopes
/// with different restrictions when looking up the resolution.
/// This enum is currently used only for early resolution (imports and macros),
Expand Down Expand Up @@ -640,7 +647,8 @@ impl<'ra> Module<'ra> {
F: FnMut(&mut R, Ident, Namespace, NameBinding<'ra>),
{
for (key, name_resolution) in resolver.as_mut().resolutions(self).borrow().iter() {
if let Some(binding) = name_resolution.borrow().binding {
let resolution = name_resolution.borrow();
if let Some(binding) = resolution.non_glob_binding.or_else(|| resolution.glob_binding) {
f(resolver, key.ident, key.ns, binding);
}
}
Expand Down
3 changes: 2 additions & 1 deletion tests/ui/imports/ambiguous-17.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
//@ check-pass
// https://github.com/rust-lang/rust/pull/113099#issuecomment-1638206152

pub use evp::*; //~ WARNING ambiguous glob re-exports
pub use evp::*;
//~^ WARNING ambiguous glob re-exports
pub use handwritten::*;

macro_rules! m {
Expand Down
5 changes: 3 additions & 2 deletions tests/ui/imports/ambiguous-17.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@ warning: ambiguous glob re-exports
|
LL | pub use evp::*;
| ^^^^^^ the name `id` in the value namespace is first re-exported here
LL |
LL | pub use handwritten::*;
| -------------- but the name `id` in the value namespace is also re-exported here
|
= note: `#[warn(ambiguous_glob_reexports)]` on by default

warning: `id` is ambiguous
--> $DIR/ambiguous-17.rs:26:5
--> $DIR/ambiguous-17.rs:27:5
|
LL | id();
| ^^ ambiguous name
Expand All @@ -24,7 +25,7 @@ LL | pub use evp::*;
| ^^^^^^
= help: consider adding an explicit import of `id` to disambiguate
note: `id` could also refer to the function imported here
--> $DIR/ambiguous-17.rs:5:9
--> $DIR/ambiguous-17.rs:6:9
|
LL | pub use handwritten::*;
| ^^^^^^^^^^^^^^
Expand Down
5 changes: 3 additions & 2 deletions tests/ui/imports/ambiguous-4-extern.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@

macro_rules! m {
() => {
pub fn id() {}
pub fn id() {}
};
}

pub use evp::*; //~ WARNING ambiguous glob re-exports
pub use evp::*;
//~^ WARNING ambiguous glob re-exports
pub use handwritten::*;

mod evp {
Expand Down
Loading
Loading