Auto merge of #149596 - petrochenkov:visambig2, r=yaahc

resolve: Report more visibility-related 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.

Detailed description: #149596 (comment).

Author: bors

compiler/rustc_lint_defs/src/builtin.rs

@@ -20,6 +20,7 @@ declare_lint_pass! {
         AMBIGUOUS_GLOB_IMPORTED_TRAITS,
         AMBIGUOUS_GLOB_IMPORTS,
         AMBIGUOUS_GLOB_REEXPORTS,
+        AMBIGUOUS_IMPORT_VISIBILITIES,
         AMBIGUOUS_PANIC_IMPORTS,
         ARITHMETIC_OVERFLOW,
         ASM_SUB_REGISTER,
@@ -4564,6 +4565,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(unknown_lints)]
+    /// #![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; // ambiguous visibility
+    /// }
+    ///
+    /// fn main() {
+    ///     reexport::Z {};
+    /// }
+    /// ```
+    ///
+    /// {{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,
+    Warn,
+    "detects certain glob imports that require reporting an ambiguity error",
+    @future_incompatible = FutureIncompatibleInfo {
+        reason: fcw!(FutureReleaseError #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

@@ -408,6 +408,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,8 +24,8 @@ 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, AMBIGUOUS_PANIC_IMPORTS,
-    MACRO_EXPANDED_MACRO_EXPORTS_ACCESSED_BY_ABSOLUTE_PATHS,
+    ABSOLUTE_PATHS_NOT_STARTING_WITH_CRATE, AMBIGUOUS_GLOB_IMPORTS, AMBIGUOUS_IMPORT_VISIBILITIES,
+    AMBIGUOUS_PANIC_IMPORTS, MACRO_EXPANDED_MACRO_EXPORTS_ACCESSED_BY_ABSOLUTE_PATHS,
 };
 use rustc_session::utils::was_invoked_from_cargo;
 use rustc_span::edit_distance::find_best_match_for_name;
@@ -145,6 +145,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                 };
 
                 let lint = match ambiguity_warning {
+                    _ if ambiguity_error.ambig_vis.is_some() => AMBIGUOUS_IMPORT_VISIBILITIES,
                     AmbiguityWarning::GlobImport => AMBIGUOUS_GLOB_IMPORTS,
                     AmbiguityWarning::PanicImport => AMBIGUOUS_PANIC_IMPORTS,
                 };
@@ -1995,7 +1996,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
     }
 
     fn ambiguity_diagnostic(&self, ambiguity_error: &AmbiguityError<'ra>) -> errors::Ambiguity {
-        let AmbiguityError { kind, ident, b1, b2, scope1, scope2, .. } = *ambiguity_error;
+        let AmbiguityError { kind, ambig_vis, ident, b1, b2, scope1, scope2, .. } =
+            *ambiguity_error;
         let extern_prelude_ambiguity = || {
             // Note: b1 may come from a module scope, as an extern crate item in module.
             matches!(scope2, Scope::ExternPreludeFlags)
@@ -2074,9 +2076,18 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
             None
         };
 
+        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,
             help,
+            ambig_vis,
             kind: kind.descr(),
             b1_note,
             b1_help_msgs,

compiler/rustc_resolve/src/errors.rs

@@ -1463,6 +1463,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 help: Option<&'static [&'static str]>,
     pub b1_note: Spanned<String>,
@@ -1473,8 +1474,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);
         if let Some(help) = self.help {

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;
@@ -485,9 +486,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(decl));
-                        }
+                        let import_vis = match finalize {
+                            None | Some(Finalize { stage: Stage::Late, .. }) => {
+                                return ControlFlow::Break(Ok(decl));
+                            }
+                            Some(Finalize { import_vis, .. }) => import_vis,
+                        };
 
                         if let Some(&(innermost_decl, _)) = innermost_results.first() {
                             // Found another solution, if the first one was "weak", report an error.
@@ -500,6 +504,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                                 decl,
                                 scope,
                                 &innermost_results,
+                                import_vis,
                             ) {
                                 // No need to search for more potential ambiguities, one is enough.
                                 return ControlFlow::Break(Ok(innermost_decl));
@@ -779,12 +784,22 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         decl: Decl<'ra>,
         scope: Scope<'ra>,
         innermost_results: &[(Decl<'ra>, Scope<'ra>)],
+        import_vis: Option<Visibility>,
     ) -> bool {
         let (innermost_decl, innermost_scope) = innermost_results[0];
         let (res, innermost_res) = (decl.res(), innermost_decl.res());
-        if res == innermost_res {
+        let ambig_vis = if res != innermost_res {
+            None
+        } else if let Some(import_vis) = import_vis
+            && let min =
+                (|d: Decl<'_>| d.vis().min(import_vis.to_def_id(), self.tcx).expect_local())
+            && let (min1, min2) = (min(decl), min(innermost_decl))
+            && min1 != min2
+        {
+            Some((min1, min2))
+        } else {
             return false;
-        }
+        };
 
         // FIXME: Use `scope` instead of `res` to detect built-in attrs and derive helpers,
         // it will exclude imports, make slightly more code legal, and will require lang approval.
@@ -871,10 +886,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
                         || (self.is_specific_builtin_macro(res, sym::core_panic)
                             && self.is_specific_builtin_macro(innermost_res, sym::std_panic)));
 
-                let warning = is_issue_147319_hack.then_some(AmbiguityWarning::PanicImport);
+                let warning = if ambig_vis.is_some() {
+                    Some(AmbiguityWarning::GlobImport)
+                } else if is_issue_147319_hack {
+                    Some(AmbiguityWarning::PanicImport)
+                } else {
+                    None
+                };
 
                 self.ambiguity_errors.push(AmbiguityError {
                     kind,
+                    ambig_vis,
                     ident: ident.orig(orig_ident_span),
                     b1: innermost_decl,
                     b2: decl,

compiler/rustc_resolve/src/imports.rs

@@ -1189,7 +1189,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().decl(),
                     Some(import),
                 );

compiler/rustc_resolve/src/lib.rs

@@ -969,6 +969,7 @@ enum AmbiguityWarning {
 
 struct AmbiguityError<'ra> {
     kind: AmbiguityKind,
+    ambig_vis: Option<(Visibility, Visibility)>,
     ident: Ident,
     b1: Decl<'ra>,
     b2: Decl<'ra>,
@@ -2087,6 +2088,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
         if let Some(b2) = used_decl.ambiguity.get() {
             let ambiguity_error = AmbiguityError {
                 kind: AmbiguityKind::GlobVsGlob,
+                ambig_vis: None,
                 ident,
                 b1: used_decl,
                 b2,
@@ -2556,6 +2558,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 an import's final segment.
+    import_vis: Option<Visibility> = None,
 }
 
 impl Finalize {

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

@@ -0,0 +1,19 @@
+//@ check-pass
+//@ 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; //~ WARN ambiguous import visibility
+                            //~| WARN this was previously accepted
+fn main() {}

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

@@ -0,0 +1,29 @@
+warning: ambiguous import visibility: pub(crate) or pub
+  --> $DIR/ambiguous-import-visibility-macro.rs:17: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:7: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:11:1
+   |
+LL | #[macro_use] // this imports the `RustEmbed` macro with `pub(crate)` visibility
+   | ^^^^^^^^^^^^
+   = note: `#[warn(ambiguous_import_visibilities)]` (part of `#[warn(future_incompatible)]`) on by default
+   = note: this warning originates in the macro `globbing` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+warning: 1 warning emitted
+

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

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