replace binding and shadowed_glob on NameResolution with non_glob_binding and glob_binding

Author: b-naber

compiler/rustc_resolve/src/check_unused.rs

@@ -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
                 {

compiler/rustc_resolve/src/diagnostics.rs

@@ -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
@@ -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) =

compiler/rustc_resolve/src/ident.rs

@@ -905,15 +905,14 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         let break_result = self.visit_module_scopes(ns, |this, scope| match scope {
             ModuleScope::NonGlobal => {
                 // Non-glob bindings are "strong" and can be resolved immediately.
-                if let Some(binding) = resolution.binding
-                    && !binding.is_glob_import()
+                if let Some(binding) = resolution.non_glob_binding
                     && ignore_binding.map_or(true, |b| binding != b)
                 {
                     if let Some(finalize) = finalize {
-                        return Some(this.finalize_module_binding(
+                        return Some(this.finalize_non_glob_module_binding(
                             ident,
-                            Some(binding),
-                            resolution.shadowed_glob,
+                            binding,
+                            resolution.glob_binding,
                             parent_scope,
                             finalize,
                             shadowing,
@@ -928,15 +927,18 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             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]
-                    .into_iter()
-                    .find_map(|binding| if binding == ignore_binding { None } else { binding });
+                let binding = resolution.glob_binding;
+
+                if let Some(binding) = binding {
+                    if !binding.is_glob_import() {
+                        panic!("binding should be glob import {:?}", binding)
+                    }
+                }
 
                 if let Some(finalize) = finalize {
-                    return Some(this.finalize_module_binding(
+                    return Some(this.finalize_glob_module_binding(
                         ident,
                         binding,
-                        resolution.shadowed_glob,
                         parent_scope,
                         finalize,
                         shadowing,
@@ -1040,21 +1042,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         Err((Determined, Weak::No))
     }
 
-    fn finalize_module_binding(
+    fn finalize_non_glob_module_binding(
         &mut self,
         ident: Ident,
-        binding: Option<NameBinding<'ra>>,
-        shadowed_glob: Option<NameBinding<'ra>>,
+        binding: NameBinding<'ra>,
+        glob_binding: Option<NameBinding<'ra>>,
         parent_scope: &ParentScope<'ra>,
         finalize: Finalize,
         shadowing: Shadowing,
     ) -> Result<NameBinding<'ra>, (Determinacy, Weak)> {
         let Finalize { path_span, report_private, used, root_span, .. } = finalize;
 
-        let Some(binding) = binding else {
-            return Err((Determined, Weak::No));
-        };
-
         if !self.is_accessible_from(binding.vis, parent_scope.module) {
             if report_private {
                 self.privacy_errors.push(PrivacyError {
@@ -1071,7 +1069,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         }
 
         // Forbid expanded shadowing to avoid time travel.
-        if let Some(shadowed_glob) = shadowed_glob
+        if let Some(shadowed_glob) = glob_binding
             && shadowing == Shadowing::Restricted
             && binding.expansion != LocalExpnId::ROOT
             && binding.res() != shadowed_glob.res()
@@ -1099,6 +1097,47 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         return Ok(binding);
     }
 
+    fn finalize_glob_module_binding(
+        &mut self,
+        ident: Ident,
+        binding: Option<NameBinding<'ra>>,
+        parent_scope: &ParentScope<'ra>,
+        finalize: Finalize,
+        shadowing: Shadowing,
+    ) -> Result<NameBinding<'ra>, (Determinacy, Weak)> {
+        let Finalize { path_span, report_private, used, root_span, .. } = finalize;
+
+        let Some(binding) = binding else {
+            return Err((Determined, Weak::No));
+        };
+
+        if !self.is_accessible_from(binding.vis, parent_scope.module) {
+            if report_private {
+                self.privacy_errors.push(PrivacyError {
+                    ident,
+                    binding,
+                    dedup_span: path_span,
+                    outermost_res: None,
+                    parent_scope: *parent_scope,
+                    single_nested: path_span != root_span,
+                });
+            } else {
+                return Err((Determined, Weak::No));
+            }
+        }
+
+        if shadowing == Shadowing::Unrestricted
+            && binding.expansion != LocalExpnId::ROOT
+            && let NameBindingKind::Import { import, .. } = binding.kind
+            && matches!(import.kind, ImportKind::MacroExport)
+        {
+            self.macro_expanded_macro_export_errors.insert((path_span, binding.span));
+        }
+
+        self.record_use(ident, binding, used);
+        return Ok(binding);
+    }
+
     // Checks if a single import can define the `Ident` corresponding to `binding`.
     // This is used to check whether we can definitively accept a glob as a resolution.
     fn single_import_can_define_name(

compiler/rustc_resolve/src/imports.rs

@@ -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::*;
@@ -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)
     }
 }
 
@@ -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(())
@@ -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
@@ -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, .. } =
@@ -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 {

compiler/rustc_resolve/src/late.rs

@@ -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.
@@ -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);
         }