Auto merge of #111378 - jieyouxu:local-shadows-glob-reexport, r=petrochenkov

Add warn-by-default lint when local binding shadows exported glob re-export item

This PR introduces a warn-by-default rustc lint for when a local binding (a use statement, or a type declaration) produces a name which shadows an exported glob re-export item, causing the name from the exported glob re-export to be hidden (see #111336).

### Unresolved Questions

- [x] ~~Is this approach correct? While it passes the UI tests, I'm not entirely convinced it is correct.~~ Seems to be ok now.
- [x] ~~What should the lint be called / how should it be worded? I don't like calling `use x::*;` or `struct Foo;` a "local binding" but they are `NameBinding`s internally if I'm not mistaken.~~ ~~The lint is called `local_binding_shadows_glob_reexport` for now, unless a better name is suggested.~~ `hidden_glob_reexports`.

Fixes #111336.

Author: bors

compiler/rustc_ast/src/token.rs

@@ -11,6 +11,7 @@ use rustc_data_structures::stable_hasher::{HashStable, StableHasher};
 use rustc_data_structures::sync::Lrc;
 use rustc_macros::HashStable_Generic;
 use rustc_span::symbol::{kw, sym};
+#[cfg_attr(not(bootstrap), allow(hidden_glob_reexports))]
 use rustc_span::symbol::{Ident, Symbol};
 use rustc_span::{self, edition::Edition, Span, DUMMY_SP};
 use std::borrow::Cow;

compiler/rustc_lint/src/context.rs

@@ -952,6 +952,10 @@ pub trait LintContext: Sized {
                     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));
                 }
+                BuiltinLintDiagnostics::HiddenGlobReexports { name, namespace, glob_reexport_span, private_item_span } => {
+                    db.span_label(glob_reexport_span, format!("the name `{}` in the {} namespace is supposed to be publicly re-exported here", name, namespace));
+                    db.span_label(private_item_span, "but the private item here shadows it");
+                }
             }
             // Rewrap `db`, and pass control to the user.
             decorate(db)

compiler/rustc_lint_defs/src/builtin.rs

@@ -3272,6 +3272,43 @@ declare_lint! {
     "ambiguous glob re-exports",
 }
 
+declare_lint! {
+    /// The `hidden_glob_reexports` lint detects cases where glob re-export items are shadowed by
+    /// private items.
+    ///
+    /// ### Example
+    ///
+    /// ```rust,compile_fail
+    /// #![deny(hidden_glob_reexports)]
+    ///
+    /// pub mod upstream {
+    ///     mod inner { pub struct Foo {}; pub struct Bar {}; }
+    ///     pub use self::inner::*;
+    ///     struct Foo {} // private item shadows `inner::Foo`
+    /// }
+    ///
+    /// // mod downstream {
+    /// //     fn test() {
+    /// //         let _ = crate::upstream::Foo; // inaccessible
+    /// //     }
+    /// // }
+    ///
+    /// pub fn main() {}
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// This was previously accepted without any errors or warnings but it could silently break a
+    /// crate's downstream user code. If the `struct Foo` was added, `dep::inner::Foo` would
+    /// silently become inaccessible and trigger a "`struct `Foo` is private`" visibility error at
+    /// the downstream use site.
+    pub HIDDEN_GLOB_REEXPORTS,
+    Warn,
+    "name introduced by a private item shadows a name introduced by a public glob re-export",
+}
+
 declare_lint_pass! {
     /// Does nothing as a lint pass, but registers some `Lint`s
     /// that are used by other parts of the compiler.
@@ -3304,6 +3341,7 @@ declare_lint_pass! {
         FORBIDDEN_LINT_GROUPS,
         FUNCTION_ITEM_REFERENCES,
         FUZZY_PROVENANCE_CASTS,
+        HIDDEN_GLOB_REEXPORTS,
         ILL_FORMED_ATTRIBUTE_INPUT,
         ILLEGAL_FLOATING_POINT_LITERAL_PATTERN,
         IMPLIED_BOUNDS_ENTAILMENT,

compiler/rustc_lint_defs/src/lib.rs

@@ -540,6 +540,16 @@ pub enum BuiltinLintDiagnostics {
         /// Span where the same name is also re-exported.
         duplicate_reexport_span: Span,
     },
+    HiddenGlobReexports {
+        /// The name of the local binding which shadows the glob re-export.
+        name: String,
+        /// The namespace for which the shadowing occurred in.
+        namespace: String,
+        /// The glob reexport that is shadowed by the local binding.
+        glob_reexport_span: Span,
+        /// The local binding that shadows the glob reexport.
+        private_item_span: Span,
+    },
 }
 
 /// Lints that are buffered up early on in the `Session` before the

compiler/rustc_middle/src/ty/mod.rs

@@ -53,7 +53,6 @@ use rustc_span::symbol::{kw, sym, Ident, Symbol};
 use rustc_span::{ExpnId, ExpnKind, Span};
 use rustc_target::abi::{Align, FieldIdx, Integer, IntegerType, VariantIdx};
 pub use rustc_target::abi::{ReprFlags, ReprOptions};
-use rustc_type_ir::WithCachedTypeInfo;
 pub use subst::*;
 pub use vtable::*;
 
@@ -145,6 +144,7 @@ mod opaque_types;
 mod parameterized;
 mod rvalue_scopes;
 mod structural_impls;
+#[cfg_attr(not(bootstrap), allow(hidden_glob_reexports))]
 mod sty;
 mod typeck_results;
 

compiler/rustc_resolve/src/imports.rs

@@ -21,7 +21,8 @@ use rustc_middle::metadata::Reexport;
 use rustc_middle::span_bug;
 use rustc_middle::ty;
 use rustc_session::lint::builtin::{
-    AMBIGUOUS_GLOB_REEXPORTS, PUB_USE_OF_PRIVATE_EXTERN_CRATE, UNUSED_IMPORTS,
+    AMBIGUOUS_GLOB_REEXPORTS, HIDDEN_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;
@@ -526,31 +527,71 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
         }
     }
 
