resolve: refactor path resolution #38014
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jseyfried
force-pushed
the
refactor_path_resolution
branch
from
8254908 to
f8e5593
Compare
|
Hey, I have a relatively large rewrite of high level path resolution and error reporting in progress: https://github.com/petrochenkov/rust/tree/altname. |
|
Reviewed. The changes are mostly orthogonal to my branch, the main contention is about |
eddyb
reviewed
Nov 26, 2016
| @@ -71,13 +71,13 @@ pub struct LoweringContext<'a> { | |||
|
|
|||
| pub trait Resolver { | |||
| // Resolve a global hir path generated by the lowerer when expanding `for`, `if let`, etc. | |||
| fn resolve_generated_global_path(&mut self, path: &hir::Path, is_value: bool) -> Def; | |||
| fn resolve_hir_path(&mut self, path: &hir::Path, is_value: bool) -> PathResolution; | |||
There was a problem hiding this comment.
There was a problem hiding this comment.
I'll wait for #37676 to merge and then rebase.
jseyfried
force-pushed
the
refactor_path_resolution
branch
from
f8e5593 to
cc0383b
Compare
|
@bors: r+ |
|
📌 Commit 84be399 has been approved by |
|
@nrc See #38014 (comment). @bors r- |
jseyfried
force-pushed
the
refactor_path_resolution
branch
from
84be399 to
f0fddcc
Compare
|
☔ The latest upstream changes (presumably #37676) made this pull request unmergeable. Please resolve the merge conflicts. |
| @@ -191,10 +189,6 @@ fn resolve_struct_error<'b, 'a: 'b, 'c>(resolver: &'b Resolver<'a>, | |||
| span: syntax_pos::Span, | |||
| resolution_error: ResolutionError<'c>) | |||
| -> DiagnosticBuilder<'a> { | |||
| if !resolver.emit_errors { | |||
| return resolver.session.diagnostic().struct_dummy(); | |||
There was a problem hiding this comment.
There should be a way to do speculative resolution without reporting errors.
It's used at least once now in fresh binding disambiguation and I'm using it in my error reporting branch as well.
There was a problem hiding this comment.
@petrochenkov You can still do speculative resolution by passing record_used: None to resolve_path, resolve_ident_in_lexical_scope, or resolve_name_in_module.
jseyfried
force-pushed
the
refactor_path_resolution
branch
from
f0fddcc to
8fe525d
Compare
|
Rebased and added a new commit -- r? @eddyb for new commit. |
eddyb
approved these changes
Nov 29, 2016
| @@ -353,9 +350,6 @@ impl<'a> LoweringContext<'a> { | |||
| // `<I as Iterator>::Item::default`. | |||
| let ty = self.ty(p.span, hir::TyPath(hir::QPath::Resolved(qself, path))); | |||
|
|
|||
| // Associate that innermost path type with the base Def. | |||
| self.resolver.record_resolution(ty.id, resolution.base_def); | |||
|
|
|||
| ty | |||
jseyfried
force-pushed
the
refactor_path_resolution
branch
from
a2ebc4c to
c871637
Compare
|
@bors r=nrc |
|
📌 Commit c871637 has been approved by |
bors
added a commit
that referenced
this pull request
Nov 30, 2016
resolve: refactor path resolution This is a pure refactoring, modulo minor diagnostics improvements. r? @nrc
This was referenced Nov 30, 2016
Closed
This was referenced Dec 4, 2016
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.
This was referenced Dec 31, 2016
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a pure refactoring, modulo minor diagnostics improvements.
r? @nrc