resolve: Report more early resolution ambiguities for imports

The new ambiguities are reported when the import's visibility is ambiguous and may depend on the resolution/expansion order.

Author: petrochenkov

compiler/rustc_lint_defs/src/builtin.rs

@@ -21,6 +21,7 @@ declare_lint_pass! {
         AMBIGUOUS_ASSOCIATED_ITEMS,
         AMBIGUOUS_GLOB_IMPORTS,
         AMBIGUOUS_GLOB_REEXPORTS,
+        AMBIGUOUS_IMPORT_VISIBILITIES,
         ARITHMETIC_OVERFLOW,
         ASM_SUB_REGISTER,
         BAD_ASM_STYLE,
@@ -4488,6 +4489,55 @@ declare_lint! {
     };
 }
 
+declare_lint! {
+    /// The `ambiguous_import_visibilities` lint detects imports that should report ambiguity
+    /// errors, but previously didn't do that due to rustc bugs.
+    ///
+    /// ### Example
+    ///
+    /// ```rust,compile_fail
+    /// #![deny(ambiguous_import_visibilities)]
+    /// mod reexport {
+    ///     mod m {
+    ///         pub struct S {}
+    ///     }
+    ///
+    ///     macro_rules! mac {
+    ///         () => { use m::S; }
+    ///     }
+    ///
+    ///     pub use m::*;
+    ///     mac!();
+    ///
+    ///     pub use S as Z;
+    /// }
+    ///
+    /// fn main() {
+    ///     reexport::Z {}; // ok, no privacy error
+    /// }
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// Previous versions of Rust compile it successfully because it
+    /// fetched the glob import's visibility for `pub use S as Z` import, and ignored the private
+    /// `use m::S` import that appeared later.
+    ///
+    /// This is a [future-incompatible] lint to transition this to a
+    /// hard error in the future.
+    ///
+    /// [future-incompatible]: ../index.md#future-incompatible-lints
+    pub AMBIGUOUS_IMPORT_VISIBILITIES,
+    Deny,
+    "detects certain glob imports that require reporting an ambiguity error",
+    @future_incompatible = FutureIncompatibleInfo {
+        reason: FutureIncompatibilityReason::FutureReleaseError,
+        reference: "issue #149145 <https://github.com/rust-lang/rust/issues/149145>",
+    };
+}
+
 declare_lint! {
     /// The `refining_impl_trait_reachable` lint detects `impl Trait` return
     /// types in method signatures that are refined by a publically reachable

compiler/rustc_middle/src/ty/mod.rs

@@ -380,6 +380,12 @@ impl<Id: Into<DefId>> Visibility<Id> {
     }
 }
 
+impl<Id: Into<DefId> + Copy> Visibility<Id> {
+    pub fn min(self, vis: Visibility<Id>, tcx: TyCtxt<'_>) -> Visibility<Id> {
+        if self.is_at_least(vis, tcx) { vis } else { self }
+    }
+}
+
 impl Visibility<DefId> {
     pub fn expect_local(self) -> Visibility {
         self.map_id(|id| id.expect_local())

compiler/rustc_resolve/src/diagnostics.rs

@@ -24,7 +24,7 @@ use rustc_middle::ty::TyCtxt;
 use rustc_session::Session;
 use rustc_session::lint::BuiltinLintDiag;
 use rustc_session::lint::builtin::{
-    ABSOLUTE_PATHS_NOT_STARTING_WITH_CRATE, AMBIGUOUS_GLOB_IMPORTS,
+    ABSOLUTE_PATHS_NOT_STARTING_WITH_CRATE, AMBIGUOUS_GLOB_IMPORTS, AMBIGUOUS_IMPORT_VISIBILITIES,
     MACRO_EXPANDED_MACRO_EXPORTS_ACCESSED_BY_ABSOLUTE_PATHS,
 };
 use rustc_session::utils::was_invoked_from_cargo;
@@ -148,12 +148,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             let diag = self.ambiguity_diagnostic(ambiguity_error);
 
             if ambiguity_error.warning {
-                let NameBindingKind::Import { import, .. } = ambiguity_error.b1.0.kind else {
-                    unreachable!()
+                let node_id = match ambiguity_error.b1.0.kind {
+                    NameBindingKind::Import { import, .. } => import.root_id,
+                    NameBindingKind::Res(_) => CRATE_NODE_ID,
                 };
                 self.lint_buffer.buffer_lint(
-                    AMBIGUOUS_GLOB_IMPORTS,
-                    import.root_id,
+                    if ambiguity_error.ambig_vis.is_some() {
+                        AMBIGUOUS_IMPORT_VISIBILITIES
+                    } else {
+                        AMBIGUOUS_GLOB_IMPORTS
+                    },
+                    node_id,
                     diag.ident.span,
                     diag,
                 );
@@ -1995,7 +2000,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
     }
 
     fn ambiguity_diagnostic(&self, ambiguity_error: &AmbiguityError<'ra>) -> errors::Ambiguity {
-        let AmbiguityError { kind, ident, b1, b2, misc1, misc2, .. } = *ambiguity_error;
+        let AmbiguityError { kind, ambig_vis, ident, b1, b2, misc1, misc2, .. } = *ambiguity_error;
         let extern_prelude_ambiguity = || {
             self.extern_prelude.get(&Macros20NormalizedIdent::new(ident)).is_some_and(|entry| {
                 entry.item_binding.map(|(b, _)| b) == Some(b1)
@@ -2051,8 +2056,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         let (b1_note, b1_help_msgs) = could_refer_to(b1, misc1, "");
         let (b2_note, b2_help_msgs) = could_refer_to(b2, misc2, " also");
 
+        let ambig_vis = ambig_vis.map(|(vis1, vis2)| {
+            format!(
+                "{} or {}",
+                vis1.to_string(CRATE_DEF_ID, self.tcx),
+                vis2.to_string(CRATE_DEF_ID, self.tcx)
+            )
+        });
+
         errors::Ambiguity {
             ident,
+            ambig_vis,
             kind: kind.descr(),
             b1_note,
             b1_help_msgs,

compiler/rustc_resolve/src/errors.rs

@@ -1450,6 +1450,7 @@ pub(crate) struct UnknownDiagnosticAttributeTypoSugg {
 // FIXME: Make this properly translatable.
 pub(crate) struct Ambiguity {
     pub ident: Ident,
+    pub ambig_vis: Option<String>,
     pub kind: &'static str,
     pub b1_note: Spanned<String>,
     pub b1_help_msgs: Vec<String>,
@@ -1459,8 +1460,12 @@ pub(crate) struct Ambiguity {
 
 impl Ambiguity {
     fn decorate<'a>(self, diag: &mut Diag<'a, impl EmissionGuarantee>) {
-        diag.primary_message(format!("`{}` is ambiguous", self.ident));
-        diag.span_label(self.ident.span, "ambiguous name");
+        if let Some(ambig_vis) = self.ambig_vis {
+            diag.primary_message(format!("ambiguous import visibility: {ambig_vis}"));
+        } else {
+            diag.primary_message(format!("`{}` is ambiguous", self.ident));
+            diag.span_label(self.ident.span, "ambiguous name");
+        }
         diag.note(format!("ambiguous because of {}", self.kind));
         diag.span_note(self.b1_note.span, self.b1_note.node);
         for help_msg in self.b1_help_msgs {

compiler/rustc_resolve/src/ident.rs

@@ -5,6 +5,7 @@ use Namespace::*;
 use rustc_ast::{self as ast, NodeId};
 use rustc_errors::ErrorGuaranteed;
 use rustc_hir::def::{DefKind, MacroKinds, Namespace, NonMacroAttrKind, PartialRes, PerNS};
+use rustc_middle::ty::Visibility;
 use rustc_middle::{bug, span_bug};
 use rustc_session::lint::builtin::PROC_MACRO_DERIVE_RESOLUTION_FALLBACK;
 use rustc_session::parse::feature_err;
@@ -469,9 +470,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                         // We do not need to report them if we are either in speculative resolution,
                         // or in late resolution when everything is already imported and expanded
                         // and no ambiguities exist.
-                        if matches!(finalize, None | Some(Finalize { stage: Stage::Late, .. })) {
-                            return ControlFlow::Break(Ok(binding));
-                        }
+                        let import_vis = match finalize {
+                            None | Some(Finalize { stage: Stage::Late, .. }) => {
+                                return ControlFlow::Break(Ok(binding));
+                            }
+                            Some(Finalize { import_vis, .. }) => import_vis,
+                        };
 
                         if let Some((innermost_binding, innermost_flags)) = innermost_result {
                             // Found another solution, if the first one was "weak", report an error.
@@ -482,6 +486,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                                 innermost_binding,
                                 flags,
                                 innermost_flags,
+                                import_vis,
                                 extern_prelude_item_binding,
                                 extern_prelude_flag_binding,
                             ) {
@@ -730,11 +735,13 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         innermost_binding: NameBinding<'ra>,
         flags: Flags,
         innermost_flags: Flags,
+        import_vis: Option<Visibility>,
         extern_prelude_item_binding: Option<NameBinding<'ra>>,
         extern_prelude_flag_binding: Option<NameBinding<'ra>>,
     ) -> bool {
         let (res, innermost_res) = (binding.res(), innermost_binding.res());
-        if res == innermost_res {
+        let ambig_vis = self.ambig_vis(import_vis, binding, innermost_binding);
+        if res == innermost_res && ambig_vis.is_none() {
             return false;
         }
 
@@ -793,10 +800,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             };
             self.ambiguity_errors.push(AmbiguityError {
                 kind,
+                ambig_vis,
                 ident: orig_ident,
                 b1: innermost_binding,
                 b2: binding,
-                warning: false,
+                warning: ambig_vis.is_some(),
                 misc1: misc(innermost_flags),
                 misc2: misc(flags),
             });
@@ -1207,17 +1215,20 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             && shadowing == Shadowing::Restricted
             && finalize.stage == Stage::Early
             && binding.expansion != LocalExpnId::ROOT
-            && binding.res() != shadowed_glob.res()
         {
-            self.ambiguity_errors.push(AmbiguityError {
-                kind: AmbiguityKind::GlobVsExpanded,
-                ident,
-                b1: binding,
-                b2: shadowed_glob,
-                warning: false,
-                misc1: AmbiguityErrorMisc::None,
-                misc2: AmbiguityErrorMisc::None,
-            });
+            let ambig_vis = self.ambig_vis(finalize.import_vis, shadowed_glob, binding);
+            if shadowed_glob.res() != binding.res() || ambig_vis.is_some() {
+                self.ambiguity_errors.push(AmbiguityError {
+                    kind: AmbiguityKind::GlobVsExpanded,
+                    ambig_vis,
+                    ident,
+                    b1: binding,
+                    b2: shadowed_glob,
+                    warning: ambig_vis.is_some(),
+                    misc1: AmbiguityErrorMisc::None,
+                    misc2: AmbiguityErrorMisc::None,
+                });
+            }
         }
 
         if shadowing == Shadowing::Unrestricted
@@ -1324,6 +1335,23 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         false
     }
 
+    fn ambig_vis(
+        &self,
+        import_vis: Option<Visibility>,
+        binding: NameBinding<'ra>,
+        innermost_binding: NameBinding<'ra>,
+    ) -> Option<(Visibility, Visibility)> {
+        match import_vis {
+            Some(import_vis) if binding.res() == innermost_binding.res() => {
+                let min =
+                    |b: NameBinding<'_>| b.vis.min(import_vis.to_def_id(), self.tcx).expect_local();
+                let (min1, min2) = (min(binding), min(innermost_binding));
+                (min1 != min2).then_some((min1, min2))
+            }
+            _ => None,
+        }
+    }
+
     /// Validate a local resolution (from ribs).
     #[instrument(level = "debug", skip(self, all_ribs))]
     fn validate_res_from_ribs(

compiler/rustc_resolve/src/imports.rs

@@ -1136,7 +1136,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                     ident,
                     ns,
                     &import.parent_scope,
-                    Some(Finalize { report_private: false, ..finalize }),
+                    Some(Finalize {
+                        report_private: false,
+                        import_vis: Some(import.vis),
+                        ..finalize
+                    }),
                     bindings[ns].get().binding(),
                     Some(import),
                 );

compiler/rustc_resolve/src/lib.rs

@@ -904,6 +904,7 @@ enum AmbiguityErrorMisc {
 
 struct AmbiguityError<'ra> {
     kind: AmbiguityKind,
+    ambig_vis: Option<(Visibility, Visibility)>,
     ident: Ident,
     b1: NameBinding<'ra>,
     b2: NameBinding<'ra>,
@@ -2032,6 +2033,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         if let Some((b2, kind)) = used_binding.ambiguity {
             let ambiguity_error = AmbiguityError {
                 kind,
+                ambig_vis: None,
                 ident,
                 b1: used_binding,
                 b2,
@@ -2502,6 +2504,8 @@ struct Finalize {
     used: Used = Used::Other,
     /// Finalizing early or late resolution.
     stage: Stage = Stage::Early,
+    /// Nominal visibility of the import item, in case we are resolving and import's final segment.
+    import_vis: Option<Visibility> = None,
 }
 
 impl Finalize {

tests/ui/imports/ambiguous-import-visibility-macro.rs

@@ -0,0 +1,18 @@
+//@ edition:2018
+//@ proc-macro: same-res-ambigious-extern-macro.rs
+
+macro_rules! globbing{
+    () => {
+        pub use same_res_ambigious_extern_macro::*;
+    }
+}
+
+#[macro_use] // this imports the `RustEmbed` macro with `pub(crate)` visibility
+extern crate same_res_ambigious_extern_macro;
+globbing! {} // this imports the same `RustEmbed` macro with `pub` visibility
+
+pub trait RustEmbed {}
+
+pub use RustEmbed as Embed; //~ ERROR ambiguous import visibility
+                            //~| WARN this was previously accepted
+fn main() {}

tests/ui/imports/ambiguous-import-visibility-macro.stderr

@@ -0,0 +1,29 @@
+error: ambiguous import visibility: pub(crate) or pub
+  --> $DIR/ambiguous-import-visibility-macro.rs:16:9
+   |
+LL | pub use RustEmbed as Embed;
+   |         ^^^^^^^^^
+   |
+   = warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
+   = note: for more information, see issue #149145 <https://github.com/rust-lang/rust/issues/149145>
+   = note: ambiguous because of a conflict between a name from a glob import and an outer scope during import or macro resolution
+note: `RustEmbed` could refer to the derive macro imported here
+  --> $DIR/ambiguous-import-visibility-macro.rs:6:17
+   |
+LL |         pub use same_res_ambigious_extern_macro::*;
+   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+...
+LL | globbing! {} // this imports the same `RustEmbed` macro with `pub` visibility
+   | ------------ in this macro invocation
+   = help: consider adding an explicit import of `RustEmbed` to disambiguate
+   = help: or use `crate::RustEmbed` to refer to this derive macro unambiguously
+note: `RustEmbed` could also refer to the derive macro imported here
+  --> $DIR/ambiguous-import-visibility-macro.rs:10:1
+   |
+LL | #[macro_use] // this imports the `RustEmbed` macro with `pub(crate)` visibility
+   | ^^^^^^^^^^^^
+   = note: `#[deny(ambiguous_import_visibilities)]` (part of `#[deny(future_incompatible)]`) on by default
+   = note: this error originates in the macro `globbing` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: aborting due to 1 previous error
+

tests/ui/imports/ambiguous-import-visibility-module.rs

@@ -0,0 +1,23 @@
+//@ edition:2018..
+
+mod reexport {
+    mod m {
+        pub struct S {}
+    }
+
+    macro_rules! mac {
+        () => {
+            use m::S;
+        };
+    }
+
+    pub use m::*;
+    mac!();
+
+    pub use S as Z; //~ ERROR ambiguous import visibility
+                    //~| WARN this was previously accepted
+}
+
+fn main() {
+    reexport::Z {};
+}