Skip to content
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.

Sign up for GitHub

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

clarify why we're suggesting removing semicolon after braced items #51955

Merged
merged 1 commit into from
Jul 8, 2018
Merged

clarify why we're suggesting removing semicolon after braced items #51955

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions src/libsyntax/parse/parser.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6132,6 +6132,22 @@ impl<'a> Parser<'a> {
err.span_suggestion_short_with_applicability(
self.span, msg, "".to_string(), Applicability::MachineApplicable
);
if !items.is_empty() { // Issue #51603
let previous_item = &items[items.len()-1];
let previous_item_kind_name = match previous_item.node {
// say "braced struct" because tuple-structs and
// braceless-empty-struct declarations do take a semicolon
ItemKind::Struct(..) => Some("braced struct"),
ItemKind::Enum(..) => Some("enum"),
ItemKind::Trait(..) => Some("trait"),
ItemKind::Union(..) => Some("union"),
_ => None,
};
if let Some(name) = previous_item_kind_name {
err.help(&format!("{} declarations are not followed by a semicolon",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is of course very opinionated, but what do you think about replacing the expected item, found ; message with this message?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And the issue was about changing the error message ;)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@oli-obk

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

name));
}
}
} else {
err.span_label(self.span, "expected item");
}
Expand Down
2 changes: 2 additions & 0 deletions src/test/ui/issue-46186.stderr
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ error: expected item, found `;`
|
LL | }; //~ ERROR expected item, found `;`
| ^ help: consider removing this semicolon
|
= help: braced struct declarations are not followed by a semicolon

error: aborting due to previous error