From b9606589c4ca72008dc8d769f1a1c2b3578be65f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=AE=B8=E6=9D=B0=E5=8F=8B=20Jieyou=20Xu=20=28Joe=29?= Date: Tue, 9 May 2023 07:15:46 +0800 Subject: [PATCH] Add warn-by-default lint for local binding shadowing exported glob re-export item --- compiler/rustc_ast/src/token.rs | 1 + compiler/rustc_lint/src/context.rs | 4 + compiler/rustc_lint_defs/src/builtin.rs | 38 +++++++++ compiler/rustc_lint_defs/src/lib.rs | 10 +++ compiler/rustc_middle/src/ty/mod.rs | 2 +- compiler/rustc_resolve/src/imports.rs | 83 ++++++++++++++----- compiler/rustc_resolve/src/lib.rs | 4 +- .../rustc_trait_selection/src/traits/mod.rs | 2 + tests/ui/imports/issue-55884-2.rs | 1 + tests/ui/imports/issue-55884-2.stderr | 6 +- tests/ui/resolve/hidden_glob_reexports.rs | 52 ++++++++++++ tests/ui/resolve/hidden_glob_reexports.stderr | 31 +++++++ 12 files changed, 207 insertions(+), 27 deletions(-) create mode 100644 tests/ui/resolve/hidden_glob_reexports.rs create mode 100644 tests/ui/resolve/hidden_glob_reexports.stderr diff --git a/compiler/rustc_ast/src/token.rs b/compiler/rustc_ast/src/token.rs index 7ef39f8026b20..6646fa9446fb1 100644 --- a/compiler/rustc_ast/src/token.rs +++ b/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; diff --git a/compiler/rustc_lint/src/context.rs b/compiler/rustc_lint/src/context.rs index 1d0c43e95e085..947530a1b65a9 100644 --- a/compiler/rustc_lint/src/context.rs +++ b/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) diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 6e9dc880a7dee..1507087bdd4f3 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/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, diff --git a/compiler/rustc_lint_defs/src/lib.rs b/compiler/rustc_lint_defs/src/lib.rs index e27e322db8858..5a5031b791964 100644 --- a/compiler/rustc_lint_defs/src/lib.rs +++ b/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 diff --git a/compiler/rustc_middle/src/ty/mod.rs b/compiler/rustc_middle/src/ty/mod.rs index a8d0dca37ff99..96023a68cf6d0 100644 --- a/compiler/rustc_middle/src/ty/mod.rs +++ b/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; diff --git a/compiler/rustc_resolve/src/imports.rs b/compiler/rustc_resolve/src/imports.rs index 7c4c05d4b9452..c1bb262c0d407 100644 --- a/compiler/rustc_resolve/src/imports.rs +++ b/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>>, ) { 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, + }, + ); + } + } } - }); + } } } diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 3d2bd8429068e..fd977e8e254a2 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/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)); diff --git a/compiler/rustc_trait_selection/src/traits/mod.rs b/compiler/rustc_trait_selection/src/traits/mod.rs index f265230ff772d..a44d8955ab95c 100644 --- a/compiler/rustc_trait_selection/src/traits/mod.rs +++ b/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; diff --git a/tests/ui/imports/issue-55884-2.rs b/tests/ui/imports/issue-55884-2.rs index 75bb4206f97d6..6f8d0cf8ae2a2 100644 --- a/tests/ui/imports/issue-55884-2.rs +++ b/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; } diff --git a/tests/ui/imports/issue-55884-2.stderr b/tests/ui/imports/issue-55884-2.stderr index 5adbc4b66d133..67d4114149a4b 100644 --- a/tests/ui/imports/issue-55884-2.stderr +++ b/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 diff --git a/tests/ui/resolve/hidden_glob_reexports.rs b/tests/ui/resolve/hidden_glob_reexports.rs new file mode 100644 index 0000000000000..361243fcd7bdf --- /dev/null +++ b/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() {} diff --git a/tests/ui/resolve/hidden_glob_reexports.stderr b/tests/ui/resolve/hidden_glob_reexports.stderr new file mode 100644 index 0000000000000..ddf7bcda827a8 --- /dev/null +++ b/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 +