Skip to content

Commit

Permalink
Auto merge of #38154 - petrochenkov:altname, r=jseyfried
Browse files Browse the repository at this point in the history
More systematic error reporting in path resolution

Path resolution for types, expressions and patterns used various heuristics to give more helpful messages on unresolved or incorrectly resolved paths.
This PR combines these heuristics and applies them to all non-import paths.

First a path is resolved in all namespaces, starting from its primary namespace (to give messages like "expected function, found macro, you probably forgot `!`").
If this resolution doesn't give a desired result we create a base error - either "path is not resolved" or "path is resolved, but the resolution is not acceptable in this context".
Other helps and notes are applied to this base error using heuristics.

Here's the list of heuristics for a path with a last segment `name` in order.
First we issue special messages for unresolved `Self` and `self`.
Second we try to find free items named `name` in other modules and suggest to import them.
Then we try to find fields and associated items named `name` and suggest `self.name` or `Self::name`.
After that we try several deterministic context dependent heuristics like "expected value, found struct, you probably forgot `{}`".
If nothing of the above works we try to find candidates with other names using Levenshtein distance.

---

Some alternatives/notes/unresolved questions:
- ~~I had a strong desire to migrate all affected tests to `test/ui`, diagnostics comparison becomes much more meaningful, but I did this only for few tests so far.~~ (Done)
- ~~Labels for "unresolved path" errors are mostly useless now, it may make sense to move some help/notes to these labels, help becomes closer to the error this way.~~ (Done)
- ~~Currently only the first successful heuristic results in additional message shown to the user, it may make sense to print them all, they are rarely compatible, so the diagnostics bloat is unlikely.~~ (Done)
- Now when #38014 landed `resolve_path` can potentially be replaced with `smart_resolve_path` in couple more places - e.g. ~~visibilities~~ (done), ~~import prefixes~~ (done), HIR paths.

---

Some additional fixes:
- Associated suggestions and typo suggestions are filtered with a context specific predicate to avoid inapplicable suggestions.
- `adjust_local_def` works properly in speculative resolution.
- I also fixed a recently introduced ICE in partially resolved UFCS paths (see test `ufcs-partially-resolved.rs`).     Minimal reproduction:
    ```
    enum E {}
    fn main() {
        <u8 as E>::A;
    }
    ```
    Fixes #38409, fixes #38504 (duplicates).
- Some bugs in resolution of visibilities are fixed - `pub(Enum)`, `pub(Trait)`, `pub(non::local::path)`.
- Fixes #38012.
---

r? @jseyfried for technical details + @jonathandturner  for diagnostics changes
How to read the patch: `smart_resolve_path(_fragment)/resolve_qpath_anywhere` are written anew and replace `resolve_trait_reference`/`resolve_type`/`resolve_pattern_path`/`resolve_struct_path`/`resolve_expr` for `ExprKind::Path`, everything else can be read as a diff.
  • Loading branch information
bors committed Dec 26, 2016
2 parents 8493dbe + 09aba18 commit 65c043f
Show file tree
Hide file tree
Showing 164 changed files with 2,291 additions and 1,335 deletions.
6 changes: 2 additions & 4 deletions src/librustc/middle/cstore.rs
Original file line number Diff line number Diff line change
Expand Up @@ -298,8 +298,7 @@ pub trait CrateStore<'tcx> {

// trait/impl-item info
fn trait_of_item(&self, def_id: DefId) -> Option<DefId>;
fn associated_item<'a>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId)
-> Option<ty::AssociatedItem>;
fn associated_item(&self, def: DefId) -> Option<ty::AssociatedItem>;

