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

odd suggestion for comma typo #72253

Closed
ehuss opened this issue May 16, 2020 · 6 comments · Fixed by #72348
Closed

odd suggestion for comma typo #72253

ehuss opened this issue May 16, 2020 · 6 comments · Fixed by #72348
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code.

Comments

@ehuss
Copy link
Contributor

ehuss commented May 16, 2020

In an obscure situation with a typo (where there is a comma instead of a period), rustc will suggest replacing the comma with a semicolon, which will fail to compile.

let a = std::process::Command::new("echo")
    .arg("1")
    ,arg("2")
    .output();

Produces the following error and suggestion:

error: expected `;`, found ``,``
 --> src/main.rs:4:5
  |
4 |     ,arg("2")
  |     ^ help: change this to `;`: `;`

error[E0425]: cannot find function `arg` in this scope
 --> src/main.rs:4:6
  |
4 |     ,arg("2")
  |      ^^^ not found in this scope

error: aborting due to 2 previous errors

For more information about this error, try `rustc --explain E0425`.

Some observations:

  • This must be a let binding.
  • The method calls must be on separate lines (all on one line gives a different error).

I would expect this to provide the same error if it is all on one line ("expected one of ., ;, ?, or an operator"), possibly with a "maybe incorrect" suggestion to replace it with a period (maybe only if the following character is not whitespace?).

Note: Feel free to close this. This is really obscure, and I suspect it is not trivial to change, and it is fairly obvious what is wrong. I just thought it was strange, and I was doubly puzzled that the indentation mattered.

Meta

rustc 1.45.0-nightly (9912925 2020-05-10)

@ehuss ehuss added the C-bug Category: This is a bug. label May 16, 2020
@csmoe csmoe added A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` D-confusing Diagnostics: Confusing error or lint that should be reworked. labels May 16, 2020
@estebank
Copy link
Contributor

This is because the error recovery tries to figure out if you typoed something when trying to write ; to end an expression. This is probably easy to fix.

@estebank estebank added the D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code. label May 17, 2020
@chrissimpkins
Copy link
Member

@rustbot claim

@chrissimpkins
Copy link
Member

chrissimpkins commented May 18, 2020

Thoughts about the better approach?:

error: expected `;`, found `,`
 --> src/main.rs:4:5
  |
4 |     ,arg("2")
  |     ^ unexpected token
  |
help: change this to `.`, `;`, `?`, or an operator
 --> src/main.rs:4:5
  |
4 |     ,arg("2")
  |     ^

vs.

error: expected `;`, found `,`
 --> src/main.rs:4:5
  |
4 |     ,arg("2")
  |     ^ unexpected token, expected one of `.`, `;`, `?`, or an operator
  |

@chrissimpkins
Copy link
Member

#72348

@estebank
Copy link
Contributor

estebank commented May 19, 2020

Thoughts about the better approach?

I would prefer something closer to what we already see in other syntax errors:

error: expected one of `.`, `;`, `?`, or an operator, found `,`
 --> src/main.rs:4:5
  |
4 |     ,arg("2")
  |     ^ expected one of `.`, `;`, `?`, or an operator

We are not afraid of using vertical space to make errors clearer, but that vertical space still comes at a premium, as does verbosity. We try to lean towards the tersest possible output that can still convey the concept to the broadest set of people.

chrissimpkins added a commit to chrissimpkins/rust that referenced this issue May 26, 2020
confusing diagnostics, issue rust-lang#72253

add test for confusing error message, issue-72253


remove is_multiline check, refactor to self.expect(&token:Semi)


update issue-72253 tests


return Ok
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue May 27, 2020
Fix confusing error message for comma typo in multiline statement

Fixes rust-lang#72253.  Expands on the issue with a colon typo check.

r? @estebank

cc @ehuss
@bors bors closed this as completed in cbe7b90 May 27, 2020
@chrissimpkins
Copy link
Member

Merged. Thanks for a good learning issue @ehuss. And thanks for mentoring it @estebank. I really appreciate your time!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix` C-bug Category: This is a bug. D-confusing Diagnostics: Confusing error or lint that should be reworked. D-invalid-suggestion Diagnostics: A structured suggestion resulting in incorrect code.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants