Skip to content

Commit 2ad865d

Browse files
eddybeddyb
committed
rustc_resolve: inject ambiguity "canaries" when #![feature(uniform_paths)] is enabled.
1 parent 39ce9ef commit 2ad865d

10 files changed

+343
-15
lines changed

‎src/librustc_resolve/build_reduced_graph.rs

+107-12
Original file line numberDiff line numberDiff line change
@@ -113,16 +113,24 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
113113
}
114114
}
115115

116-
fn build_reduced_graph_for_use_tree(&mut self,
117-
root_use_tree: &ast::UseTree,
118-
root_id: NodeId,
119-
use_tree: &ast::UseTree,
120-
id: NodeId,
121-
vis: ty::Visibility,
122-
prefix: &ast::Path,
123-
nested: bool,
124-
item: &Item,
125-
expansion: Mark) {
116+
fn build_reduced_graph_for_use_tree(
117+
&mut self,
118+
root_use_tree: &ast::UseTree,
119+
root_id: NodeId,
120+
use_tree: &ast::UseTree,
121+
id: NodeId,
122+
vis: ty::Visibility,
123+
prefix: &ast::Path,
124+
mut uniform_paths_canary_emitted: bool,
125+
nested: bool,
126+
item: &Item,
127+
expansion: Mark,
128+
) {
129+
debug!("build_reduced_graph_for_use_tree(prefix={:?}, \
130+
uniform_paths_canary_emitted={}, \
131+
use_tree={:?}, nested={})",
132+
prefix, uniform_paths_canary_emitted, use_tree, nested);
133+
126134
let is_prelude = attr::contains_name(&item.attrs, "prelude_import");
127135
let path = &use_tree.prefix;
128136

@@ -131,6 +139,71 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
131139
.map(|seg| seg.ident)
132140
.collect();
133141

142+
debug!("build_reduced_graph_for_use_tree: module_path={:?}", module_path);
143+
144+
// `#[feature(uniform_paths)]` allows an unqualified import path,
145+
// e.g. `use x::...;` to resolve not just globally (`use ::x::...;`)
146+
// but also relatively (`use self::x::...;`). To catch ambiguities
147+
// that might arise from both of these being available and resolution
148+
// silently picking one of them, an artificial `use self::x as _;`
149+
// import is injected as a "canary", and an error is emitted if it
150+
// successfully resolves while an `x` external crate exists.
151+
//
152+
// Additionally, the canary might be able to catch limitations of the
153+
// current implementation, where `::x` may be chosen due to `self::x`
154+
// not existing, but `self::x` could appear later, from macro expansion.
155+
//
156+
// NB. The canary currently only errors if the `x::...` path *could*
157+
// resolve as a relative path through the extern crate, i.e. `x` is
158+
// in `extern_prelude`, *even though* `::x` might still forcefully
159+
// load a non-`extern_prelude` crate.
160+
// While always producing an ambiguity errors if `self::x` exists and
161+
// a crate *could* be loaded, would be more conservative, imports for
162+
// local modules named `test` (or less commonly, `syntax` or `log`),
163+
// would need to be qualified (e.g. `self::test`), which is considered
164+
// ergonomically unacceptable.
165+
let emit_uniform_paths_canary =
166+
!uniform_paths_canary_emitted &&
167+
module_path.get(0).map_or(false, |ident| {
168+
!ident.is_path_segment_keyword()
169+
});
170+
if emit_uniform_paths_canary {
171+
// Relative paths should only get here if the feature-gate is on.
172+
assert!(self.session.rust_2018() &&
173+
self.session.features_untracked().uniform_paths);
174+
175+
let source = module_path[0];
176+
let subclass = SingleImport {
177+
target: Ident {
178+
name: keywords::Underscore.name().gensymed(),
179+
span: source.span,
180+
},
181+
source,
182+
result: PerNS {
183+
type_ns: Cell::new(Err(Undetermined)),
184+
value_ns: Cell::new(Err(Undetermined)),
185+
macro_ns: Cell::new(Err(Undetermined)),
186+
},
187+
type_ns_only: false,
188+
};
189+
self.add_import_directive(
190+
vec![Ident {
191+
name: keywords::SelfValue.name(),
192+
span: source.span,
193+
}],
194+
subclass,
195+
source.span,
196+
id,
197+
root_use_tree.span,
198+
root_id,
199+
ty::Visibility::Invisible,
200+
expansion,
201+
true, // is_uniform_paths_canary
202+
);
203+
204+
uniform_paths_canary_emitted = true;
205+
}
206+
134207
match use_tree.kind {
135208
ast::UseTreeKind::Simple(rename, ..) => {
136209
let mut ident = use_tree.ident();
@@ -223,6 +296,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
223296
root_id,
224297
vis,
225298
expansion,
299+
false, // is_uniform_paths_canary
226300
);
227301
}
228302
ast::UseTreeKind::Glob => {
@@ -239,6 +313,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
239313
root_id,
240314
vis,
241315
expansion,
316+
false, // is_uniform_paths_canary
242317
);
243318
}
244319
ast::UseTreeKind::Nested(ref items) => {
@@ -273,7 +348,16 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
273348

274349
for &(ref tree, id) in items {
275350
self.build_reduced_graph_for_use_tree(
276-
root_use_tree, root_id, tree, id, vis, &prefix, true, item, expansion
351+
root_use_tree,
352+
root_id,
353+
tree,
354+
id,
355+
vis,
356+
&prefix,
357+
uniform_paths_canary_emitted,
358+
true,
359+
item,
360+
expansion,
277361
);
278362
}
279363
}
@@ -305,7 +389,16 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
305389
};
306390

