-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Provide context for missing comma in match arm and if statement without block #48338
Conversation
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
8d880f5
to
aabfe31
Compare
src/libsyntax/parse/parser.rs
Outdated
if let Some(sp) = fat_arrow_sp { | ||
// if cond => expr | ||
err.span_suggestion(sp, | ||
"only necessary in match arms, not before if blocks", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I found this confusingly phrased. Maybe this?
"only necessary with `match`, not `if`"
I'm also aiming to avoid jargon like "arms" and "blocks", which users may not be familiar with.
src/libsyntax/parse/parser.rs
Outdated
|
||
let expr = self.parse_expr_res(Restrictions::STMT_EXPR, None) | ||
.map_err(|mut err| { | ||
err.span_label(arrow_span, "while parsing the match arm starting here"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say:
"while parsing the `match` arm starting here"
13 | &None => 1 | ||
| -- - help: missing a comma here to end this match arm | ||
| | | ||
| while parsing the match arm starting here |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel like this is "heavy", but I'm not sure how to suggest an improvement yet. I wonder if just the suggestion would be enough?
error: expected one of `,`, `.`, `?`, `}`, or an operator, found `=>`
--> $DIR/missing-comma-in-match.rs:14:18
|
13 | &None => 1
| - help: maybe missing a comma here: `1,`
14 | &Some(2) => { 3 }
| ^^ expected one of `,`, `.`, `?`, `}`, or an operator here
error: aborting due to previous error
Would that be hard to add?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't.
src/libsyntax/parse/parser.rs
Outdated
}; | ||
let thn = self.parse_block().map_err(|mut err| { | ||
if let Some(sp) = fat_arrow_sp { | ||
// if cond => expr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there an issue about this or any other confirmation that this mistake is realistic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was an existing test case, but it is indeed a big corner case. I can remove the special diagnostic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I think just removing this would be for the better.
LGTM except for maybe removing special diagnostics for https://github.com/rust-lang/rust/pull/48338/files#r169751541 |
@bors r+ |
📌 Commit 3e730aa has been approved by |
☔ The latest upstream changes (presumably #48449) made this pull request unmergeable. Please resolve the merge conflicts. |
When finding: ```rust match &Some(3) { &None => 1 &Some(2) => { 3 } _ => 2 } ``` provide the following diagnostic: ``` error: expected one of `,`, `.`, `?`, `}`, or an operator, found `=>` --> $DIR/missing-comma-in-match.rs:15:18 | X | &None => 1 | -- - help: missing comma | | | while parsing the match arm starting here X | &Some(2) => { 3 } | ^^ expected one of `,`, `.`, `?`, `}`, or an operator here ```
When unnecessarily using a fat arrow after an if condition, suggest the removal of it. When finding an if statement with no block, point at the `if` keyword to provide more context.
3e730aa
to
d63d363
Compare
@bors r=petrochenkov |
📌 Commit d63d363 has been approved by |
💔 Test failed - status-appveyor |
⌛ Testing commit 24be75d with merge b78bf234740ed8f040c8546e21d509fc137cb468... |
@bors retry (rollup priority) |
💥 Test timed out |
Looks like this hasn't landed as a part of rollup. |
@bors r+ |
💡 This pull request was already approved, no need to approve it again.
|
📌 Commit 24be75d has been approved by |
…rochenkov Fixes rust-lang#47311. r? @nrc
…rochenkov Fixes rust-lang#47311. r? @nrc
…rochenkov Fixes rust-lang#47311. r? @nrc
When finding:
provide the following diagnostic:
When unnecessarily using a fat arrow after an if condition, suggest the
removal of it.
When finding an if statement with no block, point at the
if
keyword toprovide more context:
Fix #29586, fix #30035.