Skip to content

fix suggestions with nested paths #50969

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 22, 2018
Merged

fix suggestions with nested paths #50969

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cce9132
track the root `UseTree` in addition to the leaf
nikomatsakis May 18, 2018
068c436
pass down information about the root tree and use that to guide lint
nikomatsakis May 22, 2018
60c4eb4
create a rust-2018 directory for tests related to edition transition
nikomatsakis May 22, 2018
9f11714
pacify the mercilous tidy
nikomatsakis May 22, 2018
dfd2a13
add new test and add an existing scenario I didn't see covered
nikomatsakis May 22, 2018
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
28 changes: 24 additions & 4 deletions src/librustc_resolve/build_reduced_graph.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,8 @@ impl<'a> Resolver<'a> {
}

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,
Expand Down Expand Up @@ -182,7 +184,14 @@ impl<'a> Resolver<'a> {
type_ns_only,
};
self.add_import_directive(
module_path, subclass, use_tree.span, id, vis, expansion,
module_path,
subclass,
use_tree.span,
id,
root_use_tree.span,
root_id,
vis,
expansion,
);
}
ast::UseTreeKind::Glob => {
Expand All @@ -191,7 +200,14 @@ impl<'a> Resolver<'a> {
max_vis: Cell::new(ty::Visibility::Invisible),
};
self.add_import_directive(
module_path, subclass, use_tree.span, id, vis, expansion,
module_path,
subclass,
use_tree.span,
id,
root_use_tree.span,
root_id,
vis,
expansion,
);
}
ast::UseTreeKind::Nested(ref items) => {
Expand Down Expand Up @@ -226,7 +242,7 @@ impl<'a> Resolver<'a> {

for &(ref tree, id) in items {
self.build_reduced_graph_for_use_tree(
tree, id, vis, &prefix, true, item, expansion
root_use_tree, root_id, tree, id, vis, &prefix, true, item, expansion
);
}
}
Expand All @@ -249,7 +265,7 @@ impl<'a> Resolver<'a> {
};

self.build_reduced_graph_for_use_tree(
use_tree, item.id, vis, &prefix, false, item, expansion,
use_tree, item.id, use_tree, item.id, vis, &prefix, false, item, expansion,
);
}

