syntax: recovery for incorrect associated item paths like [T; N]::clone
#46788
Conversation
petrochenkov
requested review from
estebank
and removed request for
estebank
| --> $DIR/bad-assoc-pat.rs:13:9 | ||
| | | ||
| 13 | [u8]::AssocItem => {} | ||
| | ^^^^^^^^^^^^^^^ help: try: `<[u8]>::AssocItem` |
There was a problem hiding this comment.
Suggestions are reserved for cases where our level of certainty that they're correct is very high (sometimes going to the length of continuing parsing to verify our theory). Because this error happens at the parser level, we don't know whether AssocItem is an associated item or not, so normally we would suggest using a span_label instead. Despite this, in this case I'm ok with the use of a span_suggestion, but I would reword it to something more descriptive, like "if you meant to use an associated item, surround the type with brackets:", or something shorter (long suggestion messages make the suggestion be presented on it's own).
There was a problem hiding this comment.
IIRC, @oli-obk in the recent months moved everything containing code snippets to span_suggestion, regardless of suggestion reliability.
(FWIW, I'd also estimate this recovery as pretty reliable.)
"if you meant to use an associated item, surround the type with brackets:"
The primary message already says roughly this, so I avoided repeating it in the label (I also like really short labels in general).
There was a problem hiding this comment.
I'm working on suggestion levels. Just make everything a suggestion and let me worry about the probablitity of it being correct
There was a problem hiding this comment.
@oli-obk I've been thinking about making the error more structured, basically having a struct for each possible error the compiler generates, that way we can start playing with having the --explain content inline with the actual code being compiled (similar output to what Elm produces). This would require suggestions to be reworked slightly as well. What do you think?
There was a problem hiding this comment.
Making the errors more explicit is definitely a good idea. Currently analysis code and error reporting is quite mangled. Maybe we can get it separated with this method.
The most important thing is that the diagnostics API stays as usable as it is now. Making it more complex will only unnecessarily hold up regular development
There was a problem hiding this comment.
@oli-obk I definitely agree. The only thing that worries me is decoupling the place where the error is emitted and where it is "defined" in different files might lead to harder maintenance, but the way I'm thinking about it it should still allow the current ad-hoc DiagnosticBuilder usage to keep the current flexibility (and allowing a gradual migration to these structured errors).
| fn to_string(&self) -> String { | ||
| pprust::ty_to_string(self) | ||
| } | ||
| const PATH_STYLE: PathStyle = PathStyle::Type; |
There was a problem hiding this comment.
I think it is customary to have the consts at the beginning of an impl, but don't bother to change it here, this is short enough to not make a difference :)
|
Let's merge this, we can change the wording later :) @bors r+ |
|
📌 Commit 70e5c37 has been approved by |
|
☀️ Test successful - status-appveyor, status-travis |
syntax: Follow-up to the incorrect qpath recovery PR cc rust-lang#46788 Add tests checking that "priority" of qpath recovery is higher than priority of unary and binary operators Fix regressed parsing of paths with fn-like generic arguments r? @estebank
syntax: Better recovery for `$ty::AssocItem` and `ty!()::AssocItem` This PR improves on rust-lang#46788 covering a few missing cases. Fixes rust-lang#52307 Fixes rust-lang#53776 r? @estebank
cc #44970
Fixes #42187
r? @estebank