From a9a2fe70d0c4e8f21efde8b0cbc1a407736f8903 Mon Sep 17 00:00:00 2001 From: Vadim Petrochenkov Date: Fri, 24 Oct 2025 15:29:33 +0300 Subject: [PATCH] resolve: Report more early resolution ambiguities for imports The new ambiguities are reported when the import's visibility is ambiguous and may depend on the resolution/expansion order. --- compiler/rustc_lint_defs/src/builtin.rs | 50 ++++++++++++++++ compiler/rustc_middle/src/ty/mod.rs | 6 ++ compiler/rustc_resolve/src/diagnostics.rs | 26 +++++++-- compiler/rustc_resolve/src/errors.rs | 9 ++- compiler/rustc_resolve/src/ident.rs | 58 ++++++++++++++----- compiler/rustc_resolve/src/imports.rs | 6 +- compiler/rustc_resolve/src/lib.rs | 4 ++ .../ambiguous-import-visibility-macro.rs | 19 ++++++ .../ambiguous-import-visibility-macro.stderr | 29 ++++++++++ .../ambiguous-import-visibility-module.rs | 24 ++++++++ .../ambiguous-import-visibility-module.stderr | 28 +++++++++ .../ui/imports/ambiguous-import-visibility.rs | 14 +++++ .../ambiguous-import-visibility.stderr | 25 ++++++++ 13 files changed, 274 insertions(+), 24 deletions(-) create mode 100644 tests/ui/imports/ambiguous-import-visibility-macro.rs create mode 100644 tests/ui/imports/ambiguous-import-visibility-macro.stderr create mode 100644 tests/ui/imports/ambiguous-import-visibility-module.rs create mode 100644 tests/ui/imports/ambiguous-import-visibility-module.stderr create mode 100644 tests/ui/imports/ambiguous-import-visibility.rs create mode 100644 tests/ui/imports/ambiguous-import-visibility.stderr diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs index 2f0333160f52b..8d5f1a26e2959 100644 --- a/compiler/rustc_lint_defs/src/builtin.rs +++ b/compiler/rustc_lint_defs/src/builtin.rs @@ -21,6 +21,7 @@ declare_lint_pass! { AMBIGUOUS_ASSOCIATED_ITEMS, AMBIGUOUS_GLOB_IMPORTS, AMBIGUOUS_GLOB_REEXPORTS, + AMBIGUOUS_IMPORT_VISIBILITIES, ARITHMETIC_OVERFLOW, ASM_SUB_REGISTER, BAD_ASM_STYLE, @@ -4488,6 +4489,55 @@ 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: FutureIncompatibilityReason::FutureReleaseError, + reference: "issue #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 d3e0fbb955c4d..d50a928562524 100644 --- a/compiler/rustc_middle/src/ty/mod.rs +++ b/compiler/rustc_middle/src/ty/mod.rs @@ -380,6 +380,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 33c111708e366..9561c44d2afa5 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; @@ -148,12 +148,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { let diag = self.ambiguity_diagnostic(ambiguity_error); if ambiguity_error.warning { - let NameBindingKind::Import { import, .. } = ambiguity_error.b1.0.kind else { - unreachable!() + let node_id = match ambiguity_error.b1.0.kind { + NameBindingKind::Import { import, .. } => import.root_id, + NameBindingKind::Res(_) => CRATE_NODE_ID, }; self.lint_buffer.buffer_lint( - AMBIGUOUS_GLOB_IMPORTS, - import.root_id, + if ambiguity_error.ambig_vis.is_some() { + AMBIGUOUS_IMPORT_VISIBILITIES + } else { + AMBIGUOUS_GLOB_IMPORTS + }, + node_id, diag.ident.span, diag, ); @@ -1995,7 +2000,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { } fn ambiguity_diagnostic(&self, ambiguity_error: &AmbiguityError<'ra>) -> errors::Ambiguity { - let AmbiguityError { kind, ident, b1, b2, misc1, misc2, .. } = *ambiguity_error; + let AmbiguityError { kind, ambig_vis, ident, b1, b2, misc1, misc2, .. } = *ambiguity_error; let extern_prelude_ambiguity = || { self.extern_prelude.get(&Macros20NormalizedIdent::new(ident)).is_some_and(|entry| { entry.item_binding.map(|(b, _)| b) == Some(b1) @@ -2051,8 +2056,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { let (b1_note, b1_help_msgs) = could_refer_to(b1, misc1, ""); let (b2_note, b2_help_msgs) = could_refer_to(b2, misc2, " 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 f59b5a0aad9a7..fa7c4aad4bc05 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; @@ -469,9 +470,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(binding)); - } + let import_vis = match finalize { + None | Some(Finalize { stage: Stage::Late, .. }) => { + return ControlFlow::Break(Ok(binding)); + } + Some(Finalize { import_vis, .. }) => import_vis, + }; if let Some((innermost_binding, innermost_flags)) = innermost_result { // Found another solution, if the first one was "weak", report an error. @@ -482,6 +486,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { innermost_binding, flags, innermost_flags, + import_vis, extern_prelude_item_binding, extern_prelude_flag_binding, ) { @@ -730,11 +735,13 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { innermost_binding: NameBinding<'ra>, flags: Flags, innermost_flags: Flags, + import_vis: Option, extern_prelude_item_binding: Option>, extern_prelude_flag_binding: Option>, ) -> bool { let (res, innermost_res) = (binding.res(), innermost_binding.res()); - if res == innermost_res { + let ambig_vis = self.ambig_vis(import_vis, binding, innermost_binding); + if res == innermost_res && ambig_vis.is_none() { return false; } @@ -793,10 +800,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { }; self.ambiguity_errors.push(AmbiguityError { kind, + ambig_vis, ident: orig_ident, b1: innermost_binding, b2: binding, - warning: false, + warning: ambig_vis.is_some(), misc1: misc(innermost_flags), misc2: misc(flags), }); @@ -1207,17 +1215,20 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { && shadowing == Shadowing::Restricted && finalize.stage == Stage::Early && binding.expansion != LocalExpnId::ROOT - && binding.res() != shadowed_glob.res() { - self.ambiguity_errors.push(AmbiguityError { - kind: AmbiguityKind::GlobVsExpanded, - ident, - b1: binding, - b2: shadowed_glob, - warning: false, - misc1: AmbiguityErrorMisc::None, - misc2: AmbiguityErrorMisc::None, - }); + let ambig_vis = self.ambig_vis(finalize.import_vis, shadowed_glob, binding); + if shadowed_glob.res() != binding.res() || ambig_vis.is_some() { + self.ambiguity_errors.push(AmbiguityError { + kind: AmbiguityKind::GlobVsExpanded, + ambig_vis, + ident, + b1: binding, + b2: shadowed_glob, + warning: ambig_vis.is_some(), + misc1: AmbiguityErrorMisc::None, + misc2: AmbiguityErrorMisc::None, + }); + } } if shadowing == Shadowing::Unrestricted @@ -1324,6 +1335,23 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { false } + fn ambig_vis( + &self, + import_vis: Option, + binding: NameBinding<'ra>, + innermost_binding: NameBinding<'ra>, + ) -> Option<(Visibility, Visibility)> { + match import_vis { + Some(import_vis) if binding.res() == innermost_binding.res() => { + let min = + |b: NameBinding<'_>| b.vis.min(import_vis.to_def_id(), self.tcx).expect_local(); + let (min1, min2) = (min(binding), min(innermost_binding)); + (min1 != min2).then_some((min1, min2)) + } + _ => None, + } + } + /// Validate a local resolution (from ribs). #[instrument(level = "debug", skip(self, all_ribs))] fn validate_res_from_ribs( diff --git a/compiler/rustc_resolve/src/imports.rs b/compiler/rustc_resolve/src/imports.rs index f844e7b9cc12f..daca1364d5cf2 100644 --- a/compiler/rustc_resolve/src/imports.rs +++ b/compiler/rustc_resolve/src/imports.rs @@ -1136,7 +1136,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().binding(), Some(import), ); diff --git a/compiler/rustc_resolve/src/lib.rs b/compiler/rustc_resolve/src/lib.rs index ed47f3124f939..c5c310a985622 100644 --- a/compiler/rustc_resolve/src/lib.rs +++ b/compiler/rustc_resolve/src/lib.rs @@ -904,6 +904,7 @@ enum AmbiguityErrorMisc { struct AmbiguityError<'ra> { kind: AmbiguityKind, + ambig_vis: Option<(Visibility, Visibility)>, ident: Ident, b1: NameBinding<'ra>, b2: NameBinding<'ra>, @@ -2032,6 +2033,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> { if let Some((b2, kind)) = used_binding.ambiguity { let ambiguity_error = AmbiguityError { kind, + ambig_vis: None, ident, b1: used_binding, b2, @@ -2502,6 +2504,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..9a3adbde562ad --- /dev/null +++ b/tests/ui/imports/ambiguous-import-visibility-module.stderr @@ -0,0 +1,28 @@ +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 name from a glob import and a macro-expanded name in the same module 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 +note: `S` could also refer to the struct imported here + --> $DIR/ambiguous-import-visibility-module.rs:15:13 + | +LL | pub use m::*; + | ^^^^ + = help: consider adding an explicit import of `S` to disambiguate + = 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 +