From 1f67949f0e9cd6c80a3e325164ef17663127f07b 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: Mon, 20 Mar 2023 03:11:28 +0800 Subject: [PATCH] Lint ambiguous glob re-exports --- compiler/rustc_lint/src/context.rs | 4 ++ compiler/rustc_lint_defs/src/builtin.rs | 40 +++++++++++ compiler/rustc_lint_defs/src/lib.rs | 10 +++ .../src/effective_visibilities.rs | 68 ++++++++++++++----- compiler/rustc_resolve/src/imports.rs | 32 ++++++++- compiler/rustc_resolve/src/lib.rs | 5 +- tests/ui/imports/auxiliary/glob-conflict.rs | 2 + .../local-modularized-tricky-fail-1.rs | 1 + .../local-modularized-tricky-fail-1.stderr | 14 ++-- .../issue-107563-ambiguous-glob-reexports.rs | 33 +++++++++ ...sue-107563-ambiguous-glob-reexports.stderr | 47 +++++++++++++ 11 files changed, 229 insertions(+), 27 deletions(-) create mode 100644 tests/ui/resolve/issue-107563-ambiguous-glob-reexports.rs create mode 100644 tests/ui/resolve/issue-107563-ambiguous-glob-reexports.stderr diff --git a/compiler/rustc_lint/src/context.rs b/compiler/rustc_lint/src/context.rs index f5a711315ea4c..626c09fea07ac 100644 --- a/compiler/rustc_lint/src/context.rs +++ b/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) diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 91966e75b5fa2..242d147b5ee11 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/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, ] } diff --git a/compiler/rustc_lint_defs/src/lib.rs b/compiler/rustc_lint_defs/src/lib.rs index 6f22bdabff450..69a8b691ab216 100644 --- a/compiler/rustc_lint_defs/src/lib.rs +++ b/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 diff --git a/compiler/rustc_resolve/src/effective_visibilities.rs b/compiler/rustc_resolve/src/effective_visibilities.rs index 4bb252bfb29dd..a1ae9b8a52181 100644 --- a/compiler/rustc_resolve/src/effective_visibilities.rs +++ b/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>> { 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; + } } } } diff --git a/compiler/rustc_resolve/src/imports.rs b/compiler/rustc_resolve/src/imports.rs index 6e27bcc5bf3d1..b36fca9bb9623 100644 --- a/compiler/rustc_resolve/src/imports.rs +++ b/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; @@ -506,6 +508,34 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } } + pub(crate) fn check_reexport_ambiguities( + &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, + }, + ); + } + }); + } + } + fn throw_unresolved_import_error(&self, errors: Vec<(&Import<'_>, UnresolvedImportError)>) { if errors.is_empty() { return; diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index cd90fd3ef84d8..de603d87e6d2d 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -1474,9 +1474,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()); diff --git a/tests/ui/imports/auxiliary/glob-conflict.rs b/tests/ui/imports/auxiliary/glob-conflict.rs index c83db64c6437c..8a146378b4395 100644 --- a/tests/ui/imports/auxiliary/glob-conflict.rs +++ b/tests/ui/imports/auxiliary/glob-conflict.rs @@ -1,3 +1,5 @@ +#![allow(ambiguous_glob_reexports)] + mod m1 { pub fn f() {} } diff --git a/tests/ui/imports/local-modularized-tricky-fail-1.rs b/tests/ui/imports/local-modularized-tricky-fail-1.rs index 29e9b8ec841f5..ce700ae0de9b1 100644 --- a/tests/ui/imports/local-modularized-tricky-fail-1.rs +++ b/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] diff --git a/tests/ui/imports/local-modularized-tricky-fail-1.stderr b/tests/ui/imports/local-modularized-tricky-fail-1.stderr index 20eadaaaa56b8..52a01e8bcdfe3 100644 --- a/tests/ui/imports/local-modularized-tricky-fail-1.stderr +++ b/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,7 +16,7 @@ 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::*; | ^^^^^^^^^ @@ -24,7 +24,7 @@ LL | use inner1::*; = 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 @@ -32,7 +32,7 @@ LL | panic!(); = 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,7 +45,7 @@ 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 @@ -53,7 +53,7 @@ LL | include!(); = 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 | | () => () diff --git a/tests/ui/resolve/issue-107563-ambiguous-glob-reexports.rs b/tests/ui/resolve/issue-107563-ambiguous-glob-reexports.rs new file mode 100644 index 0000000000000..431213e25e462 --- /dev/null +++ b/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() {} diff --git a/tests/ui/resolve/issue-107563-ambiguous-glob-reexports.stderr b/tests/ui/resolve/issue-107563-ambiguous-glob-reexports.stderr new file mode 100644 index 0000000000000..07e61dd8643d4 --- /dev/null +++ b/tests/ui/resolve/issue-107563-ambiguous-glob-reexports.stderr @@ -0,0 +1,47 @@ +error[E0659]: `A` is ambiguous + --> $DIR/issue-107563-ambiguous-glob-reexports.rs:25:24 + | +LL | pub use ambiguous::A; + | ^ ambiguous name + | + = note: ambiguous because of multiple glob imports of a name in the same module +note: `A` could refer to the type alias imported here + --> $DIR/issue-107563-ambiguous-glob-reexports.rs:19:13 + | +LL | pub use self::m1::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `A` to disambiguate +note: `A` could also refer to the type alias imported here + --> $DIR/issue-107563-ambiguous-glob-reexports.rs:21:13 + | +LL | pub use self::m2::*; + | ^^^^^^^^^^^ + = help: consider adding an explicit import of `A` to disambiguate + +error: ambiguous glob re-exports + --> $DIR/issue-107563-ambiguous-glob-reexports.rs:12:9 + | +LL | pub use foo::*; + | ^^^^^^ the name `X` in the type namespace is first re-exported here +LL | +LL | pub use bar::*; + | ------ but the name `X` in the type namespace is also re-exported here + | +note: the lint level is defined here + --> $DIR/issue-107563-ambiguous-glob-reexports.rs:1:9 + | +LL | #![deny(ambiguous_glob_reexports)] + | ^^^^^^^^^^^^^^^^^^^^^^^^ + +error: ambiguous glob re-exports + --> $DIR/issue-107563-ambiguous-glob-reexports.rs:19:13 + | +LL | pub use self::m1::*; + | ^^^^^^^^^^^ the name `A` in the type namespace is first re-exported here +LL | +LL | pub use self::m2::*; + | ----------- but the name `A` in the type namespace is also re-exported here + +error: aborting due to 3 previous errors + +For more information about this error, try `rustc --explain E0659`.