rustc_resolve: inject ambiguity "canaries" when #![feature(uniform_paths)] is enabled.

Author: eddyb

src/librustc_resolve/build_reduced_graph.rs

@@ -113,16 +113,24 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
         }
     }
 
-    fn build_reduced_graph_for_use_tree(&mut self,
-                                        root_use_tree: &ast::UseTree,
-                                        root_id: NodeId,
-                                        use_tree: &ast::UseTree,
-                                        id: NodeId,
-                                        vis: ty::Visibility,
-                                        prefix: &ast::Path,
-                                        nested: bool,
-                                        item: &Item,
-                                        expansion: Mark) {
+    fn build_reduced_graph_for_use_tree(
+        &mut self,
+        root_use_tree: &ast::UseTree,
+        root_id: NodeId,
+        use_tree: &ast::UseTree,
+        id: NodeId,
+        vis: ty::Visibility,
+        prefix: &ast::Path,
+        mut uniform_paths_canary_emitted: bool,
+        nested: bool,
+        item: &Item,
+        expansion: Mark,
+    ) {
+        debug!("build_reduced_graph_for_use_tree(prefix={:?}, \
+                uniform_paths_canary_emitted={}, \
+                use_tree={:?}, nested={})",
+               prefix, uniform_paths_canary_emitted, use_tree, nested);
+
         let is_prelude = attr::contains_name(&item.attrs, "prelude_import");
         let path = &use_tree.prefix;
 
@@ -131,6 +139,71 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
             .map(|seg| seg.ident)
             .collect();
 
+        debug!("build_reduced_graph_for_use_tree: module_path={:?}", module_path);
+
+        // `#[feature(uniform_paths)]` allows an unqualified import path,
+        // e.g. `use x::...;` to resolve not just globally (`use ::x::...;`)
+        // but also relatively (`use self::x::...;`). To catch ambiguities
+        // that might arise from both of these being available and resolution
+        // silently picking one of them, an artificial `use self::x as _;`
+        // import is injected as a "canary", and an error is emitted if it
+        // successfully resolves while an `x` external crate exists.
+        //
+        // Additionally, the canary might be able to catch limitations of the
+        // current implementation, where `::x` may be chosen due to `self::x`
+        // not existing, but `self::x` could appear later, from macro expansion.
+        //
+        // NB. The canary currently only errors if the `x::...` path *could*
+        // resolve as a relative path through the extern crate, i.e. `x` is
+        // in `extern_prelude`, *even though* `::x` might still forcefully
+        // load a non-`extern_prelude` crate.
+        // While always producing an ambiguity errors if `self::x` exists and
+        // a crate *could* be loaded, would be more conservative, imports for
+        // local modules named `test` (or less commonly, `syntax` or `log`),
+        // would need to be qualified (e.g. `self::test`), which is considered
+        // ergonomically unacceptable.
+        let emit_uniform_paths_canary =
+            !uniform_paths_canary_emitted &&
+            module_path.get(0).map_or(false, |ident| {
+                !ident.is_path_segment_keyword()
+            });
+        if emit_uniform_paths_canary {
+            // Relative paths should only get here if the feature-gate is on.
+            assert!(self.session.rust_2018() &&
+                    self.session.features_untracked().uniform_paths);
+
+            let source = module_path[0];
+            let subclass = SingleImport {
+                target: Ident {
+                    name: keywords::Underscore.name().gensymed(),
+                    span: source.span,
+                },
+                source,
+                result: PerNS {
+                    type_ns: Cell::new(Err(Undetermined)),
+                    value_ns: Cell::new(Err(Undetermined)),
+                    macro_ns: Cell::new(Err(Undetermined)),
+                },
+                type_ns_only: false,
+            };
+            self.add_import_directive(
+                vec![Ident {
+                    name: keywords::SelfValue.name(),
+                    span: source.span,
+                }],
+                subclass,
+                source.span,
+                id,
+                root_use_tree.span,
+                root_id,
+                ty::Visibility::Invisible,
+                expansion,
+                true, // is_uniform_paths_canary
+            );
+
+            uniform_paths_canary_emitted = true;
+        }
+
         match use_tree.kind {
             ast::UseTreeKind::Simple(rename, ..) => {
                 let mut ident = use_tree.ident();
@@ -223,6 +296,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
                     root_id,
                     vis,
                     expansion,
+                    false, // is_uniform_paths_canary
                 );
             }
             ast::UseTreeKind::Glob => {
@@ -239,6 +313,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
                     root_id,
                     vis,
                     expansion,
+                    false, // is_uniform_paths_canary
                 );
             }
             ast::UseTreeKind::Nested(ref items) => {
@@ -273,7 +348,16 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
 
                 for &(ref tree, id) in items {
                     self.build_reduced_graph_for_use_tree(
-                        root_use_tree, root_id, tree, id, vis, &prefix, true, item, expansion
+                        root_use_tree,
+                        root_id,
+                        tree,
+                        id,
+                        vis,
+                        &prefix,
+                        uniform_paths_canary_emitted,
+                        true,
+                        item,
+                        expansion,
                     );
                 }
             }
@@ -305,7 +389,16 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
                 };
 
                 self.build_reduced_graph_for_use_tree(
-                    use_tree, item.id, use_tree, item.id, vis, &prefix, false, item, expansion,
+                    use_tree,
+                    item.id,
+                    use_tree,
+                    item.id,
+                    vis,
+                    &prefix,
+                    false, // uniform_paths_canary_emitted
+                    false,
+                    item,
+                    expansion,
                 );
             }
 
