Skip to content

Commit c0393cf

Browse files
petrochenkovpetrochenkov
committed
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.
1 parent f60a0f1 commit c0393cf

File tree

13 files changed

+254
-12
lines changed

13 files changed

+254
-12
lines changed

‎compiler/rustc_lint_defs/src/builtin.rs‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@ declare_lint_pass! {
2020
AMBIGUOUS_GLOB_IMPORTED_TRAITS,
2121
AMBIGUOUS_GLOB_IMPORTS,
2222
AMBIGUOUS_GLOB_REEXPORTS,
23+
AMBIGUOUS_IMPORT_VISIBILITIES,
2324
AMBIGUOUS_PANIC_IMPORTS,
2425
ARITHMETIC_OVERFLOW,
2526
ASM_SUB_REGISTER,
@@ -4564,6 +4565,55 @@ declare_lint! {
45644565
};
45654566
}
45664567

4568+
declare_lint! {
4569+
/// The `ambiguous_import_visibilities` lint detects imports that should report ambiguity
4570+
/// errors, but previously didn't do that due to rustc bugs.
4571+
///
4572+
/// ### Example
4573+
///
4574+
/// ```rust,compile_fail
4575+
/// #![deny(unknown_lints)]
4576+
/// #![deny(ambiguous_import_visibilities)]
4577+
/// mod reexport {
4578+
/// mod m {
4579+
/// pub struct S {}
4580+
/// }
4581+
///
4582+
/// macro_rules! mac {
4583+
/// () => { use m::S; }
4584+
/// }
4585+
///
4586+
/// pub use m::*;
4587+
/// mac!();
4588+
///
4589+
/// pub use S as Z; // ambiguous visibility
4590+
/// }
4591+
///
4592+
/// fn main() {
4593+
/// reexport::Z {};
4594+
/// }
4595+
/// ```
4596+
///
4597+
/// {{produces}}
4598+
///
4599+
/// ### Explanation
4600+
///
4601+
/// Previous versions of Rust compile it successfully because it
4602+
/// fetched the glob import's visibility for `pub use S as Z` import, and ignored the private
4603+
/// `use m::S` import that appeared later.
4604+
///
4605+
/// This is a [future-incompatible] lint to transition this to a
4606+
/// hard error in the future.
4607+
///
4608+
/// [future-incompatible]: ../index.md#future-incompatible-lints
4609+
pub AMBIGUOUS_IMPORT_VISIBILITIES,
4610+
Warn,
4611+
"detects certain glob imports that require reporting an ambiguity error",
4612+
@future_incompatible = FutureIncompatibleInfo {
4613+
reason: fcw!(FutureReleaseError #149145),
4614+
};
4615+
}
4616+
45674617
declare_lint! {
45684618
/// The `refining_impl_trait_reachable` lint detects `impl Trait` return
45694619
/// types in method signatures that are refined by a publically reachable

‎compiler/rustc_middle/src/ty/mod.rs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -408,6 +408,12 @@ impl<Id: Into<DefId>> Visibility<Id> {
408408
}
409409
}
410410

411+
impl<Id: Into<DefId> + Copy> Visibility<Id> {
412+
pub fn min(self, vis: Visibility<Id>, tcx: TyCtxt<'_>) -> Visibility<Id> {
413+
if self.is_at_least(vis, tcx) { vis } else { self }
414+
}
415+
}
416+
411417
impl Visibility<DefId> {
412418
pub fn expect_local(self) -> Visibility {
413419
self.map_id(|id| id.expect_local())

‎compiler/rustc_resolve/src/diagnostics.rs‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,8 +24,8 @@ use rustc_middle::ty::TyCtxt;
2424
use rustc_session::Session;
2525
use rustc_session::lint::BuiltinLintDiag;
2626
use rustc_session::lint::builtin::{
27-
ABSOLUTE_PATHS_NOT_STARTING_WITH_CRATE, AMBIGUOUS_GLOB_IMPORTS, AMBIGUOUS_PANIC_IMPORTS,
28-
MACRO_EXPANDED_MACRO_EXPORTS_ACCESSED_BY_ABSOLUTE_PATHS,
27+
ABSOLUTE_PATHS_NOT_STARTING_WITH_CRATE, AMBIGUOUS_GLOB_IMPORTS, AMBIGUOUS_IMPORT_VISIBILITIES,
28+
AMBIGUOUS_PANIC_IMPORTS, MACRO_EXPANDED_MACRO_EXPORTS_ACCESSED_BY_ABSOLUTE_PATHS,
2929
};
3030
use rustc_session::utils::was_invoked_from_cargo;
3131
use rustc_span::edit_distance::find_best_match_for_name;
@@ -145,6 +145,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
145145
};
146146

147147
let lint = match ambiguity_warning {
148+
_ if ambiguity_error.ambig_vis.is_some() => AMBIGUOUS_IMPORT_VISIBILITIES,
148149
AmbiguityWarning::GlobImport => AMBIGUOUS_GLOB_IMPORTS,
149150
AmbiguityWarning::PanicImport => AMBIGUOUS_PANIC_IMPORTS,
150151
};
@@ -1995,7 +1996,8 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
19951996
}
19961997

19971998
fn ambiguity_diagnostic(&self, ambiguity_error: &AmbiguityError<'ra>) -> errors::Ambiguity {
1998-
let AmbiguityError { kind, ident, b1, b2, scope1, scope2, .. } = *ambiguity_error;
1999+
let AmbiguityError { kind, ambig_vis, ident, b1, b2, scope1, scope2, .. } =
2000+
*ambiguity_error;
19992001
let extern_prelude_ambiguity = || {
20002002
// Note: b1 may come from a module scope, as an extern crate item in module.
20012003
matches!(scope2, Scope::ExternPreludeFlags)
@@ -2074,9 +2076,18 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
20742076
None
20752077
};
20762078

2079+
let ambig_vis = ambig_vis.map(|(vis1, vis2)| {
2080+
format!(
2081+
"{} or {}",
2082+
vis1.to_string(CRATE_DEF_ID, self.tcx),
2083+
vis2.to_string(CRATE_DEF_ID, self.tcx)
2084+
)
2085+
});
2086+
20772087
errors::Ambiguity {
20782088
ident,
20792089
help,
2090+
ambig_vis,
20802091
kind: kind.descr(),
20812092
b1_note,
20822093
b1_help_msgs,

‎compiler/rustc_resolve/src/errors.rs‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1463,6 +1463,7 @@ pub(crate) struct UnknownDiagnosticAttributeTypoSugg {
14631463
// FIXME: Make this properly translatable.
14641464
pub(crate) struct Ambiguity {
14651465
pub ident: Ident,
1466+
pub ambig_vis: Option<String>,
14661467
pub kind: &'static str,
14671468
pub help: Option<&'static [&'static str]>,
14681469
pub b1_note: Spanned<String>,
@@ -1473,8 +1474,12 @@ pub(crate) struct Ambiguity {
14731474

14741475
impl Ambiguity {
14751476
fn decorate<'a>(self, diag: &mut Diag<'a, impl EmissionGuarantee>) {
1476-
diag.primary_message(format!("`{}` is ambiguous", self.ident));
1477-
diag.span_label(self.ident.span, "ambiguous name");
1477+
if let Some(ambig_vis) = self.ambig_vis {
1478+
diag.primary_message(format!("ambiguous import visibility: {ambig_vis}"));
1479+
} else {
1480+
diag.primary_message(format!("`{}` is ambiguous", self.ident));
1481+
diag.span_label(self.ident.span, "ambiguous name");
1482+
}
14781483
diag.note(format!("ambiguous because of {}", self.kind));
14791484
diag.span_note(self.b1_note.span, self.b1_note.node);
14801485
if let Some(help) = self.help {

‎compiler/rustc_resolve/src/ident.rs‎

Lines changed: 28 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ use Namespace::*;
55
use rustc_ast::{self as ast, NodeId};
66
use rustc_errors::ErrorGuaranteed;
77
use rustc_hir::def::{DefKind, MacroKinds, Namespace, NonMacroAttrKind, PartialRes, PerNS};
8+
use rustc_middle::ty::Visibility;
89
use rustc_middle::{bug, span_bug};
910
use rustc_session::lint::builtin::PROC_MACRO_DERIVE_RESOLUTION_FALLBACK;
1011
use rustc_session::parse::feature_err;
@@ -485,9 +486,12 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
485486
// We do not need to report them if we are either in speculative resolution,
486487
// or in late resolution when everything is already imported and expanded
487488
// and no ambiguities exist.
488-
if matches!(finalize, None | Some(Finalize { stage: Stage::Late, .. })) {
489-
return ControlFlow::Break(Ok(decl));
490-
}
489+
let import_vis = match finalize {
490+
None | Some(Finalize { stage: Stage::Late, .. }) => {
491+
return ControlFlow::Break(Ok(decl));
492+
}
493+
Some(Finalize { import_vis, .. }) => import_vis,
494+
};
491495

492496
if let Some(&(innermost_decl, _)) = innermost_results.first() {
493497
// Found another solution, if the first one was "weak", report an error.
@@ -500,6 +504,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
500504
decl,
501505
scope,
502506
&innermost_results,
507+
import_vis,
503508
) {
504509
// No need to search for more potential ambiguities, one is enough.
505510
return ControlFlow::Break(Ok(innermost_decl));
@@ -779,12 +784,22 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
779784
decl: Decl<'ra>,
780785
scope: Scope<'ra>,
781786
innermost_results: &[(Decl<'ra>, Scope<'ra>)],
787+
import_vis: Option<Visibility>,
782788
) -> bool {
783789
let (innermost_decl, innermost_scope) = innermost_results[0];
784790
let (res, innermost_res) = (decl.res(), innermost_decl.res());
785-
if res == innermost_res {
791+
let ambig_vis = if res != innermost_res {
792+
None
793+
} else if let Some(import_vis) = import_vis
794+
&& let min =
795+
(|d: Decl<'_>| d.vis().min(import_vis.to_def_id(), self.tcx).expect_local())
796+
&& let (min1, min2) = (min(decl), min(innermost_decl))
797+
&& min1 != min2
798+
{
799+
Some((min1, min2))
800+
} else {
786801
return false;
787-
}
802+
};
788803

789804
// FIXME: Use `scope` instead of `res` to detect built-in attrs and derive helpers,
790805
// it will exclude imports, make slightly more code legal, and will require lang approval.
@@ -871,10 +886,17 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
871886
|| (self.is_specific_builtin_macro(res, sym::core_panic)
872887
&& self.is_specific_builtin_macro(innermost_res, sym::std_panic)));
873888

874-
let warning = is_issue_147319_hack.then_some(AmbiguityWarning::PanicImport);
889+
let warning = if ambig_vis.is_some() {
890+
Some(AmbiguityWarning::GlobImport)
891+
} else if is_issue_147319_hack {
892+
Some(AmbiguityWarning::PanicImport)
893+
} else {
894+
None
895+
};
875896

876897
self.ambiguity_errors.push(AmbiguityError {
877898
kind,
899+
ambig_vis,
878900
ident: ident.orig(orig_ident_span),
879901
b1: innermost_decl,
880902
b2: decl,

‎compiler/rustc_resolve/src/imports.rs‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1189,7 +1189,11 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
11891189
ident,
11901190
ns,
11911191
&import.parent_scope,
1192-
Some(Finalize { report_private: false, ..finalize }),
1192+
Some(Finalize {
1193+
report_private: false,
1194+
import_vis: Some(import.vis),
1195+
..finalize
1196+
}),
11931197
bindings[ns].get().decl(),
11941198
Some(import),
11951199
);

‎compiler/rustc_resolve/src/lib.rs‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -969,6 +969,7 @@ enum AmbiguityWarning {
969969

970970
struct AmbiguityError<'ra> {
971971
kind: AmbiguityKind,
972+
ambig_vis: Option<(Visibility, Visibility)>,
972973
ident: Ident,
973974
b1: Decl<'ra>,
974975
b2: Decl<'ra>,
@@ -2087,6 +2088,7 @@ impl<'ra, 'tcx> Resolver<'ra, 'tcx> {
20872088
if let Some(b2) = used_decl.ambiguity.get() {
20882089
let ambiguity_error = AmbiguityError {
20892090
kind: AmbiguityKind::GlobVsGlob,
2091+
ambig_vis: None,
20902092
ident,
20912093
b1: used_decl,
20922094
b2,
@@ -2556,6 +2558,8 @@ struct Finalize {
25562558
used: Used = Used::Other,
25572559
/// Finalizing early or late resolution.
25582560
stage: Stage = Stage::Early,
2561+
/// Nominal visibility of the import item, in case we are resolving an import's final segment.
2562+
import_vis: Option<Visibility> = None,
25592563
}
25602564

25612565
impl Finalize {

‎tests/ui/imports/ambiguous-import-visibility-macro.rs‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,19 @@
1+
//@ check-pass
2+
//@ edition:2018
3+
//@ proc-macro: same-res-ambigious-extern-macro.rs
4+
5+
macro_rules! globbing{
6+
() => {
7+
pub use same_res_ambigious_extern_macro::*;
8+
}
9+
}
10+
11+
#[macro_use] // this imports the `RustEmbed` macro with `pub(crate)` visibility
12+
extern crate same_res_ambigious_extern_macro;
13+
globbing! {} // this imports the same `RustEmbed` macro with `pub` visibility
14+
15+
pub trait RustEmbed {}
16+
17+
pub use RustEmbed as Embed; //~ WARN ambiguous import visibility
18+
//~| WARN this was previously accepted
19+
fn main() {}

‎tests/ui/imports/ambiguous-import-visibility-macro.stderr‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
warning: ambiguous import visibility: pub(crate) or pub
2+
--> $DIR/ambiguous-import-visibility-macro.rs:17:9
3+
|
4+
LL | pub use RustEmbed as Embed;
5+
| ^^^^^^^^^
6+
|
7+
= warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
8+
= note: for more information, see issue #149145 <https://github.com/rust-lang/rust/issues/149145>
9+
= note: ambiguous because of a conflict between a name from a glob import and an outer scope during import or macro resolution
10+
note: `RustEmbed` could refer to the derive macro imported here
11+
--> $DIR/ambiguous-import-visibility-macro.rs:7:17
12+
|
13+
LL | pub use same_res_ambigious_extern_macro::*;
14+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
15+
...
16+
LL | globbing! {} // this imports the same `RustEmbed` macro with `pub` visibility
17+
| ------------ in this macro invocation
18+
= help: consider adding an explicit import of `RustEmbed` to disambiguate
19+
= help: or use `crate::RustEmbed` to refer to this derive macro unambiguously
20+
note: `RustEmbed` could also refer to the derive macro imported here
21+
--> $DIR/ambiguous-import-visibility-macro.rs:11:1
22+
|
23+
LL | #[macro_use] // this imports the `RustEmbed` macro with `pub(crate)` visibility
24+
| ^^^^^^^^^^^^
25+
= note: `#[warn(ambiguous_import_visibilities)]` (part of `#[warn(future_incompatible)]`) on by default
26+
= note: this warning originates in the macro `globbing` (in Nightly builds, run with -Z macro-backtrace for more info)
27+
28+
warning: 1 warning emitted
29+

‎tests/ui/imports/ambiguous-import-visibility-module.rs‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
//@ check-pass
2+
//@ edition:2018..
3+
4+
mod reexport {
5+
mod m {
6+
pub struct S {}
7+
}
8+
9+
macro_rules! mac {
10+
() => {
11+
use m::S;
12+
};
13+
}
14+
15+
pub use m::*;
16+
mac!();
17+
18+
pub use S as Z; //~ WARN ambiguous import visibility
19+
//~| WARN this was previously accepted
20+
}
21+
22+
fn main() {
23+
reexport::Z {};
24+
}

0 commit comments

Comments
 (0)