-    pub(crate) fn check_reexport_ambiguities(
+    pub(crate) fn check_hidden_glob_reexports(
         &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,
-                        },
-                    );
+            for (key, resolution) in self.resolutions(module).borrow().iter() {
+                let resolution = resolution.borrow();
+
+                if let Some(binding) = resolution.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))
+                    {
+                        self.lint_buffer.buffer_lint_with_diagnostic(
+                            AMBIGUOUS_GLOB_REEXPORTS,
+                            import.root_id,
+                            import.root_span,
+                            "ambiguous glob re-exports",
+                            BuiltinLintDiagnostics::AmbiguousGlobReexports {
+                                name: key.ident.to_string(),
+                                namespace: key.ns.descr().to_string(),
+                                first_reexport_span: import.root_span,
+                                duplicate_reexport_span: amb_binding.span,
+                            },
+                        );
+                    }
+
+                    if let Some(glob_binding) = resolution.shadowed_glob {
+                        let binding_id = match binding.kind {
+                            NameBindingKind::Res(res) => {
+                                Some(self.def_id_to_node_id[res.def_id().expect_local()])
+                            }
+                            NameBindingKind::Module(module) => {
+                                Some(self.def_id_to_node_id[module.def_id().expect_local()])
+                            }
+                            NameBindingKind::Import { import, .. } => import.id(),
+                        };
+
+                        if binding.res() != Res::Err
+                            && glob_binding.res() != Res::Err
+                            && let NameBindingKind::Import { import: glob_import, .. } = glob_binding.kind
+                            && let Some(binding_id) = binding_id
+                            && let Some(glob_import_id) = glob_import.id()
+                            && let glob_import_def_id = self.local_def_id(glob_import_id)
+                            && self.effective_visibilities.is_exported(glob_import_def_id)
+                            && glob_binding.vis.is_public()
+                            && !binding.vis.is_public()
+                        {
+                            self.lint_buffer.buffer_lint_with_diagnostic(
+                                HIDDEN_GLOB_REEXPORTS,
+                                binding_id,
+                                binding.span,
+                                "private item shadows public glob re-export",
+                                BuiltinLintDiagnostics::HiddenGlobReexports {
+                                    name: key.ident.name.to_string(),
+                                    namespace: key.ns.descr().to_owned(),
+                                    glob_reexport_span: glob_binding.span,
+                                    private_item_span: binding.span,
+                                },
+                            );
+                        }
+                    }
                 }
-            });
+            }
         }
     }
 

compiler/rustc_resolve/src/lib.rs

