struct pattern parsing and diagnostic tweaks
#47242
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
petrochenkov
added
the
S-waiting-on-review
estebank
force-pushed
the
issue-15980
branch
2 times, most recently
from
9f83d48 to
515f216
Compare
| | ------------- while parsing this struct | ||
| 19 | //~^ NOTE while parsing this struct | ||
| 20 | return | ||
| | ^^^^^^ expected identifier, found keyword |
There was a problem hiding this comment.
This was the only line originally being emitted for this input. Now it correctly complains about missing => below, as well as continue to type checking.
estebank
force-pushed
the
issue-15980
branch
3 times, most recently
from
1cfe680 to
0098ed8
Compare
- Recover from struct parse error on match and point out missing match
body.
- Point at struct when finding non-identifier while parsing its fields.
- Add label to "expected identifier, found {}" error.
| token_descr)); | ||
| } else { | ||
| err.span_label(self.span, "expected identifier"); | ||
| } |
There was a problem hiding this comment.
I don't see much point in a label that duplicates the primary message exactly.
There was a problem hiding this comment.
There have been (off hand) talks about RLS and other tools being able to reliably show only the error message or the label and although the message works better in isolation, the primary span (should) works better when pointing at the code, and should be shorter as they don't have to duplicate the code's context.
I also want to accommodate people that we might have been "training" to ignore the error message in lieu of the primary label (I know I have that behavior at times).
Because of those reasons, I've been trying to move away from errors that have highlighted spans devoid of labels.
There was a problem hiding this comment.
Ok, sounds reasonable.
| @@ -2,7 +2,7 @@ error: expected identifier, found keyword `true` | |||
| --> $DIR/issue-44406.rs:18:10 | |||
| | | |||
| 18 | foo!(true); //~ ERROR expected type, found keyword | |||
| | ^^^^ | |||
| | ^^^^ expected identifier, found keyword | |||
There was a problem hiding this comment.
This case looks interesting.
We don't actually expect an identifier here, why does the note appears?
There was a problem hiding this comment.
A previous commit in a separate PR actually removed this incorrect issue (mostly by accident), but it broke too many things to be mergeable.
The macro is expecting a token tree as an argument, so I'm sure it's coming from that. Both cases should probably be pointing at $rest instead, like macro errors that happen after parsing.
There was a problem hiding this comment.
We don't actually expect an identifier here
My eyes deceive me, we expect an identifier (but this is not related to tt).
First we expect a type, but then we try to do recovery in parse_assoc_op_cast and get one more message (not entirely desirable) about expecting an identifier.
So, it's ok, not related to this PR.
| @@ -2056,7 +2085,7 @@ impl<'a> Parser<'a> { | |||
| self.bump(); | |||
| Ok(Ident::with_empty_ctxt(name)) | |||
| } else { | |||
| self.parse_ident() | |||
| self.parse_ident_common(false) | |||
There was a problem hiding this comment.
I don't think we should fix this in parse_ident.
The condition is "doesn't start like a struct literal", not "is_reserved_ident".
#15980 just happens to use return as something that isn't a struct literal, but the block can start with any other statement and the fix won't work.
I'd rather move the fix into parsing code for struct literals and used "not a non-reserved ident followed by colon" as a condition.
There was a problem hiding this comment.
Could you provide incorrect code that you'd expect to fail to be addressed by this PR? I'm confused, as currently (and after this PR) other types of statements will have the expected behavior (continue and point out the lack of =>).
There was a problem hiding this comment.
Also, for the record, I'm slowly going through the parser looking for places where the span of an error is context dependent on the current state parser, in this case for example, the reason a non reserved ident was found is because we're parsing a struct, in the case where
fn main() {
if 3 == {
4
}
}is expecting { is because it wants to parse the block following the 3 == { 4 } if condition, and closer to this case, the reason "- expected one of ., =>, ?, or an operator here" (the actual correct error to be displayed with the original issue's code) is displayed here is because it is expecting a block for the Err(ref e) if e.kind == io::EndOfFile { return } match arm.
Do you feel this is too heavy handed? The main advantage I see of this is 1) these are easy enough mistakes to make and 2) the diagnostic doesn't show you enough context as it is now to identify why the problem is happening in the first place.
There was a problem hiding this comment.
I thought we get a fatal error from Struct { non_ident } from which we cannot recover, but after rereading it turns out not to be the case.
And ability to not recover from a keyword in parse_ident seems usable in other contexts too.
petrochenkov
added
S-waiting-on-author
|
@bors r+ |
|
📌 Commit d17e38f has been approved by |
petrochenkov
added
S-waiting-on-bors
`struct` pattern parsing and diagnostic tweaks
- Recover from struct parse error on match and point out missing match
body.
- Point at struct when finding non-identifier while parsing its fields.
- Add label to "expected identifier, found {}" error.
Fix #15980.
|
☀️ Test successful - status-appveyor, status-travis |
body.
Fix #15980.