@@ -333,6 +426,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
                     vis: Cell::new(vis),
                     expansion,
                     used: Cell::new(used),
+                    is_uniform_paths_canary: false,
                 });
                 self.potentially_unused_imports.push(directive);
                 let imported_binding = self.import(binding, directive);
@@ -735,6 +829,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
             vis: Cell::new(ty::Visibility::Restricted(DefId::local(CRATE_DEF_INDEX))),
             expansion,
             used: Cell::new(false),
+            is_uniform_paths_canary: false,
         });
 
         if let Some(span) = legacy_imports.import_all {

src/librustc_resolve/resolve_imports.rs

@@ -91,6 +91,13 @@ pub struct ImportDirective<'a> {
     pub vis: Cell<ty::Visibility>,
     pub expansion: Mark,
     pub used: Cell<bool>,
+
+    /// Whether this import is a "canary" for the `uniform_paths` feature,
+    /// i.e. `use x::...;` results in an `use self::x as _;` canary.
+    /// This flag affects diagnostics: an error is reported if and only if
+    /// the import resolves successfully and an external crate with the same
+    /// name (`x` above) also exists; any resolution failures are ignored.
+    pub is_uniform_paths_canary: bool,
 }
 
 impl<'a> ImportDirective<'a> {
@@ -177,6 +184,11 @@ impl<'a, 'crateloader> Resolver<'a, 'crateloader> {
                     // But when a crate does exist, it will get chosen even when
                     // macro expansion could result in a success from the lookup
                     // in the `self` module, later on.
+                    //
+                    // NB. This is currently alleviated by the "ambiguity canaries"
+                    // (see `is_uniform_paths_canary`) that get introduced for the
+                    // maybe-relative imports handled here: if the false negative
+                    // case were to arise, it would *also* cause an ambiguity error.
                     if binding.is_ok() {
                         return binding;
                     }
@@ -369,7 +381,8 @@ impl<'a, 'crateloader> Resolver<'a, 'crateloader> {
                                 root_span: Span,
                                 root_id: NodeId,
                                 vis: ty::Visibility,
-                                expansion: Mark) {
+                                expansion: Mark,
+                                is_uniform_paths_canary: bool) {
         let current_module = self.current_module;
         let directive = self.arenas.alloc_import_directive(ImportDirective {
             parent: current_module,
@@ -383,8 +396,11 @@ impl<'a, 'crateloader> Resolver<'a, 'crateloader> {
             vis: Cell::new(vis),
             expansion,
             used: Cell::new(false),
+            is_uniform_paths_canary,
         });
 
+        debug!("add_import_directive({:?})", directive);
+
         self.indeterminate_imports.push(directive);
         match directive.subclass {
             SingleImport { target, type_ns_only, .. } => {
@@ -602,7 +618,47 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
         let mut seen_spans = FxHashSet();
         for i in 0 .. self.determined_imports.len() {
             let import = self.determined_imports[i];
-            if let Some((span, err)) = self.finalize_import(import) {
+            let error = self.finalize_import(import);
+
+            // 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 extern_crate_exists = self.extern_prelude.contains(&name);
+
+                // A successful `self::x` is ambiguous with an `x` external crate.
+                if !extern_crate_exists {
+                    continue;
+                }
+
+                errors = true;
+
+                let msg = format!("import from `{}` is ambiguous", name);
+                let mut err = self.session.struct_span_err(import.span, &msg);
+                err.span_label(import.span,
+                    format!("could refer to external crate `::{}`", name));
+                if let Some(result) = result {
+                    err.span_label(result.span,
+                        format!("could also refer to `self::{}`", name));
+                        err.span_label(result.span,
+                            format!("could also refer to `self::{}`", name));
+                }
+                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;
 
                 if let SingleImport { source, ref result, .. } = import.subclass {
@@ -632,9 +688,14 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
         // Report unresolved imports only if no hard error was already reported
         // to avoid generating multiple errors on the same import.
         if !errors {
-            if let Some(import) = self.indeterminate_imports.iter().next() {
+            for import in &self.indeterminate_imports {
+                if import.is_uniform_paths_canary {
+                    continue;
+                }
+
                 let error = ResolutionError::UnresolvedImport(None);
                 resolve_error(self.resolver, import.span, error);
+                break;
             }
         }
     }

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

@@ -0,0 +1,31 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// edition:2018
+
+#![feature(uniform_paths)]
+
+// This test is similar to `ambiguity-macros.rs`, but nested in a module.
+
+mod foo {
+    pub use std::io;
+    //~^ ERROR import from `std` is ambiguous
+
+    macro_rules! m {
+        () => {
+            mod std {
+                pub struct io;
+            }
+        }
+    }
+    m!();
+}
+
+fn main() {}

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

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

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

@@ -0,0 +1,29 @@
+// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// edition:2018
+
+#![feature(uniform_paths)]
+
+// This test is similar to `ambiguity.rs`, but with macros defining local items.
+
+use std::io;
+//~^ ERROR import from `std` is ambiguous
+
+macro_rules! m {
+    () => {
+        mod std {
+            pub struct io;
+        }
+    }
+}
+m!();
+
+fn main() {}

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

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