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

More systematic error reporting in path resolution #38154

Merged
merged 2 commits into from
Dec 26, 2016

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Dec 4, 2016

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 resolve: refactor path resolution #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:


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.

let ns = if let Def::AssociatedTy(..) = child.def { TypeNS } else { ValueNS };
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));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIXME: Move self.define(...) for associated items here.

Copy link
Contributor

@jseyfried jseyfried left a comment

Choose a reason for hiding this comment

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

I'd like to read through this again tomorrow, but otherwise r=me pending @jonathandturner.

@@ -419,7 +419,6 @@ impl<'b> Resolver<'b> {
let def_id = def.def_id();
let vis = match def {
Def::Macro(..) => ty::Visibility::Public,
_ if parent.is_trait() => ty::Visibility::Public,
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated, but I believe we don't need the Def::Macro special case anymore, i.e.
let vis = self.session.cstore.visibility(def_id); should suffice.

@@ -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.
Copy link
Contributor

@jseyfried jseyfried Dec 5, 2016

Choose a reason for hiding this comment

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

I find it unnatural to group these two cases into one error code -- I would think either have an error code just for latter case or not have a separate error code here at all.
That being said, I'm not too familiar with best practices for error codes, so I'll defer to @jonathandturner.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I mostly kept status quo regarding error codes, it may make sense to have a separate code for each (path_source, has_resolution) combination, 3 new error codes will need to be allocated.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added new error codes.

ty::Visibility::Public
if nonlocal || !self.is_accessible(vis) {
let msg = format!("visibilities can only be restricted to ancestor modules");
self.session.span_err(path.span, &msg);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FIXME: reset visibility to public when this error happens

@@ -14,6 +14,5 @@ enum Foo {

fn main() {
let f = Foo::Variant(42);
//~^ ERROR uses it like a function
//~| struct called like a function
//~^ ERROR expected function, found struct variant `Foo::Variant`
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to indicate we've removed the text on the primary error. I'd like to keep labels. This one specifically I think is helpful.


let homura = issue_19452_aux::Homura::Madoka;
//~^ ERROR uses it like a function
//~| struct called like a function
//~^ ERROR expected value, found struct variant `issue_19452_aux::Homura::Madoka`
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto to above (I won't repeat same comment)

//~| NOTE did you mean to call `Groom::shave`?
//~^ ERROR unresolved function `shave`
//~| NOTE no resolution found
//~| HELP did you mean to write `Self::shave`?
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's better to have the "did you mean to write Self::shave?" as the label text rather than a help. We've been trying to move in that direction since it's less text and a little friendlier.

| type parameter defined here
| ^^^ not a trait
|
= help: possible better candidate is found in another module, you can import it into scope: `use std::ops::Add;`
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems the label got worse here, but I like the help that was added. A combination might be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a separate problem from already mentioned "label vs help". I'd be okay with attaching "y is defined/imported here" for all "expected x found y" situations, but not for a single arbitrary chosen (and not even common) context.

Copy link
Contributor Author

@petrochenkov petrochenkov Dec 6, 2016

Choose a reason for hiding this comment

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

@jonathandturner
By the way, do you think this "possible better candidate is found ..." message may to be reworded into something? I feel this message is to long (doesn't fit 80 characters, for example).

--> $DIR/typo-suggestion.rs:18:26
|
18 | println!("Hello {}", fob);
| ^^^ did you mean `foo`?
| ^^^ no resolution found
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as comment above. We're trying to move to using "did you mean foo?" as the label text rather than the help because it's less overall text and a little friendlier.

@sophiajt
Copy link
Contributor

sophiajt commented Dec 6, 2016

Some thoughts:

  • Prefer using label text. I noticed some places the label text was removed and others it was changed to something more generic. We've been moving towards the more friendly "did you mean Blah?" as the label text where possible. For more complex errors using help is perfectly fine, but we should prefer a more direct, helpful label wherever we can.
  • I definitely like that we get to be a little more clear generally, though there were a couple places we lost helpful text, eg "calling a struct like a function". A message like this directly keys the user into what's going wrong and why. If specific text like that becomes too general it will take someone longer to understand and act on it.
  • That said, I like that this moves us towards a more universal heuristic that's generally higher quality
  • 👍 on moving to UI tests. Feel free to move as many as you like.

@petrochenkov
Copy link
Contributor Author

petrochenkov commented Dec 6, 2016

@jonathandturner

Prefer using label text. I noticed some places the label text was removed and others it was changed to something more generic. We've been moving towards the more friendly "did you mean Blah?" as the label text where possible. For more complex errors using help is perfectly fine, but we should prefer a more direct, helpful label wherever we can.

Thanks! This is the point 2 from my "alternatives/notes" list.

I definitely like that we get to be a little more clear generally, though there were a couple places we lost helpful text, eg "calling a struct like a function". A message like this directly keys the user into what's going wrong and why. If specific text like that becomes too general it will take someone longer to understand and act on it.

This particular message is reworded into generic (but saying the same thing) "expected function, found struct S" (+ special "did you mean S { /*fields*/ }", as before), so nothing is lost IMO (besides label vs help problem which I intend to fix).

👍 on moving to UI tests. Feel free to move as many as you like.

Will do!

@petrochenkov
Copy link
Contributor Author

Updated.
Help and notes are turned into labels, tests checking help or notes are moved into UI directory.
r? @jonathandturner

@rust-highfive rust-highfive assigned sophiajt and unassigned jseyfried Dec 8, 2016
@bors
Copy link
Contributor

bors commented Dec 8, 2016

☔ The latest upstream changes (presumably #38191) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov
Copy link
Contributor Author

Rebased.

@sophiajt
Copy link
Contributor

sophiajt commented Dec 9, 2016

@alexcrichton - is the above error legit?

@frewsxcv
Copy link
Member

frewsxcv commented Dec 9, 2016

All new PRs are currently failing on Travis. The error Travis is reporting for this one doesn't appear to be any different so this PR is Probably Fine™.

@bors
Copy link
Contributor

bors commented Dec 21, 2016

☔ The latest upstream changes (presumably #38499) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Dec 23, 2016

☔ The latest upstream changes (presumably #38232) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Dec 26, 2016

☔ The latest upstream changes (presumably #38490) made this pull request unmergeable. Please resolve the merge conflicts.

@jseyfried
Copy link
Contributor

@petrochenkov r=me (unless @jonathandturner objects).

@petrochenkov
Copy link
Contributor Author

@bors r=jseyfried

@bors
Copy link
Contributor

bors commented Dec 26, 2016

📌 Commit 09aba18 has been approved by jseyfried

@bors
Copy link
Contributor

bors commented Dec 26, 2016

⌛ Testing commit 09aba18 with merge 65c043f...

bors added a commit that referenced this pull request Dec 26, 2016
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.
@bors
Copy link
Contributor

bors commented Dec 26, 2016

☀️ Test successful - status-appveyor, status-travis
Approved by: jseyfried
Pushing 65c043f to master...

}

fn record_def(&mut self, node_id: NodeId, resolution: PathResolution) {
debug!("(recording def) recording {:?} for {}", resolution, node_id);
assert!(resolution.depth == 0 || resolution.base_def != Def::Err);
Copy link
Member

Choose a reason for hiding this comment

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

This adds an ICE to this example:

fn foo<T>() {
    const FOO: T::Item = ();
}

I'll change it locally to if resolution.base_def == Def::Err { resolution.depth = 0; }.

@petrochenkov petrochenkov deleted the altname branch March 16, 2017 19:45
jsonn pushed a commit to jsonn/pkgsrc that referenced this pull request Mar 20, 2017
Version 1.16.0 (2017-03-16)
===========================

Language
--------

* Lifetimes in statics and consts default to `'static`. [RFC 1623]
* [The compiler's `dead_code` lint now accounts for type aliases][38051].
* [Uninhabitable enums (those without any variants) no longer permit wildcard
  match patterns][38069]
* [Clean up semantics of `self` in an import list][38313]
* [`Self` may appear in `impl` headers][38920]
* [`Self` may appear in struct expressions][39282]

Compiler
--------

* [`rustc` now supports `--emit=metadata`, which causes rustc to emit
  a `.rmeta` file containing only crate metadata][38571]. This can be
  used by tools like the Rust Language Service to perform
  metadata-only builds.
* [Levenshtein based typo suggestions now work in most places, while
  previously they worked only for fields and sometimes for local
  variables][38927]. Together with the overhaul of "no
  resolution"/"unexpected resolution" errors (#[38154]) they result in
  large and systematic improvement in resolution diagnostics.
* [Fix `transmute::<T, U>` where `T` requires a bigger alignment than
  `U`][38670]
* [rustc: use -Xlinker when specifying an rpath with ',' in it][38798]
* [`rustc` no longer attempts to provide "consider using an explicit
  lifetime" suggestions][37057]. They were inaccurate.

Stabilized APIs
---------------

* [`VecDeque::truncate`]
* [`VecDeque::resize`]
* [`String::insert_str`]
* [`Duration::checked_add`]
* [`Duration::checked_sub`]
* [`Duration::checked_div`]
* [`Duration::checked_mul`]
* [`str::replacen`]
* [`str::repeat`]
* [`SocketAddr::is_ipv4`]
* [`SocketAddr::is_ipv6`]
* [`IpAddr::is_ipv4`]
* [`IpAddr::is_ipv6`]
* [`Vec::dedup_by`]
* [`Vec::dedup_by_key`]
* [`Result::unwrap_or_default`]
* [`<*const T>::wrapping_offset`]
* [`<*mut T>::wrapping_offset`]
* `CommandExt::creation_flags`
* [`File::set_permissions`]
* [`String::split_off`]

Libraries
---------

* [`[T]::binary_search` and `[T]::binary_search_by_key` now take
  their argument by `Borrow` parameter][37761]
* [All public types in std implement `Debug`][38006]
* [`IpAddr` implements `From<Ipv4Addr>` and `From<Ipv6Addr>`][38327]
* [`Ipv6Addr` implements `From<[u16; 8]>`][38131]
* [Ctrl-Z returns from `Stdin.read()` when reading from the console on
  Windows][38274]
* [std: Fix partial writes in `LineWriter`][38062]
* [std: Clamp max read/write sizes on Unix][38062]
* [Use more specific panic message for `&str` slicing errors][38066]
* [`TcpListener::set_only_v6` is deprecated][38304]. This
  functionality cannot be achieved in std currently.
* [`writeln!`, like `println!`, now accepts a form with no string
  or formatting arguments, to just print a newline][38469]
* [Implement `iter::Sum` and `iter::Product` for `Result`][38580]
* [Reduce the size of static data in `std_unicode::tables`][38781]
* [`char::EscapeDebug`, `EscapeDefault`, `EscapeUnicode`,
  `CaseMappingIter`, `ToLowercase`, `ToUppercase`, implement
  `Display`][38909]
* [`Duration` implements `Sum`][38712]
* [`String` implements `ToSocketAddrs`][39048]

Cargo
-----

* [The `cargo check` command does a type check of a project without
  building it][cargo/3296]
* [crates.io will display CI badges from Travis and AppVeyor, if
  specified in Cargo.toml][cargo/3546]
* [crates.io will display categories listed in Cargo.toml][cargo/3301]
* [Compilation profiles accept integer values for `debug`, in addition
  to `true` and `false`. These are passed to `rustc` as the value to
  `-C debuginfo`][cargo/3534]
* [Implement `cargo --version --verbose`][cargo/3604]
* [All builds now output 'dep-info' build dependencies compatible with
  make and ninja][cargo/3557]
* [Build all workspace members with `build --all`][cargo/3511]
* [Document all workspace members with `doc --all`][cargo/3515]
* [Path deps outside workspace are not members][cargo/3443]

Misc
----

* [`rustdoc` has a `--sysroot` argument that, like `rustc`, specifies
  the path to the Rust implementation][38589]
* [The `armv7-linux-androideabi` target no longer enables NEON
  extensions, per Google's ABI guide][38413]
* [The stock standard library can be compiled for Redox OS][38401]
* [Rust has initial SPARC support][38726]. Tier 3. No builds
  available.
* [Rust has experimental support for Nvidia PTX][38559]. Tier 3. No
  builds available.
* [Fix backtraces on i686-pc-windows-gnu by disabling FPO][39379]

Compatibility Notes
-------------------

* [Uninhabitable enums (those without any variants) no longer permit wildcard
  match patterns][38069]
* In this release, references to uninhabited types can not be
  pattern-matched. This was accidentally allowed in 1.15.
* [The compiler's `dead_code` lint now accounts for type aliases][38051].
* [Ctrl-Z returns from `Stdin.read()` when reading from the console on
  Windows][38274]
* [Clean up semantics of `self` in an import list][38313]

[37057]: rust-lang/rust#37057
[37761]: rust-lang/rust#37761
[38006]: rust-lang/rust#38006
[38051]: rust-lang/rust#38051
[38062]: rust-lang/rust#38062
[38062]: rust-lang/rust#38622
[38066]: rust-lang/rust#38066
[38069]: rust-lang/rust#38069
[38131]: rust-lang/rust#38131
[38154]: rust-lang/rust#38154
[38274]: rust-lang/rust#38274
[38304]: rust-lang/rust#38304
[38313]: rust-lang/rust#38313
[38314]: rust-lang/rust#38314
[38327]: rust-lang/rust#38327
[38401]: rust-lang/rust#38401
[38413]: rust-lang/rust#38413
[38469]: rust-lang/rust#38469
[38559]: rust-lang/rust#38559
[38571]: rust-lang/rust#38571
[38580]: rust-lang/rust#38580
[38589]: rust-lang/rust#38589
[38670]: rust-lang/rust#38670
[38712]: rust-lang/rust#38712
[38726]: rust-lang/rust#38726
[38781]: rust-lang/rust#38781
[38798]: rust-lang/rust#38798
[38909]: rust-lang/rust#38909
[38920]: rust-lang/rust#38920
[38927]: rust-lang/rust#38927
[39048]: rust-lang/rust#39048
[39282]: rust-lang/rust#39282
[39379]: rust-lang/rust#39379
[`<*const T>::wrapping_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.wrapping_offset
[`<*mut T>::wrapping_offset`]: https://doc.rust-lang.org/std/primitive.pointer.html#method.wrapping_offset
[`Duration::checked_add`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_add
[`Duration::checked_div`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_div
[`Duration::checked_mul`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_mul
[`Duration::checked_sub`]: https://doc.rust-lang.org/std/time/struct.Duration.html#method.checked_sub
[`File::set_permissions`]: https://doc.rust-lang.org/std/fs/struct.File.html#method.set_permissions
[`IpAddr::is_ipv4`]: https://doc.rust-lang.org/std/net/enum.IpAddr.html#method.is_ipv4
[`IpAddr::is_ipv6`]: https://doc.rust-lang.org/std/net/enum.IpAddr.html#method.is_ipv6
[`Result::unwrap_or_default`]: https://doc.rust-lang.org/std/result/enum.Result.html#method.unwrap_or_default
[`SocketAddr::is_ipv4`]: https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.is_ipv4
[`SocketAddr::is_ipv6`]: https://doc.rust-lang.org/std/net/enum.SocketAddr.html#method.is_ipv6
[`String::insert_str`]: https://doc.rust-lang.org/std/string/struct.String.html#method.insert_str
[`String::split_off`]: https://doc.rust-lang.org/std/string/struct.String.html#method.split_off
[`Vec::dedup_by_key`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.dedup_by_key
[`Vec::dedup_by`]: https://doc.rust-lang.org/std/vec/struct.Vec.html#method.dedup_by
[`VecDeque::resize`]:  https://doc.rust-lang.org/std/collections/vec_deque/struct.VecDeque.html#method.resize
[`VecDeque::truncate`]: https://doc.rust-lang.org/std/collections/vec_deque/struct.VecDeque.html#method.truncate
[`str::repeat`]: https://doc.rust-lang.org/std/primitive.str.html#method.repeat
[`str::replacen`]: https://doc.rust-lang.org/std/primitive.str.html#method.replacen
[cargo/3296]: rust-lang/cargo#3296
[cargo/3301]: rust-lang/cargo#3301
[cargo/3443]: rust-lang/cargo#3443
[cargo/3511]: rust-lang/cargo#3511
[cargo/3515]: rust-lang/cargo#3515
[cargo/3534]: rust-lang/cargo#3534
[cargo/3546]: rust-lang/cargo#3546
[cargo/3557]: rust-lang/cargo#3557
[cargo/3604]: rust-lang/cargo#3604
[RFC 1623]: https://github.com/rust-lang/rfcs/blob/master/text/1623-static.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants