Auto merge of #53427 - eddyb:uniform-paths-diagnostics, r=varkor

rustc_resolve: overhaul `#![feature(uniform_paths)]` error reporting.

Fixes #53408 by only considering external crates to conflict within their (type/module) namespace, *not* with the value or macro namespaces, and also by adding a special-cased error for redundant `use crate_name;` imports (without actually allowing them).
Also, all canaries for a given import are grouped into one diagnostic per namespace, in order to make block-scoped ambiguities clearer.
See changed/added tests for more details.

r? @petrochenkov cc @aturon @joshtriplett

Author: bors

src/librustc_resolve/resolve_imports.rs

@@ -11,7 +11,7 @@
 use self::ImportDirectiveSubclass::*;
 
 use {AmbiguityError, CrateLint, Module, ModuleOrUniformRoot, PerNS};
-use Namespace::{self, TypeNS, MacroNS, ValueNS};
+use Namespace::{self, TypeNS, MacroNS};
 use {NameBinding, NameBindingKind, ToNameBinding, PathResult, PrivacyError};
 use Resolver;
 use {names_to_string, module_to_string};
@@ -34,6 +34,8 @@ use syntax::util::lev_distance::find_best_match_for_name;
 use syntax_pos::Span;
 
 use std::cell::{Cell, RefCell};
+use std::collections::BTreeMap;
+use std::fmt::Write;
 use std::{mem, ptr};
 
 /// Contains data for specific types of import directives.
@@ -615,6 +617,17 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
             self.finalize_resolutions_in(module);
         }
 
