diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 8c69cc089cafb..24ca746326532 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -19,6 +19,7 @@ declare_lint_pass! { AMBIGUOUS_ASSOCIATED_ITEMS, AMBIGUOUS_GLOB_IMPORTS, AMBIGUOUS_GLOB_REEXPORTS, + AMBIGUOUS_IMPORT_VISIBILITIES, ARITHMETIC_OVERFLOW, ASM_SUB_REGISTER, BAD_ASM_STYLE, @@ -4472,6 +4473,54 @@ 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(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 diff --git a/compiler/rustc_middle/src/ty/mod.rs b/compiler/rustc_middle/src/ty/mod.rs index e2a94b607de10..c803dc24b384b 100644 --- a/compiler/rustc_middle/src/ty/mod.rs +++ b/compiler/rustc_middle/src/ty/mod.rs @@ -408,6 +408,12 @@ impl> Visibility { } } +impl + Copy> Visibility { + pub fn min(self, vis: Visibility, tcx: TyCtxt<'_>) -> Visibility { + if self.is_at_least(vis, tcx) { vis } else { self } + } +} + impl Visibility { pub fn expect_local(self) -> Visibility { self.map_id(|id| id.expect_local()) diff --git a/compiler/rustc_resolve/src/diagnostics.rs b/compiler/rustc_resolve/src/diagnostics.rs index d704280f3fa23..6f3f2563091fc 100644 --- a/compiler/rustc_resolve/src/diagnostics.rs +++ b/compiler/rustc_resolve/src/diagnostics.rs @@ -24,7 +24,7 @@ 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, + ABSOLUTE_PATHS_NOT_STARTING_WITH_CRATE, AMBIGUOUS_GLOB_IMPORTS, AMBIGUOUS_IMPORT_VISIBILITIES, MACRO_EXPANDED_MACRO_EXPORTS_ACCESSED_BY_ABSOLUTE_PATHS, }; use rustc_session::utils::was_invoked_from_cargo; @@ -152,7 +152,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { DeclKind::Def(_) => CRATE_NODE_ID, }; self.lint_buffer.buffer_lint( - AMBIGUOUS_GLOB_IMPORTS, + if ambiguity_error.ambig_vis.is_some() { + AMBIGUOUS_IMPORT_VISIBILITIES + } else { + AMBIGUOUS_GLOB_IMPORTS + }, node_id, diag.ident.span, diag, @@ -1996,7 +2000,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) @@ -2059,8 +2064,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { let (b1_note, b1_help_msgs) = could_refer_to(b1, scope1, ""); let (b2_note, b2_help_msgs) = could_refer_to(b2, scope2, " also"); + 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, + ambig_vis, kind: kind.descr(), b1_note, b1_help_msgs, diff --git a/compiler/rustc_resolve/src/errors.rs b/compiler/rustc_resolve/src/errors.rs index af58d88ec35f2..c5c60c7805b54 100644 --- a/compiler/rustc_resolve/src/errors.rs +++ b/compiler/rustc_resolve/src/errors.rs @@ -1450,6 +1450,7 @@ pub(crate) struct UnknownDiagnosticAttributeTypoSugg { // FIXME: Make this properly translatable. pub(crate) struct Ambiguity { pub ident: Ident, + pub ambig_vis: Option, pub kind: &'static str, pub b1_note: Spanned, pub b1_help_msgs: Vec, @@ -1459,8 +1460,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); for help_msg in self.b1_help_msgs { diff --git a/compiler/rustc_resolve/src/ident.rs b/compiler/rustc_resolve/src/ident.rs index f47bb28d3fa1f..0d11f587e22d5 100644 --- a/compiler/rustc_resolve/src/ident.rs +++ b/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; @@ -464,9 +465,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. @@ -478,6 +482,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)); @@ -760,12 +765,22 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { decl: Decl<'ra>, scope: Scope<'ra>, innermost_results: &[(Decl<'ra>, Scope<'ra>)], + import_vis: Option, ) -> 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. @@ -843,12 +858,13 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { } else { self.ambiguity_errors.push(AmbiguityError { kind, + ambig_vis, ident: orig_ident, b1: innermost_decl, b2: decl, scope1: innermost_scope, scope2: scope, - warning: false, + warning: ambig_vis.is_some(), }); return true; } diff --git a/compiler/rustc_resolve/src/imports.rs b/compiler/rustc_resolve/src/imports.rs index 68f042c6e041d..83ea33e29a821 100644 --- a/compiler/rustc_resolve/src/imports.rs +++ b/compiler/rustc_resolve/src/imports.rs @@ -1150,7 +1150,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), ); diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index 311efb3e881b1..8baee778bfe17 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -913,6 +913,7 @@ impl AmbiguityKind { struct AmbiguityError<'ra> { kind: AmbiguityKind, + ambig_vis: Option<(Visibility, Visibility)>, ident: Ident, b1: Decl<'ra>, b2: Decl<'ra>, @@ -2058,6 +2059,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, @@ -2524,6 +2526,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 and import's final segment. + import_vis: Option = None, } impl Finalize { diff --git a/tests/ui/imports/ambiguous-import-visibility-macro.rs b/tests/ui/imports/ambiguous-import-visibility-macro.rs new file mode 100644 index 0000000000000..e1861cc5d4e0a --- /dev/null +++ b/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() {} diff --git a/tests/ui/imports/ambiguous-import-visibility-macro.stderr b/tests/ui/imports/ambiguous-import-visibility-macro.stderr new file mode 100644 index 0000000000000..ed6eb6f893af2 --- /dev/null +++ b/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 + = 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 + diff --git a/tests/ui/imports/ambiguous-import-visibility-module.rs b/tests/ui/imports/ambiguous-import-visibility-module.rs new file mode 100644 index 0000000000000..35c6da8b21a4f --- /dev/null +++ b/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 {}; +} diff --git a/tests/ui/imports/ambiguous-import-visibility-module.stderr b/tests/ui/imports/ambiguous-import-visibility-module.stderr new file mode 100644 index 0000000000000..a97070c20a62d --- /dev/null +++ b/tests/ui/imports/ambiguous-import-visibility-module.stderr @@ -0,0 +1,29 @@ +warning: ambiguous import visibility: pub or pub(in crate::reexport) + --> $DIR/ambiguous-import-visibility-module.rs:18:13 + | +LL | pub use S as Z; + | ^ + | + = 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 + = 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: `S` could refer to the struct imported here + --> $DIR/ambiguous-import-visibility-module.rs:11:17 + | +LL | use m::S; + | ^^^^ +... +LL | mac!(); + | ------ in this macro invocation + = help: use `self::S` to refer to this struct unambiguously +note: `S` could also refer to the struct imported here + --> $DIR/ambiguous-import-visibility-module.rs:15:13 + | +LL | pub use m::*; + | ^^^^ + = help: use `self::S` to refer to this struct unambiguously + = note: `#[warn(ambiguous_import_visibilities)]` (part of `#[warn(future_incompatible)]`) on by default + = note: this warning originates in the macro `mac` (in Nightly builds, run with -Z macro-backtrace for more info) + +warning: 1 warning emitted + diff --git a/tests/ui/imports/ambiguous-import-visibility.rs b/tests/ui/imports/ambiguous-import-visibility.rs new file mode 100644 index 0000000000000..4cb8b763fbc9d --- /dev/null +++ b/tests/ui/imports/ambiguous-import-visibility.rs @@ -0,0 +1,14 @@ +//@ check-pass +//@ edition:2018 +//@ proc-macro: same-res-ambigious-extern-macro.rs + +#[macro_use] // this imports the `RustEmbed` macro with `pub(crate)` visibility +extern crate same_res_ambigious_extern_macro; +// this imports the same `RustEmbed` macro with `pub` visibility +pub use same_res_ambigious_extern_macro::*; + +pub trait RustEmbed {} + +pub use RustEmbed as Embed; //~ WARN ambiguous import visibility + //~| WARN this was previously accepted +fn main() {} diff --git a/tests/ui/imports/ambiguous-import-visibility.stderr b/tests/ui/imports/ambiguous-import-visibility.stderr new file mode 100644 index 0000000000000..30cddca4697d5 --- /dev/null +++ b/tests/ui/imports/ambiguous-import-visibility.stderr @@ -0,0 +1,25 @@ +warning: ambiguous import visibility: pub(crate) or pub + --> $DIR/ambiguous-import-visibility.rs:12: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 + = 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.rs:8:9 + | +LL | pub use same_res_ambigious_extern_macro::*; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + = 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.rs:5: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 + +warning: 1 warning emitted +