@@ -1496,8 +1496,8 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> {
             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("check_hidden_glob_reexports", || {
+                self.check_hidden_glob_reexports(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));

compiler/rustc_trait_selection/src/traits/mod.rs

@@ -14,10 +14,12 @@ mod object_safety;
 pub mod outlives_bounds;
 mod project;
 pub mod query;
+#[cfg_attr(not(bootstrap), allow(hidden_glob_reexports))]
 mod select;
 mod specialize;
 mod structural_match;
 mod structural_normalize;
+#[cfg_attr(not(bootstrap), allow(hidden_glob_reexports))]
 mod util;
 mod vtable;
 pub mod wf;

tests/ui/imports/issue-55884-2.rs

@@ -6,6 +6,7 @@ mod parser {
     pub use options::*;
     // Private single import shadows public glob import, but arrives too late for initial
     // resolution of `use parser::ParseOptions` because it depends on that resolution itself.
+    #[allow(hidden_glob_reexports)]
     use ParseOptions;
 }
 

tests/ui/imports/issue-55884-2.stderr

@@ -1,16 +1,16 @@
 error[E0603]: struct import `ParseOptions` is private
-  --> $DIR/issue-55884-2.rs:12:17
+  --> $DIR/issue-55884-2.rs:13:17
    |
 LL | pub use parser::ParseOptions;
    |                 ^^^^^^^^^^^^ private struct import
    |
 note: the struct import `ParseOptions` is defined here...
-  --> $DIR/issue-55884-2.rs:9:9
+  --> $DIR/issue-55884-2.rs:10:9
    |
 LL |     use ParseOptions;
    |         ^^^^^^^^^^^^
 note: ...and refers to the struct import `ParseOptions` which is defined here...
-  --> $DIR/issue-55884-2.rs:12:9
+  --> $DIR/issue-55884-2.rs:13:9
    |
 LL | pub use parser::ParseOptions;
    |         ^^^^^^^^^^^^^^^^^^^^ consider importing it directly

tests/ui/resolve/hidden_glob_reexports.rs

@@ -0,0 +1,52 @@
+// check-pass
+
+pub mod upstream_a {
+    mod inner {
+        pub struct Foo {}
+        pub struct Bar {}
+    }
+
+    pub use self::inner::*;
+
+    struct Foo;
+    //~^ WARN private item shadows public glob re-export
+}
+
+pub mod upstream_b {
+    mod inner {
+        pub struct Foo {}
+        pub struct Qux {}
+    }
+
+    mod other {
+        pub struct Foo;
+    }
+
+    pub use self::inner::*;
+
+    use self::other::Foo;
+    //~^ WARN private item shadows public glob re-export
+}
+
+pub mod upstream_c {
+    mod no_def_id {
+        #![allow(non_camel_case_types)]
+        pub struct u8;
+        pub struct World;
+    }
+
+    pub use self::no_def_id::*;
+
+    use std::primitive::u8;
+    //~^ WARN private item shadows public glob re-export
+}
+
+// Downstream crate
+// mod downstream {
+//     fn proof() {
+//         let _ = crate::upstream_a::Foo;
+//         let _ = crate::upstream_b::Foo;
+//     }
+// }
+
+pub fn main() {}

tests/ui/resolve/hidden_glob_reexports.stderr

@@ -0,0 +1,31 @@
+warning: private item shadows public glob re-export
+  --> $DIR/hidden_glob_reexports.rs:11:5
+   |
+LL |     pub use self::inner::*;
+   |             -------------- the name `Foo` in the type namespace is supposed to be publicly re-exported here
+LL |
+LL |     struct Foo;
+   |     ^^^^^^^^^^^ but the private item here shadows it
+   |
+   = note: `#[warn(hidden_glob_reexports)]` on by default
+
+warning: private item shadows public glob re-export
+  --> $DIR/hidden_glob_reexports.rs:27:9
+   |
+LL |     pub use self::inner::*;
+   |             -------------- the name `Foo` in the type namespace is supposed to be publicly re-exported here
+LL |
+LL |     use self::other::Foo;
+   |         ^^^^^^^^^^^^^^^^ but the private item here shadows it
+
+warning: private item shadows public glob re-export
+  --> $DIR/hidden_glob_reexports.rs:40:9
+   |
+LL |     pub use self::no_def_id::*;
+   |             ------------------ the name `u8` in the type namespace is supposed to be publicly re-exported here
+LL |
+LL |     use std::primitive::u8;
+   |         ^^^^^^^^^^^^^^^^^^ but the private item here shadows it
+
+warning: 3 warnings emitted
+