Rollup merge of #107880 - jieyouxu:issue-107563, r=petrochenkov

Lint ambiguous glob re-exports

Attempts to fix #107563.

We currently already emit errors for ambiguous re-exports when two names are re-exported *specifically*, i.e. not from glob exports. This PR attempts to emit deny-by-default lints for ambiguous glob re-exports.

Author: matthiaskrgr

compiler/rustc_lint/src/context.rs

@@ -910,6 +910,10 @@ pub trait LintContext: Sized {
                         Applicability::MachineApplicable,
                     );
                 }
+                BuiltinLintDiagnostics::AmbiguousGlobReexports { name, namespace, first_reexport_span, duplicate_reexport_span } => {
+                    db.span_label(first_reexport_span, format!("the name `{}` in the {} namespace is first re-exported here", name, namespace));
+                    db.span_label(duplicate_reexport_span, format!("but the name `{}` in the {} namespace is also re-exported here", name, namespace));
+                }
             }
             // Rewrap `db`, and pass control to the user.
             decorate(db)

compiler/rustc_lint_defs/src/builtin.rs

@@ -3230,6 +3230,45 @@ declare_lint! {
     };
 }
 
+declare_lint! {
+    /// The `ambiguous_glob_reexports` lint detects cases where names re-exported via globs
+    /// collide. Downstream users trying to use the same name re-exported from multiple globs
+    /// will receive a warning pointing out redefinition of the same name.
+    ///
+    /// ### Example
+    ///
+    /// ```rust,compile_fail
+    /// #![deny(ambiguous_glob_reexports)]
+    /// pub mod foo {
+    ///     pub type X = u8;
+    /// }
+    ///
+    /// pub mod bar {
+    ///     pub type Y = u8;
+    ///     pub type X = u8;
+    /// }
+    ///
+    /// pub use foo::*;
+    /// pub use bar::*;
+    ///
+    ///
+    /// pub fn main() {}
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// This was previously accepted but it could silently break a crate's downstream users code.
+    /// For example, if `foo::*` and `bar::*` were re-exported before `bar::X` was added to the
+    /// re-exports, down stream users could use `this_crate::X` without problems. However, adding
+    /// `bar::X` would cause compilation errors in downstream crates because `X` is defined
+    /// multiple times in the same namespace of `this_crate`.
+    pub AMBIGUOUS_GLOB_REEXPORTS,
+    Warn,
+    "ambiguous glob re-exports",
+}
+
 declare_lint_pass! {
     /// Does nothing as a lint pass, but registers some `Lint`s
     /// that are used by other parts of the compiler.
@@ -3337,6 +3376,7 @@ declare_lint_pass! {
         NAMED_ARGUMENTS_USED_POSITIONALLY,
         IMPLIED_BOUNDS_ENTAILMENT,
         BYTE_SLICE_IN_PACKED_STRUCT_WITH_DERIVE,
+        AMBIGUOUS_GLOB_REEXPORTS,
     ]
 }
 

compiler/rustc_lint_defs/src/lib.rs

@@ -529,6 +529,16 @@ pub enum BuiltinLintDiagnostics {
         vis_span: Span,
         ident_span: Span,
     },
+    AmbiguousGlobReexports {
+        /// The name for which collision(s) have occurred.
+        name: String,
+        /// The name space for whihc the collision(s) occurred in.
+        namespace: String,
+        /// Span where the name is first re-exported.
+        first_reexport_span: Span,
+        /// Span where the same name is also re-exported.
+        duplicate_reexport_span: Span,
+    },
 }
 
 /// Lints that are buffered up early on in the `Session` before the

compiler/rustc_resolve/src/effective_visibilities.rs

@@ -4,6 +4,7 @@ use rustc_ast::visit;
 use rustc_ast::visit::Visitor;
 use rustc_ast::Crate;
 use rustc_ast::EnumDef;
+use rustc_data_structures::fx::FxHashSet;
 use rustc_data_structures::intern::Interned;
 use rustc_hir::def_id::LocalDefId;
 use rustc_hir::def_id::CRATE_DEF_ID;