307391
self.build_reduced_graph_for_use_tree(
308-
use_tree, item.id, use_tree, item.id, vis, &prefix, false, item, expansion,
392+
use_tree,
393+
item.id,
394+
use_tree,
395+
item.id,
396+
vis,
397+
&prefix,
398+
false, // uniform_paths_canary_emitted
399+
false,
400+
item,
401+
expansion,
309402
);
310403
}
311404

@@ -333,6 +426,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
333426
vis: Cell::new(vis),
334427
expansion,
335428
used: Cell::new(used),
429+
is_uniform_paths_canary: false,
336430
});
337431
self.potentially_unused_imports.push(directive);
338432
let imported_binding = self.import(binding, directive);
@@ -735,6 +829,7 @@ impl<'a, 'cl> Resolver<'a, 'cl> {
735829
vis: Cell::new(ty::Visibility::Restricted(DefId::local(CRATE_DEF_INDEX))),
736830
expansion,
737831
used: Cell::new(false),
832+
is_uniform_paths_canary: false,
738833
});
739834

740835
if let Some(span) = legacy_imports.import_all {

‎src/librustc_resolve/resolve_imports.rs

+64-3
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,13 @@ pub struct ImportDirective<'a> {
9191
pub vis: Cell<ty::Visibility>,
9292
pub expansion: Mark,
9393
pub used: Cell<bool>,
94+
95+
/// Whether this import is a "canary" for the `uniform_paths` feature,
96+
/// i.e. `use x::...;` results in an `use self::x as _;` canary.
97+
/// This flag affects diagnostics: an error is reported if and only if
98+
/// the import resolves successfully and an external crate with the same
99+
/// name (`x` above) also exists; any resolution failures are ignored.
100+
pub is_uniform_paths_canary: bool,
94101
}
95102