+        #[derive(Default)]
+        struct UniformPathsCanaryResult {
+            module_scope: Option<Span>,
+            block_scopes: Vec<Span>,
+        }
+        // Collect all tripped `uniform_paths` canaries separately.
+        let mut uniform_paths_canaries: BTreeMap<
+            (Span, NodeId),
+            (Name, PerNS<UniformPathsCanaryResult>),
+        > = BTreeMap::new();
+
         let mut errors = false;
         let mut seen_spans = FxHashSet();
         for i in 0 .. self.determined_imports.len() {
@@ -624,49 +637,37 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
             // For a `#![feature(uniform_paths)]` `use self::x as _` canary,
             // failure is ignored, while success may cause an ambiguity error.
             if import.is_uniform_paths_canary {
-                let (name, result) = match import.subclass {
-                    SingleImport { source, ref result, .. } => {
-                        let type_ns = result[TypeNS].get().ok();
-                        let value_ns = result[ValueNS].get().ok();
-                        (source.name, type_ns.or(value_ns))
-                    }
-                    _ => bug!(),
-                };
-
                 if error.is_some() {
                     continue;
                 }
 
-                let is_explicit_self =
+                let (name, result) = match import.subclass {
+                    SingleImport { source, ref result, .. } => (source.name, result),
+                    _ => bug!(),
+                };
+
+                let has_explicit_self =
                     import.module_path.len() > 0 &&
                     import.module_path[0].name == keywords::SelfValue.name();
-                let extern_crate_exists = self.extern_prelude.contains(&name);
 
-                // A successful `self::x` is ambiguous with an `x` external crate.
-                if is_explicit_self && !extern_crate_exists {
-                    continue;
-                }
+                let (prev_name, canary_results) =
+                    uniform_paths_canaries.entry((import.span, import.id))
+                        .or_insert((name, PerNS::default()));
 
-                errors = true;
+                // All the canaries with the same `id` should have the same `name`.
+                assert_eq!(*prev_name, name);
 
-                let msg = format!("import from `{}` is ambiguous", name);
-                let mut err = self.session.struct_span_err(import.span, &msg);
-                if extern_crate_exists {
-                    err.span_label(import.span,
-                        format!("could refer to external crate `::{}`", name));
-                }
-                if let Some(result) = result {
-                    if is_explicit_self {
-                        err.span_label(result.span,
-                            format!("could also refer to `self::{}`", name));
-                    } else {
-                        err.span_label(result.span,
-                            format!("shadowed by block-scoped `{}`", name));
+                self.per_ns(|_, ns| {
+                    if let Some(result) = result[ns].get().ok() {
+                        if has_explicit_self {
+                            // There should only be one `self::x` (module-scoped) canary.
+                            assert_eq!(canary_results[ns].module_scope, None);
+                            canary_results[ns].module_scope = Some(result.span);
+                        } else {
+                            canary_results[ns].block_scopes.push(result.span);
+                        }
                     }
-                }
-                err.help(&format!("write `::{0}` or `self::{0}` explicitly instead", name));
-                err.note("relative `use` paths enabled by `#![feature(uniform_paths)]`");
-                err.emit();
+                });
             } else if let Some((span, err)) = error {
                 errors = true;
 
@@ -694,6 +695,66 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
             }
         }
 
+        for ((span, _), (name, results)) in uniform_paths_canaries {
+            self.per_ns(|this, ns| {
+                let results = &results[ns];
+
+                let has_external_crate =
+                    ns == TypeNS && this.extern_prelude.contains(&name);
+
+                // An ambiguity requires more than one possible resolution.
+                let possible_resultions =
+                    (has_external_crate as usize) +
+                    (results.module_scope.is_some() as usize) +
+                    (!results.block_scopes.is_empty() as usize);
+                if possible_resultions <= 1 {
+                    return;
+                }
+
+                errors = true;
+
+                // Special-case the error when `self::x` finds its own `use x;`.
+                if has_external_crate &&
+                   results.module_scope == Some(span) &&
+                   results.block_scopes.is_empty() {
+                    let msg = format!("`{}` import is redundant", name);
+                    this.session.struct_span_err(span, &msg)
+                        .span_label(span,
+                            format!("refers to external crate `::{}`", name))
+                        .span_label(span,
+                            format!("defines `self::{}`, shadowing itself", name))
+                        .help(&format!("remove or write `::{}` explicitly instead", name))
+                        .note("relative `use` paths enabled by `#![feature(uniform_paths)]`")
+                        .emit();
+                    return;
+                }
+
+                let msg = format!("`{}` import is ambiguous", name);
+                let mut err = this.session.struct_span_err(span, &msg);
+                let mut suggestion_choices = String::new();
+                if has_external_crate {
+                    write!(suggestion_choices, "`::{}`", name);
+                    err.span_label(span,
+                        format!("can refer to external crate `::{}`", name));
+                }
+                if let Some(span) = results.module_scope {
+                    if !suggestion_choices.is_empty() {
+                        suggestion_choices.push_str(" or ");
+                    }
+                    write!(suggestion_choices, "`self::{}`", name);
+                    err.span_label(span,
+                        format!("can refer to `self::{}`", name));
+                }
+                for &span in &results.block_scopes {
+                    err.span_label(span,
+                        format!("shadowed by block-scoped `{}`", name));
+                }
+                err.help(&format!("write {} explicitly instead", suggestion_choices));
+                err.note("relative `use` paths enabled by `#![feature(uniform_paths)]`");
+                err.emit();
+            });
+        }
+
         // Report unresolved imports only if no hard error was already reported
         // to avoid generating multiple errors on the same import.
         if !errors {

src/test/run-pass/uniform-paths/basic-nested.rs

@@ -10,7 +10,7 @@
 
 // edition:2018
 
-#![feature(uniform_paths)]
+#![feature(decl_macro, uniform_paths)]
 
 // This test is similar to `basic.rs`, but nested in modules.
 
@@ -40,6 +40,11 @@ mod bar {
     // item, e.g. the automatically injected `extern crate std;`, which in
     // the Rust 2018 should no longer be visible through `crate::std`.
     pub use std::io;
+
+    // Also test that items named `std` in other namespaces don't
+    // cause ambiguity errors for the import from `std` above.
+    pub fn std() {}
+    pub macro std() {}
 }
 
 
@@ -49,4 +54,6 @@ fn main() {
     foo::local_io(());
     io::stdout();
     bar::io::stdout();
+    bar::std();
+    bar::std!();
 }

src/test/ui/rust-2018/uniform-paths/ambiguity-macros-nested.rs

@@ -16,7 +16,7 @@
 
 mod foo {
     pub use std::io;
-    //~^ ERROR import from `std` is ambiguous
+    //~^ ERROR `std` import is ambiguous
 
     macro_rules! m {
         () => {

src/test/ui/rust-2018/uniform-paths/ambiguity-macros-nested.stderr

@@ -1,13 +1,13 @@
-error: import from `std` is ambiguous
+error: `std` import is ambiguous
   --> $DIR/ambiguity-macros-nested.rs:18:13
    |
 LL |       pub use std::io;
-   |               ^^^ could refer to external crate `::std`
+   |               ^^^ can refer to external crate `::std`
 ...
 LL | /             mod std {
 LL | |                 pub struct io;
 LL | |             }
-   | |_____________- could also refer to `self::std`
+   | |_____________- can refer to `self::std`
    |
    = help: write `::std` or `self::std` explicitly instead
    = note: relative `use` paths enabled by `#![feature(uniform_paths)]`

src/test/ui/rust-2018/uniform-paths/ambiguity-macros.rs

@@ -15,7 +15,7 @@
 // This test is similar to `ambiguity.rs`, but with macros defining local items.
 
 use std::io;
-//~^ ERROR import from `std` is ambiguous
+//~^ ERROR `std` import is ambiguous
 
 macro_rules! m {
     () => {

src/test/ui/rust-2018/uniform-paths/ambiguity-macros.stderr

@@ -1,13 +1,13 @@
-error: import from `std` is ambiguous
+error: `std` import is ambiguous
   --> $DIR/ambiguity-macros.rs:17:5
    |
 LL |   use std::io;
-   |       ^^^ could refer to external crate `::std`
+   |       ^^^ can refer to external crate `::std`
 ...
 LL | /         mod std {
 LL | |             pub struct io;
 LL | |         }
-   | |_________- could also refer to `self::std`
+   | |_________- can refer to `self::std`
    |
    = help: write `::std` or `self::std` explicitly instead
    = note: relative `use` paths enabled by `#![feature(uniform_paths)]`

src/test/ui/rust-2018/uniform-paths/ambiguity-nested.rs

@@ -16,7 +16,7 @@
 
 mod foo {
     pub use std::io;
-    //~^ ERROR import from `std` is ambiguous
+    //~^ ERROR `std` import is ambiguous
 
     mod std {
         pub struct io;

src/test/ui/rust-2018/uniform-paths/ambiguity-nested.stderr

@@ -1,13 +1,13 @@
-error: import from `std` is ambiguous
+error: `std` import is ambiguous
   --> $DIR/ambiguity-nested.rs:18:13
    |
 LL |       pub use std::io;
-   |               ^^^ could refer to external crate `::std`
+   |               ^^^ can refer to external crate `::std`
 ...
 LL | /     mod std {
 LL | |         pub struct io;
 LL | |     }
-   | |_____- could also refer to `self::std`
+   | |_____- can refer to `self::std`
    |
    = help: write `::std` or `self::std` explicitly instead
    = note: relative `use` paths enabled by `#![feature(uniform_paths)]`

src/test/ui/rust-2018/uniform-paths/ambiguity.rs

@@ -13,7 +13,7 @@
 #![feature(uniform_paths)]
 
 use std::io;
-//~^ ERROR import from `std` is ambiguous
+//~^ ERROR `std` import is ambiguous
 
 mod std {
     pub struct io;

src/test/ui/rust-2018/uniform-paths/ambiguity.stderr

@@ -1,13 +1,13 @@
-error: import from `std` is ambiguous
+error: `std` import is ambiguous
   --> $DIR/ambiguity.rs:15:5
    |
 LL |   use std::io;
-   |       ^^^ could refer to external crate `::std`
+   |       ^^^ can refer to external crate `::std`
 ...
 LL | / mod std {
 LL | |     pub struct io;
 LL | | }
-   | |_- could also refer to `self::std`
+   | |_- can refer to `self::std`
    |
    = help: write `::std` or `self::std` explicitly instead
    = note: relative `use` paths enabled by `#![feature(uniform_paths)]`