clarify why we're suggesting removing semicolon after braced items #51955
Conversation
Previously (issue rust-lang#46186, pull-request rust-lang#46258), a suggestion was added to remove the semicolon after we fail to parse an item, but issue rust-lang#51603 complains that it's still insufficiently obvious why. Let's add a note. Resolves rust-lang#51603.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
r? @oli-obk |
| _ => None, | ||
| }; | ||
| if let Some(name) = previous_item_kind_name { | ||
| err.help(&format!("{} declarations are not followed by a semicolon", |
There was a problem hiding this comment.
This is of course very opinionated, but what do you think about replacing the expected item, found ; message with this message?
There was a problem hiding this comment.
I'm negatively disposed. We're trying to make inferences about the user's state of mind and helpfully remind them how the language works (in contrast to other languages that we think they might be confusing us with). It's a good magical thing to do, but I think it would be overstepping the mandate of the parser to make it the top-line message rather than an ancillary note.
There was a problem hiding this comment.
I think it's a question of numbers. If most ppl add semicolons because they think the definitions need one, then the error should reflect that. Sadly we don't have numbers...
There was a problem hiding this comment.
And the issue was about changing the error message ;)
There was a problem hiding this comment.
I think it's a question of numbers.
While I'm happy to defer to the authority of you or other team members, I don't think numbers are very relevant to the distinction I was trying to draw in my previous comment.
For a perhaps clearer-cut case, consider what we emit when someone tries to destructure a tuple using comma-separated values and no parentheses (e.g., let a, b = (1, 2);). In that case, I think it's appropriate that the top-line error is "unexpected , in pattern", and only the ancillary suggestion suggests adding parentheses: the former message says what went wrong with parsing (there's nothing in the grammar that lets you put a comma there) while being agnostic about what could have motivated someone to write that code, and then, that having been established, we go on to provide a helpful suggestion that maybe you meant to use parentheses. Even if (as seems likely), among people who encounter this error, a solid majority of them geniunely wanted a tuple-unpacking and didn't think parens were necessary, and only a tiny minority just happened to have their fingers slip and insert a comma for no reason, it still makes sense to have both messages in that order, because they're serving different functions: flagging the parsing error is distinct from pointing out what misunderstanding of the language might have motivated that error, and the former should come first because it's an analytic truth, whereas the latter is fundamentally only a guess, though a very high-probability guess it may be. I think the same phenomenon is going on here.
There was a problem hiding this comment.
Well we definitely err on the safe side by keeping the "expected,found" error. You're right that it would be a major change to diagnostics and should not be done lightly
|
Ping from triage @zackmdavis we have heard from you for a while, any progress on this PR? |
|
@bors r+ |
|
📌 Commit db2f3d7 has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - status-appveyor, status-travis |
Previously (issue #46186, pull-request #46258), a suggestion was added
to remove the semicolon after we fail to parse an item, but issue #51603
complains that it's still insufficiently obvious why. Let's add a note.
Resolves #51603.