Expand All @@ -266,10 +282,12 @@ impl<'a> Resolver<'a> {
let binding =
(module, ty::Visibility::Public, sp, expansion).to_name_binding(self.arenas);
let directive = self.arenas.alloc_import_directive(ImportDirective {
root_id: item.id,
id: item.id,
parent,
imported_module: Cell::new(Some(module)),
subclass: ImportDirectiveSubclass::ExternCrate(orig_name),
root_span: item.span,
span: item.span,
module_path: Vec::new(),
vis: Cell::new(vis),
Expand Down Expand Up @@ -640,10 +658,12 @@ impl<'a> Resolver<'a> {

let (graph_root, arenas) = (self.graph_root, self.arenas);
let macro_use_directive = |span| arenas.alloc_import_directive(ImportDirective {
root_id: item.id,
id: item.id,
parent: graph_root,
imported_module: Cell::new(Some(module)),
subclass: ImportDirectiveSubclass::MacroUse,
root_span: span,
span,
module_path: Vec::new(),
vis: Cell::new(ty::Visibility::Restricted(DefId::local(CRATE_DEF_INDEX))),
Expand Down
96 changes: 69 additions & 27 deletions src/librustc_resolve/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
html_favicon_url = "https://doc.rust-lang.org/favicon.ico",
html_root_url = "https://doc.rust-lang.org/nightly/")]

#![feature(crate_visibility_modifier)]
#![feature(rustc_diagnostic_macros)]
#![feature(slice_sort_by_cached_key)]

Expand Down Expand Up @@ -1655,11 +1656,17 @@ impl<'a> Resolver<'a> {
.map(|seg| Ident::new(seg.name, span))
.collect();
// FIXME (Manishearth): Intra doc links won't get warned of epoch changes
match self.resolve_path(&path, Some(namespace), true, span, None) {
match self.resolve_path(&path, Some(namespace), true, span, CrateLint::No) {
PathResult::Module(module) => *def = module.def().unwrap(),
PathResult::NonModule(path_res) if path_res.unresolved_segments() == 0 =>
*def = path_res.base_def(),
PathResult::NonModule(..) => match self.resolve_path(&path, None, true, span, None) {
PathResult::NonModule(..) => match self.resolve_path(
&path,
None,
true,
span,
CrateLint::No,
) {
PathResult::Failed(span, msg, _) => {
error_callback(self, span, ResolutionError::FailedToResolve(&msg));
}
Expand Down Expand Up @@ -2378,8 +2385,13 @@ impl<'a> Resolver<'a> {
if def != Def::Err {
new_id = Some(def.def_id());
let span = trait_ref.path.span;
if let PathResult::Module(module) = self.resolve_path(&path, None, false, span,
Some(trait_ref.ref_id)) {
if let PathResult::Module(module) = self.resolve_path(
&path,
None,
false,
span,
CrateLint::SimplePath(trait_ref.ref_id),
) {
new_val = Some((module, trait_ref.clone()));
}
}
Expand Down Expand Up @@ -2839,7 +2851,7 @@ impl<'a> Resolver<'a> {
} else {
let mod_path = &path[..path.len() - 1];
let mod_prefix = match this.resolve_path(mod_path, Some(TypeNS),
false, span, None) {
false, span, CrateLint::No) {
PathResult::Module(module) => module.def(),
_ => None,
}.map_or(format!(""), |def| format!("{} ", def.kind_name()));
Expand Down Expand Up @@ -3169,7 +3181,13 @@ impl<'a> Resolver<'a> {
));
}

let result = match self.resolve_path(&path, Some(ns), true, span, Some(id)) {
let result = match self.resolve_path(
&path,
Some(ns),
true,
span,
CrateLint::SimplePath(id),
) {
PathResult::NonModule(path_res) => path_res,
PathResult::Module(module) if !module.is_normal() => {
PathResolution::new(module.def().unwrap())
Expand Down Expand Up @@ -3206,7 +3224,13 @@ impl<'a> Resolver<'a> {
path[0].name != keywords::CrateRoot.name() &&
path[0].name != keywords::DollarCrate.name() {
let unqualified_result = {
match self.resolve_path(&[*path.last().unwrap()], Some(ns), false, span, None) {
match self.resolve_path(
&[*path.last().unwrap()],
Some(ns),
false,
span,
CrateLint::No,
) {
PathResult::NonModule(path_res) => path_res.base_def(),
PathResult::Module(module) => module.def().unwrap(),
_ => return Some(result),
Expand All @@ -3221,14 +3245,14 @@ impl<'a> Resolver<'a> {
Some(result)
}

fn resolve_path(&mut self,
path: &[Ident],
opt_ns: Option<Namespace>, // `None` indicates a module path
record_used: bool,
path_span: Span,
node_id: Option<NodeId>) // None indicates that we don't care about linting
// `::module` paths
-> PathResult<'a> {
fn resolve_path(
&mut self,
path: &[Ident],
opt_ns: Option<Namespace>, // `None` indicates a module path
record_used: bool,
path_span: Span,
crate_lint: CrateLint,
) -> PathResult<'a> {
let mut module = None;
let mut allow_super = true;
let mut second_binding = None;
Expand Down Expand Up @@ -3347,7 +3371,7 @@ impl<'a> Resolver<'a> {
return PathResult::NonModule(err_path_resolution());
} else if opt_ns.is_some() && (is_last || maybe_assoc) {
self.lint_if_path_starts_with_module(
node_id,
crate_lint,
path,
path_span,
second_binding,
Expand Down Expand Up @@ -3392,19 +3416,22 @@ impl<'a> Resolver<'a> {
}
}

self.lint_if_path_starts_with_module(node_id, path, path_span, second_binding);
self.lint_if_path_starts_with_module(crate_lint, path, path_span, second_binding);

PathResult::Module(module.unwrap_or(self.graph_root))
}

fn lint_if_path_starts_with_module(&self,
id: Option<NodeId>,
path: &[Ident],
path_span: Span,
second_binding: Option<&NameBinding>) {
let id = match id {
Some(id) => id,
None => return,
fn lint_if_path_starts_with_module(
&self,
crate_lint: CrateLint,
path: &[Ident],
path_span: Span,
second_binding: Option<&NameBinding>,
) {
let (diag_id, diag_span) = match crate_lint {
CrateLint::No => return,
CrateLint::SimplePath(id) => (id, path_span),
CrateLint::UsePath { root_id, root_span } => (root_id, root_span),
};

let first_name = match path.get(0) {
Expand Down Expand Up @@ -3440,7 +3467,7 @@ impl<'a> Resolver<'a> {
}
}

self.lint_path_starts_with_module(id, path_span);
self.lint_path_starts_with_module(diag_id, diag_span);
}

fn lint_path_starts_with_module(&self, id: NodeId, span: Span) {
Expand Down Expand Up @@ -3676,7 +3703,7 @@ impl<'a> Resolver<'a> {
// Search in module.
let mod_path = &path[..path.len() - 1];
if let PathResult::Module(module) = self.resolve_path(mod_path, Some(TypeNS),
false, span, None) {
false, span, CrateLint::No) {
add_module_candidates(module, &mut names);
}
}
Expand Down Expand Up @@ -4475,4 +4502,19 @@ pub enum MakeGlobMap {
No,
}

enum CrateLint {
/// Do not issue the lint
No,

/// This lint applies to some random path like `impl ::foo::Bar`
/// or whatever. In this case, we can take the span of that path.
SimplePath(NodeId),

/// This lint comes from a `use` statement. In this case, what we
/// care about really is the *root* `use` statement; e.g., if we
/// have nested things like `use a::{b, c}`, we care about the
/// `use a` part.
UsePath { root_id: NodeId, root_span: Span },
}

__build_diagnostic_array! { librustc_resolve, DIAGNOSTICS }
6 changes: 3 additions & 3 deletions src/librustc_resolve/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

use {AmbiguityError, Resolver, ResolutionError, resolve_error};
use {AmbiguityError, CrateLint, Resolver, ResolutionError, resolve_error};
use {Module, ModuleKind, NameBinding, NameBindingKind, PathResult};
use Namespace::{self, MacroNS};
use build_reduced_graph::BuildReducedGraphVisitor;
Expand Down Expand Up @@ -441,7 +441,7 @@ impl<'a> Resolver<'a> {
return Err(Determinacy::Determined);
}

let def = match self.resolve_path(&path, Some(MacroNS), false, span, None) {
let def = match self.resolve_path(&path, Some(MacroNS), false, span, CrateLint::No) {
PathResult::NonModule(path_res) => match path_res.base_def() {
Def::Err => Err(Determinacy::Determined),
def @ _ => {
Expand Down Expand Up @@ -619,7 +619,7 @@ impl<'a> Resolver<'a> {
pub fn finalize_current_module_macro_resolutions(&mut self) {
let module = self.current_module;
for &(ref path, span) in module.macro_resolutions.borrow().iter() {
match self.resolve_path(&path, Some(MacroNS), true, span, None) {
match self.resolve_path(&path, Some(MacroNS), true, span, CrateLint::No) {
PathResult::NonModule(_) => {},
PathResult::Failed(span, msg, _) => {
resolve_error(self, span, ResolutionError::FailedToResolve(&msg));
Expand Down
Loading