Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rustc_resolve: don't treat uniform_paths canaries as ambiguities unless they resolve to distinct Def's. #54201

Merged
merged 3 commits into from
Sep 14, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
146 changes: 73 additions & 73 deletions src/librustc_resolve/resolve_imports.rs
Original file line number Diff line number Diff line change
Expand Up @@ -630,15 +630,16 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
self.finalize_resolutions_in(module);
}

#[derive(Default)]
struct UniformPathsCanaryResult<'a> {
struct UniformPathsCanaryResults<'a> {
name: Name,
module_scope: Option<&'a NameBinding<'a>>,
block_scopes: Vec<&'a NameBinding<'a>>,
}

// Collect all tripped `uniform_paths` canaries separately.
let mut uniform_paths_canaries: BTreeMap<
(Span, NodeId),
(Name, PerNS<UniformPathsCanaryResult>),
(Span, NodeId, Namespace),
UniformPathsCanaryResults,
> = BTreeMap::new();

let mut errors = false;
Expand All @@ -665,21 +666,25 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
import.module_path.len() > 0 &&
import.module_path[0].name == keywords::SelfValue.name();

let (prev_name, canary_results) =
uniform_paths_canaries.entry((import.span, import.id))
.or_insert((name, PerNS::default()));

// All the canaries with the same `id` should have the same `name`.
assert_eq!(*prev_name, name);

self.per_ns(|_, ns| {
if let Some(result) = result[ns].get().ok() {
let canary_results =
uniform_paths_canaries.entry((import.span, import.id, ns))
.or_insert(UniformPathsCanaryResults {
name,
module_scope: None,
block_scopes: vec![],
});

// All the canaries with the same `id` should have the same `name`.
assert_eq!(canary_results.name, name);

if has_explicit_self {
// There should only be one `self::x` (module-scoped) canary.
assert!(canary_results[ns].module_scope.is_none());
canary_results[ns].module_scope = Some(result);
assert!(canary_results.module_scope.is_none());
canary_results.module_scope = Some(result);
} else {
canary_results[ns].block_scopes.push(result);
canary_results.block_scopes.push(result);
}
}
});
Expand Down Expand Up @@ -720,77 +725,72 @@ impl<'a, 'b:'a, 'c: 'b> ImportResolver<'a, 'b, 'c> {
}

let uniform_paths_feature = self.session.features_untracked().uniform_paths;
for ((span, _), (name, results)) in uniform_paths_canaries {
self.per_ns(|this, ns| {
let external_crate = if ns == TypeNS && this.extern_prelude.contains(&name) {
let crate_id =
this.crate_loader.process_path_extern(name, span);
Some(DefId { krate: crate_id, index: CRATE_DEF_INDEX })
} else {
None
};
let result_filter = |result: &&NameBinding| {
// Ignore canaries that resolve to an import of the same crate.
// That is, we allow `use crate_name; use crate_name::foo;`.
if let Some(def_id) = external_crate {
if let Some(module) = result.module() {
if module.normal_ancestor_id == def_id {
return false;
}
}
}
for ((span, _, ns), results) in uniform_paths_canaries {
let name = results.name;
let external_crate = if ns == TypeNS && self.extern_prelude.contains(&name) {
let crate_id =
self.crate_loader.process_path_extern(name, span);
Some(Def::Mod(DefId { krate: crate_id, index: CRATE_DEF_INDEX }))
} else {
None
};

true
};
let module_scope = results[ns].module_scope.filter(result_filter);
let block_scopes = || {
results[ns].block_scopes.iter().cloned().filter(result_filter)
};
// Currently imports can't resolve in non-module scopes,
// we only have canaries in them for future-proofing.
if external_crate.is_none() && results.module_scope.is_none() {
return;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, this is now a for loop so I need to continue instead!

Pretty sure this caused #54253, will fix in #54116.

}

{
let mut all_results = external_crate.into_iter().chain(
results.module_scope.iter()
.chain(&results.block_scopes)
.map(|binding| binding.def())
);
let first = all_results.next().unwrap();

// An ambiguity requires more than one possible resolution.
// An ambiguity requires more than one *distinct* possible resolution.
let possible_resultions =
(external_crate.is_some() as usize) +
(module_scope.is_some() as usize) +
(block_scopes().next().is_some() as usize);
1 + all_results.filter(|&def| def != first).count();
if possible_resultions <= 1 {
return;
}
}

errors = true;
errors = true;

let msg = format!("`{}` import is ambiguous", name);
let mut err = this.session.struct_span_err(span, &msg);
let mut suggestion_choices = String::new();
if external_crate.is_some() {
write!(suggestion_choices, "`::{}`", name);
err.span_label(span,
format!("can refer to external crate `::{}`", name));
}
if let Some(result) = module_scope {
if !suggestion_choices.is_empty() {
suggestion_choices.push_str(" or ");
}
write!(suggestion_choices, "`self::{}`", name);
if uniform_paths_feature {
err.span_label(result.span,
format!("can refer to `self::{}`", name));
} else {
err.span_label(result.span,
format!("may refer to `self::{}` in the future", name));
}
}
for result in block_scopes() {
err.span_label(result.span,
format!("shadowed by block-scoped `{}`", name));
let msg = format!("`{}` import is ambiguous", name);
let mut err = self.session.struct_span_err(span, &msg);
let mut suggestion_choices = String::new();
if external_crate.is_some() {
write!(suggestion_choices, "`::{}`", name);
err.span_label(span,
format!("can refer to external crate `::{}`", name));
}
if let Some(result) = results.module_scope {
if !suggestion_choices.is_empty() {
suggestion_choices.push_str(" or ");
}
err.help(&format!("write {} explicitly instead", suggestion_choices));
write!(suggestion_choices, "`self::{}`", name);
if uniform_paths_feature {
err.note("relative `use` paths enabled by `#![feature(uniform_paths)]`");
err.span_label(result.span,
format!("can refer to `self::{}`", name));
} else {
err.note("in the future, `#![feature(uniform_paths)]` may become the default");
err.span_label(result.span,
format!("may refer to `self::{}` in the future", name));
}
err.emit();
});
}
for result in results.block_scopes {
err.span_label(result.span,
format!("shadowed by block-scoped `{}`", name));
}
err.help(&format!("write {} explicitly instead", suggestion_choices));
if uniform_paths_feature {
err.note("relative `use` paths enabled by `#![feature(uniform_paths)]`");
} else {
err.note("in the future, `#![feature(uniform_paths)]` may become the default");
}
err.emit();
}

if !error_vec.is_empty() {
Expand Down
8 changes: 8 additions & 0 deletions src/test/ui/run-pass/uniform-paths/basic-nested.rs
Original file line number Diff line number Diff line change
Expand Up @@ -59,4 +59,12 @@ fn main() {
bar::io::stdout();
bar::std();
bar::std!();

{
// Test that having `io` in a module scope and a non-module
// scope is allowed, when both resolve to the same definition.
use std::io;
use io::stdout;
stdout();
}
}
8 changes: 8 additions & 0 deletions src/test/ui/run-pass/uniform-paths/basic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,12 @@ fn main() {
Foo(());
std_io::stdout();
local_io(());

{
// Test that having `std_io` in a module scope and a non-module
// scope is allowed, when both resolve to the same definition.
use std::io as std_io;
use std_io::stdout;
stdout();
}
}