@@ -70,11 +71,11 @@ impl Resolver<'_, '_> {
 impl<'r, 'a, 'tcx> EffectiveVisibilitiesVisitor<'r, 'a, 'tcx> {
     /// Fills the `Resolver::effective_visibilities` table with public & exported items
     /// For now, this doesn't resolve macros (FIXME) and cannot resolve Impl, as we
-    /// need access to a TyCtxt for that.
+    /// need access to a TyCtxt for that. Returns the set of ambiguous re-exports.
     pub(crate) fn compute_effective_visibilities<'c>(
         r: &'r mut Resolver<'a, 'tcx>,
         krate: &'c Crate,
-    ) {
+    ) -> FxHashSet<Interned<'a, NameBinding<'a>>> {
         let mut visitor = EffectiveVisibilitiesVisitor {
             r,
             def_effective_visibilities: Default::default(),
@@ -93,18 +94,26 @@ impl<'r, 'a, 'tcx> EffectiveVisibilitiesVisitor<'r, 'a, 'tcx> {
         }
         visitor.r.effective_visibilities = visitor.def_effective_visibilities;
 
+        let mut exported_ambiguities = FxHashSet::default();
+
         // Update visibilities for import def ids. These are not used during the
         // `EffectiveVisibilitiesVisitor` pass, because we have more detailed binding-based
         // information, but are used by later passes. Effective visibility of an import def id
         // is the maximum value among visibilities of bindings corresponding to that def id.
         for (binding, eff_vis) in visitor.import_effective_visibilities.iter() {
             let NameBindingKind::Import { import, .. } = binding.kind else { unreachable!() };
-            if let Some(node_id) = import.id() {
-                r.effective_visibilities.update_eff_vis(r.local_def_id(node_id), eff_vis, r.tcx)
+            if !binding.is_ambiguity() {
+                if let Some(node_id) = import.id() {
+                    r.effective_visibilities.update_eff_vis(r.local_def_id(node_id), eff_vis, r.tcx)
+                }
+            } else if binding.ambiguity.is_some() && eff_vis.is_public_at_level(Level::Reexported) {
+                exported_ambiguities.insert(*binding);
             }
         }
 
         info!("resolve::effective_visibilities: {:#?}", r.effective_visibilities);
+
+        exported_ambiguities
     }
 
     /// Update effective visibilities of bindings in the given module,
@@ -115,21 +124,44 @@ impl<'r, 'a, 'tcx> EffectiveVisibilitiesVisitor<'r, 'a, 'tcx> {
         let resolutions = self.r.resolutions(module);
 
         for (_, name_resolution) in resolutions.borrow().iter() {
-            if let Some(mut binding) = name_resolution.borrow().binding() && !binding.is_ambiguity() {
-                // Set the given effective visibility level to `Level::Direct` and
-                // sets the rest of the `use` chain to `Level::Reexported` until
-                // we hit the actual exported item.
-                let mut parent_id = ParentId::Def(module_id);
-                while let NameBindingKind::Import { binding: nested_binding, .. } = binding.kind {
-                    let binding_id = ImportId::new_unchecked(binding);
-                    self.update_import(binding_id, parent_id);
-
-                    parent_id = ParentId::Import(binding_id);
-                    binding = nested_binding;
-                }
+            if let Some(mut binding) = name_resolution.borrow().binding() {
+                if !binding.is_ambiguity() {
+                    // Set the given effective visibility level to `Level::Direct` and
+                    // sets the rest of the `use` chain to `Level::Reexported` until
+                    // we hit the actual exported item.
+                    let mut parent_id = ParentId::Def(module_id);
+                    while let NameBindingKind::Import { binding: nested_binding, .. } = binding.kind
+                    {
+                        let binding_id = ImportId::new_unchecked(binding);
+                        self.update_import(binding_id, parent_id);
+
+                        parent_id = ParentId::Import(binding_id);
+                        binding = nested_binding;
+                    }
+
+                    if let Some(def_id) = binding.res().opt_def_id().and_then(|id| id.as_local()) {
+                        self.update_def(def_id, binding.vis.expect_local(), parent_id);
+                    }
+                } else {
+                    // Put the root ambiguity binding and all reexports leading to it into the
+                    // table. They are used by the `ambiguous_glob_reexports` lint. For all
+                    // bindings added to the table here `is_ambiguity` returns true.
+                    let mut parent_id = ParentId::Def(module_id);
+                    while let NameBindingKind::Import { binding: nested_binding, .. } = binding.kind
+                    {
+                        let binding_id = ImportId::new_unchecked(binding);
+                        self.update_import(binding_id, parent_id);
 
-                if let Some(def_id) = binding.res().opt_def_id().and_then(|id| id.as_local()) {
-                    self.update_def(def_id, binding.vis.expect_local(), parent_id);
+                        if binding.ambiguity.is_some() {
+                            // Stop at the root ambiguity, further bindings in the chain should not
+                            // be reexported because the root ambiguity blocks any access to them.
+                            // (Those further bindings are most likely not ambiguities themselves.)
+                            break;
+                        }
+
+                        parent_id = ParentId::Import(binding_id);
+                        binding = nested_binding;
+                    }
                 }
             }
         }

compiler/rustc_resolve/src/imports.rs

@@ -19,7 +19,9 @@ use rustc_hir::def::{self, DefKind, PartialRes};
 use rustc_middle::metadata::ModChild;
 use rustc_middle::span_bug;
 use rustc_middle::ty;
-use rustc_session::lint::builtin::{PUB_USE_OF_PRIVATE_EXTERN_CRATE, UNUSED_IMPORTS};
+use rustc_session::lint::builtin::{
+    AMBIGUOUS_GLOB_REEXPORTS, PUB_USE_OF_PRIVATE_EXTERN_CRATE, UNUSED_IMPORTS,
+};
 use rustc_session::lint::BuiltinLintDiagnostics;
 use rustc_span::edit_distance::find_best_match_for_name;
 use rustc_span::hygiene::LocalExpnId;
@@ -510,6 +512,34 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         }
     }
 
+    pub(crate) fn check_reexport_ambiguities(
+        &mut self,
+        exported_ambiguities: FxHashSet<Interned<'a, NameBinding<'a>>>,
+    ) {
+        for module in self.arenas.local_modules().iter() {
+            module.for_each_child(self, |this, ident, ns, binding| {
+                if let NameBindingKind::Import { import, .. } = binding.kind
+                && let Some((amb_binding, _)) = binding.ambiguity
+                && binding.res() != Res::Err
+                && exported_ambiguities.contains(&Interned::new_unchecked(binding))
+                {
+                    this.lint_buffer.buffer_lint_with_diagnostic(
+                        AMBIGUOUS_GLOB_REEXPORTS,
+                        import.root_id,
+                        import.root_span,
+                        "ambiguous glob re-exports",
+                        BuiltinLintDiagnostics::AmbiguousGlobReexports {
+                            name: ident.to_string(),
+                            namespace: ns.descr().to_string(),
+                            first_reexport_span: import.root_span,
+                            duplicate_reexport_span: amb_binding.span,
+                        },
+                    );
+                }
+            });
+        }
+    }
+
     fn throw_unresolved_import_error(&self, errors: Vec<(&Import<'_>, UnresolvedImportError)>) {
         if errors.is_empty() {
             return;

compiler/rustc_resolve/src/lib.rs

@@ -1475,9 +1475,12 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
     pub fn resolve_crate(&mut self, krate: &Crate) {
         self.tcx.sess.time("resolve_crate", || {
             self.tcx.sess.time("finalize_imports", || self.finalize_imports());
-            self.tcx.sess.time("compute_effective_visibilities", || {
+            let exported_ambiguities = self.tcx.sess.time("compute_effective_visibilities", || {
                 EffectiveVisibilitiesVisitor::compute_effective_visibilities(self, krate)
             });
+            self.tcx.sess.time("check_reexport_ambiguities", || {
+                self.check_reexport_ambiguities(exported_ambiguities)
+            });
             self.tcx.sess.time("finalize_macro_resolutions", || self.finalize_macro_resolutions());
             self.tcx.sess.time("late_resolve_crate", || self.late_resolve_crate(krate));
             self.tcx.sess.time("resolve_main", || self.resolve_main());

tests/ui/imports/auxiliary/glob-conflict.rs

@@ -1,3 +1,5 @@
+#![allow(ambiguous_glob_reexports)]
+
 mod m1 {
     pub fn f() {}
 }

tests/ui/imports/local-modularized-tricky-fail-1.rs

@@ -1,4 +1,5 @@
 #![feature(decl_macro)]
+#![allow(ambiguous_glob_reexports)]
 
 macro_rules! define_exported { () => {
     #[macro_export]

tests/ui/imports/local-modularized-tricky-fail-1.stderr

@@ -1,12 +1,12 @@
 error[E0659]: `exported` is ambiguous
-  --> $DIR/local-modularized-tricky-fail-1.rs:28:1
+  --> $DIR/local-modularized-tricky-fail-1.rs:29:1
    |
 LL | exported!();
    | ^^^^^^^^ ambiguous name
    |
    = note: ambiguous because of a conflict between a name from a glob import and a macro-expanded name in the same module during import or macro resolution
 note: `exported` could refer to the macro defined here
-  --> $DIR/local-modularized-tricky-fail-1.rs:5:5
+  --> $DIR/local-modularized-tricky-fail-1.rs:6:5
    |
 LL | /     macro_rules! exported {
 LL | |         () => ()
@@ -16,23 +16,23 @@ LL | |     }
 LL |       define_exported!();
    |       ------------------ in this macro invocation
 note: `exported` could also refer to the macro imported here
-  --> $DIR/local-modularized-tricky-fail-1.rs:22:5
+  --> $DIR/local-modularized-tricky-fail-1.rs:23:5
    |
 LL | use inner1::*;
    |     ^^^^^^^^^
    = help: consider adding an explicit import of `exported` to disambiguate
    = note: this error originates in the macro `define_exported` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error[E0659]: `panic` is ambiguous
-  --> $DIR/local-modularized-tricky-fail-1.rs:35:5
+  --> $DIR/local-modularized-tricky-fail-1.rs:36:5
    |
 LL |     panic!();
    |     ^^^^^ ambiguous name
    |
    = note: ambiguous because of a conflict between a macro-expanded name and a less macro-expanded name from outer scope during import or macro resolution
    = note: `panic` could refer to a macro from prelude
 note: `panic` could also refer to the macro defined here
-  --> $DIR/local-modularized-tricky-fail-1.rs:11:5
+  --> $DIR/local-modularized-tricky-fail-1.rs:12:5
    |
 LL | /     macro_rules! panic {
 LL | |         () => ()
@@ -45,15 +45,15 @@ LL |       define_panic!();
    = note: this error originates in the macro `define_panic` (in Nightly builds, run with -Z macro-backtrace for more info)
 
 error[E0659]: `include` is ambiguous
-  --> $DIR/local-modularized-tricky-fail-1.rs:46:1
+  --> $DIR/local-modularized-tricky-fail-1.rs:47:1
    |
 LL | include!();
    | ^^^^^^^ ambiguous name
    |
    = note: ambiguous because of a conflict between a macro-expanded name and a less macro-expanded name from outer scope during import or macro resolution
    = note: `include` could refer to a macro from prelude
 note: `include` could also refer to the macro defined here
-  --> $DIR/local-modularized-tricky-fail-1.rs:17:5
+  --> $DIR/local-modularized-tricky-fail-1.rs:18:5
    |
 LL | /     macro_rules! include {
 LL | |         () => ()

tests/ui/resolve/issue-107563-ambiguous-glob-reexports.rs

@@ -0,0 +1,33 @@
+#![deny(ambiguous_glob_reexports)]
+
+pub mod foo {
+    pub type X = u8;
+}
+
+pub mod bar {
+    pub type X = u8;
+    pub type Y = u8;
+}
+
+pub use foo::*;
+//~^ ERROR ambiguous glob re-exports
+pub use bar::*;
+
+mod ambiguous {
+    mod m1 { pub type A = u8; }
+    mod m2 { pub type A = u8; }
+    pub use self::m1::*;
+    //~^ ERROR ambiguous glob re-exports
+    pub use self::m2::*;
+}
+
+pub mod single {
+    pub use ambiguous::A;
+    //~^ ERROR `A` is ambiguous
+}
+
+pub mod glob {
+    pub use ambiguous::*;
+}
+
+pub fn main() {}