96103
impl<'a> ImportDirective<'a> {
@@ -177,6 +184,11 @@ impl<'a, 'crateloader> Resolver<'a, 'crateloader> {
177184
// But when a crate does exist, it will get chosen even when
178185
// macro expansion could result in a success from the lookup
179186
// in the `self` module, later on.
187+
//
188+
// NB. This is currently alleviated by the "ambiguity canaries"
189+
// (see `is_uniform_paths_canary`) that get introduced for the
190+
// maybe-relative imports handled here: if the false negative
191+
// case were to arise, it would *also* cause an ambiguity error.
180192
if binding.is_ok() {
181193
return binding;
182194
}
@@ -369,7 +381,8 @@ impl<'a, 'crateloader> Resolver<'a, 'crateloader> {
369381
root_span: Span,
370382
root_id: NodeId,
371383
vis: ty::Visibility,
372-
expansion: Mark) {
384+
expansion: Mark,
385+
is_uniform_paths_canary: bool) {
373386
let current_module = self.current_module;
374387
let directive = self.arenas.alloc_import_directive(ImportDirective {
375388
parent: current_module,
@@ -383,8 +396,11 @@ impl<'a, 'crateloader> Resolver<'a, 'crateloader> {
383396
vis: Cell::new(vis),
384397
expansion,
385398
used: Cell::new(false),
399+
is_uniform_paths_canary,
386400
});
387401

402+
debug!("add_import_directive({:?})", directive);
403+
388404
self.indeterminate_imports.push(directive);
389405
match directive.subclass {
390406
SingleImport { target, type_ns_only, .. } => {
@@ -602,7 +618,47 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
602618
let mut seen_spans = FxHashSet();
603619
for i in 0 .. self.determined_imports.len() {
604620
let import = self.determined_imports[i];
605-
if let Some((span, err)) = self.finalize_import(import) {
621+
let error = self.finalize_import(import);
622+
623+
// For a `#![feature(uniform_paths)]` `use self::x as _` canary,
624+
// failure is ignored, while success may cause an ambiguity error.
625+
if import.is_uniform_paths_canary {
626+
let (name, result) = match import.subclass {
627+
SingleImport { source, ref result, .. } => {
628+
let type_ns = result[TypeNS].get().ok();
629+
let value_ns = result[ValueNS].get().ok();
630+
(source.name, type_ns.or(value_ns))
631+
}
632+
_ => bug!(),
633+
};
634+
635+
if error.is_some() {
636+
continue;
637+
}
638+
639+
let extern_crate_exists = self.extern_prelude.contains(&name);
640+
641+
// A successful `self::x` is ambiguous with an `x` external crate.
642+
if !extern_crate_exists {
643+
continue;
644+
}
645+
646+
errors = true;
647+
648+
let msg = format!("import from `{}` is ambiguous", name);
649+
let mut err = self.session.struct_span_err(import.span, &msg);
650+
err.span_label(import.span,
651+
format!("could refer to external crate `::{}`", name));
652+
if let Some(result) = result {
653+
err.span_label(result.span,
654+
format!("could also refer to `self::{}`", name));
655+
err.span_label(result.span,
656+
format!("could also refer to `self::{}`", name));
657+
}
658+
err.help(&format!("write `::{0}` or `self::{0}` explicitly instead", name));
659+
err.note("relative `use` paths enabled by `#![feature(uniform_paths)]`");
660+
err.emit();
661+
} else if let Some((span, err)) = error {
606662
errors = true;
607663

608664
if let SingleImport { source, ref result, .. } = import.subclass {
@@ -632,9 +688,14 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
632688
// Report unresolved imports only if no hard error was already reported
633689
// to avoid generating multiple errors on the same import.
634690
if !errors {
635-
if let Some(import) = self.indeterminate_imports.iter().next() {
691+
for import in &self.indeterminate_imports {
692+
if import.is_uniform_paths_canary {
693+
continue;
694+
}
695+
636696
let error = ResolutionError::UnresolvedImport(None);
637697
resolve_error(self.resolver, import.span, error);
698+
break;
638699
}
639700
}
640701
}

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

+31
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// edition:2018
12+
13+
#![feature(uniform_paths)]
14+
15+
// This test is similar to `ambiguity-macros.rs`, but nested in a module.
16+
17+
mod foo {
18+
pub use std::io;
19+
//~^ ERROR import from `std` is ambiguous
20+
21+
macro_rules! m {
22+
() => {
23+
mod std {
24+
pub struct io;
25+
}
26+
}
27+
}
28+
m!();
29+
}
30+
31+
fn main() {}

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

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
error: import from `std` is ambiguous
2+
--> $DIR/ambiguity-macros-nested.rs:18:13
3+
|
4+
LL | pub use std::io;
5+
| ^^^ could refer to external crate `::std`
6+
...
7+
LL | / mod std {
8+
LL | | pub struct io;
9+
LL | | }
10+
| |_____________- could also refer to `self::std`
11+
|
12+
= help: write `::std` or `self::std` explicitly instead
13+
= note: relative `use` paths enabled by `#![feature(uniform_paths)]`
14+
15+
error: aborting due to previous error
16+

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

+29
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
// Copyright 2018 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// edition:2018
12+
13+
#![feature(uniform_paths)]
14+
15+
// This test is similar to `ambiguity.rs`, but with macros defining local items.
16+
17+
use std::io;
18+
//~^ ERROR import from `std` is ambiguous
19+
20+
macro_rules! m {
21+
() => {
22+
mod std {
23+
pub struct io;
24+
}
25+
}
26+
}
27+
m!();
28+
29+
fn main() {}

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

+16
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,16 @@
1+
error: import from `std` is ambiguous
2+
--> $DIR/ambiguity-macros.rs:17:5
3+
|
4+
LL | use std::io;
5+
| ^^^ could refer to external crate `::std`
6+
...
7+
LL | / mod std {
8+
LL | | pub struct io;
9+
LL | | }
10+
| |_________- could also refer to `self::std`
11+
|
12+
= help: write `::std` or `self::std` explicitly instead
13+
= note: relative `use` paths enabled by `#![feature(uniform_paths)]`
14+
15+
error: aborting due to previous error
16+

0 commit comments

Comments
 (0)