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.

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

Consider removing a non-existing semicolon #81098

Closed
adlerd opened this issue Jan 16, 2021 · 0 comments · Fixed by #81407
Closed

Consider removing a non-existing semicolon #81098

adlerd opened this issue Jan 16, 2021 · 0 comments · Fixed by #81407
Labels
A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@adlerd
Copy link

adlerd commented Jan 16, 2021

I tried this code:

fn wat() -> impl core::fmt::Display {
    fn why() {}
}

I expected to see this happen: An error explaining that () does not impl Display (although a different message would be nice, cf #54771).

Instead, this happened: I got that error, but I also got a suggestion to remove a semicolon. As you can see, this program does not contain any semicolons to remove.

error[E0277]: `()` doesn't implement `std::fmt::Display`
 --> src/lib.rs:1:13
  |
1 | fn wat() -> impl core::fmt::Display {
  |             ^^^^^^^^^^^^^^^^^^^^^^^ `()` cannot be formatted with the default formatter
2 |     fn why() {}
  |               - consider removing this semicolon
  |
  = help: the trait `std::fmt::Display` is not implemented for `()`
  = note: in format strings you may be able to use `{:?}` (or {:#?} for pretty-print) instead
rustc 1.49.0 (e1884a8e3 2020-12-29)
binary: rustc
commit-hash: e1884a8e3c3e813aada8254edfa120e85bf5ffca
commit-date: 2020-12-29
host: x86_64-unknown-linux-gnu
release: 1.49.0

Of course, I understand why the suggestion exists, but rustc should probably check to see if a semicolon exists before suggesting to remove it. For bonus points, rustc should find the semicolon correctly in this slightly modified program. (Currently it still points at why's }.)

fn wat() -> impl core::fmt::Display {
    5i32;
    fn why() {}
}

Here is perhaps a worse variant of the same issue:

fn wat() -> impl core::fmt::Display {
    5i32;
    const WHY: () = ();
}

Here the suggestion is to remove the semicolon after the WHY item, which is now a "syntactically correct suggestion" so to speak (i.e., the "remove a semicolon" is now pointing at an actual semicolon)... but applying it does not result in a syntactically correct program.

@adlerd adlerd added the C-bug Category: This is a bug. label Jan 16, 2021
@jonas-schievink jonas-schievink added A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 16, 2021
JohnTitor added a commit to JohnTitor/rust that referenced this issue Jan 26, 2021
Refine "remove semicolon" suggestion in trait selection

Don't suggest it if the last statement doesn't have a semicolon

Fixes rust-lang#81098

See also rust-lang#54771 for why this suggestion was added
@bors bors closed this as completed in 8ddc1c8 Jan 26, 2021
zaharidichev pushed a commit to zaharidichev/rust that referenced this issue Mar 15, 2021
Don't suggest it if the last statement doesn't have a semicolon

Fixes rust-lang#81098

See also rust-lang#54771 for why this suggestion was added
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants