Rollup merge of #117009 - fmease:diag-disambig-sugg-crate, r=b-naber

On unresolved imports, suggest a disambiguated path if necessary to avoid collision with local items

Fixes #116970.

Author: matthiaskrgr

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<ast::PathSegment>,
         filter_fn: FilterFn,
     ) -> Vec<ImportSuggestion>
     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,
                     ));
                 }
@@ -2554,7 +2584,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

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

tests/ui/unresolved/auxiliary/library.rs

@@ -0,0 +1 @@
+pub struct SomeUsefulType;

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() {}

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`.

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`

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`

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`.