// flags
fn is_const_fn(&self, did: DefId) -> bool;
Expand Down Expand Up @@ -456,8 +455,7 @@ impl<'tcx> CrateStore<'tcx> for DummyCrateStore {

// trait/impl-item info
fn trait_of_item(&self, def_id: DefId) -> Option<DefId> { bug!("trait_of_item") }
fn associated_item<'a>(&self, tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId)
-> Option<ty::AssociatedItem> { bug!("associated_item") }
fn associated_item(&self, def: DefId) -> Option<ty::AssociatedItem> { bug!("associated_item") }

// flags
fn is_const_fn(&self, did: DefId) -> bool { bug!("is_const_fn") }
Expand Down
5 changes: 2 additions & 3 deletions src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2071,7 +2071,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
pub fn associated_item(self, def_id: DefId) -> AssociatedItem {
self.associated_items.memoize(def_id, || {
if !def_id.is_local() {
return self.sess.cstore.associated_item(self.global_tcx(), def_id)
return self.sess.cstore.associated_item(def_id)
.expect("missing AssociatedItem in metadata");
}

Expand Down Expand Up @@ -2526,8 +2526,7 @@ impl<'a, 'gcx, 'tcx> TyCtxt<'a, 'gcx, 'tcx> {
/// ID of the impl that the method belongs to. Otherwise, return `None`.
pub fn impl_of_method(self, def_id: DefId) -> Option<DefId> {
if def_id.krate != LOCAL_CRATE {
return self.sess.cstore.associated_item(self.global_tcx(), def_id)
.and_then(|item| {
return self.sess.cstore.associated_item(def_id).and_then(|item| {
match item.container {
TraitContainer(_) => None,
ImplContainer(def_id) => Some(def_id),
Expand Down
3 changes: 1 addition & 2 deletions src/librustc_metadata/cstore_impl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -188,8 +188,7 @@ impl<'tcx> CrateStore<'tcx> for cstore::CStore {
self.get_crate_data(def_id.krate).get_trait_of_item(def_id.index)
}

fn associated_item<'a>(&self, _tcx: TyCtxt<'a, 'tcx, 'tcx>, def: DefId)
-> Option<ty::AssociatedItem>
fn associated_item(&self, def: DefId) -> Option<ty::AssociatedItem>
{
self.dep_graph.read(DepNode::MetaData(def));
self.get_crate_data(def.krate).get_associated_item(def.index)
Expand Down
75 changes: 22 additions & 53 deletions src/librustc_resolve/build_reduced_graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -417,58 +417,38 @@ impl<'a> Resolver<'a> {
let ident = Ident::with_empty_ctxt(child.name);
let def = child.def;
let def_id = def.def_id();
let vis = match def {
Def::Macro(..) => ty::Visibility::Public,
_ if parent.is_trait() => ty::Visibility::Public,
_ => self.session.cstore.visibility(def_id),
};
let vis = self.session.cstore.visibility(def_id);

match def {
Def::Mod(..) | Def::Enum(..) => {
let module = self.new_module(parent, ModuleKind::Def(def, ident.name), def_id);
self.define(parent, ident, TypeNS, (module, vis, DUMMY_SP, Mark::root()));
}
Def::Variant(..) => {
Def::Variant(..) | Def::TyAlias(..) => {
self.define(parent, ident, TypeNS, (def, vis, DUMMY_SP, Mark::root()));
}
Def::VariantCtor(..) => {
self.define(parent, ident, ValueNS, (def, vis, DUMMY_SP, Mark::root()));
}
Def::Fn(..) |
Def::Static(..) |
Def::Const(..) |
Def::AssociatedConst(..) |
Def::Method(..) => {
Def::Fn(..) | Def::Static(..) | Def::Const(..) |
Def::VariantCtor(..) | Def::StructCtor(..) => {
self.define(parent, ident, ValueNS, (def, vis, DUMMY_SP, Mark::root()));
}
Def::Trait(..) => {
let module_kind = ModuleKind::Def(def, ident.name);
let module = self.new_module(parent, module_kind, parent.normal_ancestor_id);
self.define(parent, ident, TypeNS, (module, vis, DUMMY_SP, Mark::root()));

// If this is a trait, add all the trait item names to the trait info.
let trait_item_def_ids = self.session.cstore.associated_item_def_ids(def_id);
for trait_item_def_id in trait_item_def_ids {
let trait_item_name = self.session.cstore.def_key(trait_item_def_id)
.disambiguated_data.data.get_opt_name()
.expect("opt_item_name returned None for trait");
self.trait_item_map.insert((trait_item_name, def_id), false);
}
}
Def::TyAlias(..) | Def::AssociatedTy(..) => {
self.define(parent, ident, TypeNS, (def, vis, DUMMY_SP, Mark::root()));
}
Def::Struct(..) => {
self.define(parent, ident, TypeNS, (def, vis, DUMMY_SP, Mark::root()));
for child in self.session.cstore.item_children(def_id) {
let ns = if let Def::AssociatedTy(..) = child.def { TypeNS } else { ValueNS };
let ident = Ident::with_empty_ctxt(child.name);
self.define(module, ident, ns, (child.def, ty::Visibility::Public,
DUMMY_SP, Mark::root()));

// Record field names for error reporting.
let field_names = self.session.cstore.struct_field_names(def_id);
self.insert_field_names(def_id, field_names);
}
Def::StructCtor(..) => {
self.define(parent, ident, ValueNS, (def, vis, DUMMY_SP, Mark::root()));
let has_self = self.session.cstore.associated_item(child.def.def_id())
.map_or(false, |item| item.method_has_self_argument);
self.trait_item_map.insert((def_id, child.name, ns), (child.def, has_self));
}
module.populated.set(true);
}
Def::Union(..) => {
Def::Struct(..) | Def::Union(..) => {
self.define(parent, ident, TypeNS, (def, vis, DUMMY_SP, Mark::root()));

// Record field names for error reporting.
Expand All @@ -478,15 +458,7 @@ impl<'a> Resolver<'a> {
Def::Macro(..) => {
self.define(parent, ident, MacroNS, (def, vis, DUMMY_SP, Mark::root()));
}
Def::Local(..) |
Def::PrimTy(..) |
Def::TyParam(..) |
Def::Upvar(..) |
Def::Label(..) |
Def::SelfTy(..) |
Def::Err => {
bug!("unexpected definition: {:?}", def);
}
_ => bug!("unexpected definition: {:?}", def)
}
}

Expand Down Expand Up @@ -751,18 +723,15 @@ impl<'a, 'b> Visitor<'a> for BuildReducedGraphVisitor<'a, 'b> {

// Add the item to the trait info.
let item_def_id = self.resolver.definitions.local_def_id(item.id);
let mut is_static_method = false;
let (def, ns) = match item.node {
TraitItemKind::Const(..) => (Def::AssociatedConst(item_def_id), ValueNS),
TraitItemKind::Method(ref sig, _) => {
is_static_method = !sig.decl.has_self();
(Def::Method(item_def_id), ValueNS)
}
TraitItemKind::Type(..) => (Def::AssociatedTy(item_def_id), TypeNS),
let (def, ns, has_self) = match item.node {
TraitItemKind::Const(..) => (Def::AssociatedConst(item_def_id), ValueNS, false),
TraitItemKind::Method(ref sig, _) =>
(Def::Method(item_def_id), ValueNS, sig.decl.has_self()),
TraitItemKind::Type(..) => (Def::AssociatedTy(item_def_id), TypeNS, false),
TraitItemKind::Macro(_) => bug!(), // handled above
};

self.resolver.trait_item_map.insert((item.ident.name, def_id), is_static_method);
self.resolver.trait_item_map.insert((def_id, item.ident.name, ns), (def, has_self));

let vis = ty::Visibility::Public;
self.resolver.define(parent, item.ident, ns, (def, vis, item.span, self.expansion));
Expand Down
27 changes: 26 additions & 1 deletion src/librustc_resolve/diagnostics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -860,6 +860,26 @@ match (A, B, C) {
```
"##,

E0422: r##"
You are trying to use an identifier that is either undefined or not a struct.
Erroneous code example:
``` compile_fail,E0422
fn main () {
let x = Foo { x: 1, y: 2 };
}
```
In this case, `Foo` is undefined, so it inherently isn't anything, and
definitely not a struct.
```compile_fail
fn main () {
let foo = 1;
let x = foo { x: 1, y: 2 };
}
```
In this case, `foo` is defined, but is not a struct, so Rust can't use it as
one.
"##,

E0423: r##"
A `struct` variant name was used like a function name.
Expand Down Expand Up @@ -1519,7 +1539,12 @@ register_diagnostics! {
// E0419, merged into 531
// E0420, merged into 532
// E0421, merged into 531
// E0422, merged into 531/532
E0531, // unresolved pattern path kind `name`
// E0427, merged into 530
E0573,
E0574,
E0575,
E0576,
E0577,
E0578,
}
Loading

0 comments on commit 65c043f

Please sign in to comment.