From 4aaf8e03e1a1e2a53e95196569213baafc421b2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Le=C3=B3n=20Orell=20Valerian=20Liehr?= Date: Sat, 21 Oct 2023 06:04:41 +0200 Subject: [PATCH] on unresolved import disambiguate suggested path if it would collide --- compiler/rustc_resolve/src/diagnostics.rs | 58 ++++++++++++++----- tests/ui/imports/issue-56125.stderr | 12 ++-- tests/ui/unresolved/auxiliary/library.rs | 1 + ...ved-import-avoid-suggesting-global-path.rs | 31 ++++++++++ ...import-avoid-suggesting-global-path.stderr | 25 ++++++++ ...ort-suggest-disambiguated-crate-name.fixed | 19 ++++++ ...import-suggest-disambiguated-crate-name.rs | 19 ++++++ ...rt-suggest-disambiguated-crate-name.stderr | 14 +++++ 8 files changed, 159 insertions(+), 20 deletions(-) create mode 100644 tests/ui/unresolved/auxiliary/library.rs create mode 100644 tests/ui/unresolved/unresolved-import-avoid-suggesting-global-path.rs create mode 100644 tests/ui/unresolved/unresolved-import-avoid-suggesting-global-path.stderr create mode 100644 tests/ui/unresolved/unresolved-import-suggest-disambiguated-crate-name.fixed create mode 100644 tests/ui/unresolved/unresolved-import-suggest-disambiguated-crate-name.rs create mode 100644 tests/ui/unresolved/unresolved-import-suggest-disambiguated-crate-name.stderr diff --git a/compiler/rustc_resolve/src/diagnostics.rs b/compiler/rustc_resolve/src/diagnostics.rs index 925ee615b095b..3e43b6bfdaec8 100644 --- a/compiler/rustc_resolve/src/diagnostics.rs +++ b/compiler/rustc_resolve/src/diagnostics.rs @@ -25,7 +25,7 @@ use rustc_span::hygiene::MacroKind; use rustc_span::source_map::SourceMap; use rustc_span::symbol::{kw, sym, Ident, Symbol}; use rustc_span::{BytePos, Span, SyntaxContext}; -use thin_vec::ThinVec; +use thin_vec::{thin_vec, ThinVec}; use crate::errors::{ AddedMacroUse, ChangeImportBinding, ChangeImportBindingSuggestion, ConsiderAddingADerive, @@ -1147,7 +1147,7 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { namespace: Namespace, parent_scope: &ParentScope<'a>, start_module: Module<'a>, - crate_name: Ident, + crate_path: ThinVec, filter_fn: FilterFn, ) -> Vec where @@ -1163,8 +1163,6 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { Some(x) => Some(x), } { let in_module_is_extern = !in_module.def_id().is_local(); - // We have to visit module children in deterministic order to avoid - // instabilities in reported imports (#43552). in_module.for_each_child(self, |this, ident, ns, name_binding| { // avoid non-importable candidates if !name_binding.is_importable() { @@ -1214,12 +1212,14 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { let res = name_binding.res(); if filter_fn(res) { // create the path - let mut segms = path_segments.clone(); - if lookup_ident.span.at_least_rust_2018() { + let mut segms = if lookup_ident.span.at_least_rust_2018() { // crate-local absolute paths start with `crate::` in edition 2018 // FIXME: may also be stabilized for Rust 2015 (Issues #45477, #44660) - segms.insert(0, ast::PathSegment::from_ident(crate_name)); - } + crate_path.clone() + } else { + ThinVec::new() + }; + segms.append(&mut path_segments.clone()); segms.push(ast::PathSegment::from_ident(ident)); let path = Path { span: name_binding.span, segments: segms, tokens: None }; @@ -1318,18 +1318,18 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { where FilterFn: Fn(Res) -> bool, { + let crate_path = thin_vec![ast::PathSegment::from_ident(Ident::with_dummy_span(kw::Crate))]; let mut suggestions = self.lookup_import_candidates_from_module( lookup_ident, namespace, parent_scope, self.graph_root, - Ident::with_dummy_span(kw::Crate), + crate_path, &filter_fn, ); if lookup_ident.span.at_least_rust_2018() { - let extern_prelude_names = self.extern_prelude.clone(); - for (ident, _) in extern_prelude_names.into_iter() { + for ident in self.extern_prelude.clone().into_keys() { if ident.span.from_expansion() { // Idents are adjusted to the root context before being // resolved in the extern prelude, so reporting this to the @@ -1340,13 +1340,43 @@ impl<'a, 'tcx> Resolver<'a, 'tcx> { } let crate_id = self.crate_loader(|c| c.maybe_process_path_extern(ident.name)); if let Some(crate_id) = crate_id { - let crate_root = self.expect_module(crate_id.as_def_id()); + let crate_def_id = crate_id.as_def_id(); + let crate_root = self.expect_module(crate_def_id); + + // Check if there's already an item in scope with the same name as the crate. + // If so, we have to disambiguate the potential import suggestions by making + // the paths *global* (i.e., by prefixing them with `::`). + let needs_disambiguation = + self.resolutions(parent_scope.module).borrow().iter().any( + |(key, name_resolution)| { + if key.ns == TypeNS + && key.ident == ident + && let Some(binding) = name_resolution.borrow().binding + { + match binding.res() { + // No disambiguation needed if the identically named item we + // found in scope actually refers to the crate in question. + Res::Def(_, def_id) => def_id != crate_def_id, + Res::PrimTy(_) => true, + _ => false, + } + } else { + false + } + }, + ); + let mut crate_path = ThinVec::new(); + if needs_disambiguation { + crate_path.push(ast::PathSegment::path_root(rustc_span::DUMMY_SP)); + } + crate_path.push(ast::PathSegment::from_ident(ident)); + suggestions.extend(self.lookup_import_candidates_from_module( lookup_ident, namespace, parent_scope, crate_root, - ident, + crate_path, &filter_fn, )); } @@ -2541,7 +2571,7 @@ fn show_candidates( candidates.iter().for_each(|c| { (if c.accessible { &mut accessible_path_strings } else { &mut inaccessible_path_strings }) - .push((path_names_to_string(&c.path), c.descr, c.did, &c.note, c.via_import)) + .push((pprust::path_to_string(&c.path), c.descr, c.did, &c.note, c.via_import)) }); // we want consistent results across executions, but candidates are produced diff --git a/tests/ui/imports/issue-56125.stderr b/tests/ui/imports/issue-56125.stderr index 15477fb6f1015..d2a0f436c42d0 100644 --- a/tests/ui/imports/issue-56125.stderr +++ b/tests/ui/imports/issue-56125.stderr @@ -6,14 +6,14 @@ LL | use empty::issue_56125; | help: consider importing one of these items instead | +LL | use ::issue_56125::issue_56125; + | ~~~~~~~~~~~~~~~~~~~~~~~~~~ +LL | use ::issue_56125::last_segment::issue_56125; + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ +LL | use ::issue_56125::non_last_segment::non_last_segment::issue_56125; + | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ LL | use crate::m3::last_segment::issue_56125; | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -LL | use crate::m3::non_last_segment::non_last_segment::issue_56125; - | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -LL | use issue_56125::issue_56125; - | ~~~~~~~~~~~~~~~~~~~~~~~~ -LL | use issue_56125::last_segment::issue_56125; - | ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ and 1 other candidate error[E0659]: `issue_56125` is ambiguous diff --git a/tests/ui/unresolved/auxiliary/library.rs b/tests/ui/unresolved/auxiliary/library.rs new file mode 100644 index 0000000000000..1169ed9622523 --- /dev/null +++ b/tests/ui/unresolved/auxiliary/library.rs @@ -0,0 +1 @@ +pub struct SomeUsefulType; diff --git a/tests/ui/unresolved/unresolved-import-avoid-suggesting-global-path.rs b/tests/ui/unresolved/unresolved-import-avoid-suggesting-global-path.rs new file mode 100644 index 0000000000000..af8207aaadded --- /dev/null +++ b/tests/ui/unresolved/unresolved-import-avoid-suggesting-global-path.rs @@ -0,0 +1,31 @@ +// Test that we don't prepend `::` to paths referencing crates from the extern prelude +// when it can be avoided[^1] since it's more idiomatic to do so. +// +// [^1]: Counterexample: `unresolved-import-suggest-disambiguated-crate-name.rs` +#![feature(decl_macro)] // allows us to create items with hygienic names + +// aux-crate:library=library.rs +// edition: 2021 + +mod hygiene { + make!(); + macro make() { + // This won't conflict with the suggested *non-global* path as the syntax context differs. + mod library {} + } + + mod module {} + use module::SomeUsefulType; //~ ERROR unresolved import `module::SomeUsefulType` +} + +mod glob { + use inner::*; + mod inner { + mod library {} + } + + mod module {} + use module::SomeUsefulType; //~ ERROR unresolved import `module::SomeUsefulType` +} + +fn main() {} diff --git a/tests/ui/unresolved/unresolved-import-avoid-suggesting-global-path.stderr b/tests/ui/unresolved/unresolved-import-avoid-suggesting-global-path.stderr new file mode 100644 index 0000000000000..b0352ab675435 --- /dev/null +++ b/tests/ui/unresolved/unresolved-import-avoid-suggesting-global-path.stderr @@ -0,0 +1,25 @@ +error[E0432]: unresolved import `module::SomeUsefulType` + --> $DIR/unresolved-import-avoid-suggesting-global-path.rs:18:9 + | +LL | use module::SomeUsefulType; + | ^^^^^^^^^^^^^^^^^^^^^^ no `SomeUsefulType` in `hygiene::module` + | +help: consider importing this struct instead + | +LL | use library::SomeUsefulType; + | ~~~~~~~~~~~~~~~~~~~~~~~ + +error[E0432]: unresolved import `module::SomeUsefulType` + --> $DIR/unresolved-import-avoid-suggesting-global-path.rs:28:9 + | +LL | use module::SomeUsefulType; + | ^^^^^^^^^^^^^^^^^^^^^^ no `SomeUsefulType` in `glob::module` + | +help: consider importing this struct instead + | +LL | use library::SomeUsefulType; + | ~~~~~~~~~~~~~~~~~~~~~~~ + +error: aborting due to 2 previous errors + +For more information about this error, try `rustc --explain E0432`. diff --git a/tests/ui/unresolved/unresolved-import-suggest-disambiguated-crate-name.fixed b/tests/ui/unresolved/unresolved-import-suggest-disambiguated-crate-name.fixed new file mode 100644 index 0000000000000..2b20d3f106b9d --- /dev/null +++ b/tests/ui/unresolved/unresolved-import-suggest-disambiguated-crate-name.fixed @@ -0,0 +1,19 @@ +// Regression test for issue #116970. +// +// When we suggest importing an item from a crate found in the extern prelude and there +// happens to exist a module or type in the current scope with the same name as the crate, +// disambiguate the suggested path by making it global (i.e., by prefixing it with `::`). +// +// For context, when it can be avoided we don't prepend `::` to paths referencing crates +// from the extern prelude. See also `unresolved-import-avoid-suggesting-global-path.rs`. + +// run-rustfix + +// compile-flags: --crate-type=lib +// aux-crate:library=library.rs +// edition: 2021 + +mod library {} // this module shares the same name as the external crate! + +mod module {} +pub use ::library::SomeUsefulType; //~ ERROR unresolved import `module::SomeUsefulType` diff --git a/tests/ui/unresolved/unresolved-import-suggest-disambiguated-crate-name.rs b/tests/ui/unresolved/unresolved-import-suggest-disambiguated-crate-name.rs new file mode 100644 index 0000000000000..b810a7f52966a --- /dev/null +++ b/tests/ui/unresolved/unresolved-import-suggest-disambiguated-crate-name.rs @@ -0,0 +1,19 @@ +// Regression test for issue #116970. +// +// When we suggest importing an item from a crate found in the extern prelude and there +// happens to exist a module or type in the current scope with the same name as the crate, +// disambiguate the suggested path by making it global (i.e., by prefixing it with `::`). +// +// For context, when it can be avoided we don't prepend `::` to paths referencing crates +// from the extern prelude. See also `unresolved-import-avoid-suggesting-global-path.rs`. + +// run-rustfix + +// compile-flags: --crate-type=lib +// aux-crate:library=library.rs +// edition: 2021 + +mod library {} // this module shares the same name as the external crate! + +mod module {} +pub use module::SomeUsefulType; //~ ERROR unresolved import `module::SomeUsefulType` diff --git a/tests/ui/unresolved/unresolved-import-suggest-disambiguated-crate-name.stderr b/tests/ui/unresolved/unresolved-import-suggest-disambiguated-crate-name.stderr new file mode 100644 index 0000000000000..f139c0f3cf1e8 --- /dev/null +++ b/tests/ui/unresolved/unresolved-import-suggest-disambiguated-crate-name.stderr @@ -0,0 +1,14 @@ +error[E0432]: unresolved import `module::SomeUsefulType` + --> $DIR/unresolved-import-suggest-disambiguated-crate-name.rs:19:9 + | +LL | pub use module::SomeUsefulType; + | ^^^^^^^^^^^^^^^^^^^^^^ no `SomeUsefulType` in `module` + | +help: consider importing this struct instead + | +LL | pub use ::library::SomeUsefulType; + | ~~~~~~~~~~~~~~~~~~~~~~~~~ + +error: aborting due to previous error + +For more information about this error, try `rustc --explain E0432`.