Skip to content

Diagnostic for early closing delimiter incorrectly blames empty block #98601

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

Closed
mcy opened this issue Jun 28, 2022 · 6 comments · Fixed by #116034
Closed

Diagnostic for early closing delimiter incorrectly blames empty block #98601

mcy opened this issue Jun 28, 2022 · 6 comments · Fixed by #116034
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-parser Area: The lexing & parsing of Rust source code to an AST E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@mcy
Copy link
Contributor

mcy commented Jun 28, 2022

See: https://godbolt.org/z/rPxvYMf1h

This code:

fn foo() {
  match 0 {
    _ => {}
  }
  if foo
  }
}

produces

error: unexpected closing delimiter: `}`
 --> <source>:7:1
  |
3 |     _ => {}
  |          -- block is empty, you might have not meant to close it
...
7 | }
  | ^ unexpected closing delimiter

error: aborting due to previous error

Compiler returned: 1

on latest stable (1.61.0). @estebank

@mcy mcy added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jun 28, 2022
@mcy mcy changed the title Diagnostic for missing delimiter is completely wrong Diagnostic for early closing delimiter incorrectly blames empty block Jun 28, 2022
@compiler-errors
Copy link
Member

Maybe we could form a heuristic around tokens that we know should eventually be followed by { -- if, match, fn, etc.

If we see one of those, (then some random other tokens), then we see }, then that would be a suspicious rbrace which we could point out later..

e.g. if we see if a b c d e } that triggers the heuristic, but not if a b c d e { }

@estebank estebank added A-parser Area: The lexing & parsing of Rust source code to an AST D-confusing Diagnostics: Confusing error or lint that should be reworked. labels Jun 29, 2022
@estebank
Copy link
Contributor

The problem is that the error happens in the lexer, not the parser. Ideally we'd rework the lexer to have a token stream mode of operation instead of a token tree mode of operation, only for code that is being passed to the parser (i.e., not coming from a macro).

@sgued
Copy link
Contributor

sgued commented Sep 1, 2022

This is particularly confusing when the block is empty points to a macro:

fn main() {
    todo!();
}

fn other(_: i32)) {}

gives

error: unexpected closing delimiter: `)`
 --> src/main.rs:5:17
  |
2 |     todo!();
  |          -- block is empty, you might have not meant to close it
...
5 | fn other(_: i32)) {}
  |                 ^ unexpected closing delimiter

on 1.65.0-nightly (2022-08-31 9243168)

@chenyukang
Copy link
Member

the output now changed to:

error: unexpected closing delimiter: `}`
 --> <source>:7:1
  |
1 | fn foo() {
  |          - this delimiter might not be properly closed...
2 |   match 0 {
3 |     _ => {}
  |          -- block is empty, you might have not meant to close it
...
6 |   }
  |   - ...as it matches this but it has different indentation
7 | }
  | ^ unexpected closing delimiter

error: aborting due to previous error

Do you think the block is empty, you might have not meant to close it should be removed?

@chenyukang
Copy link
Member

This is particularly confusing when the block is empty points to a macro:

fn main() {
    todo!();
}

fn other(_: i32)) {}

gives

error: unexpected closing delimiter: `)`
 --> src/main.rs:5:17
  |
2 |     todo!();
  |          -- block is empty, you might have not meant to close it
...
5 | fn other(_: i32)) {}
  |                 ^ unexpected closing delimiter

on 1.65.0-nightly (2022-08-31 9243168)

This is changed to:

error: unexpected closing delimiter: `)`
 --> <source>:5:17
  |
1 | fn main() {
  |           - this opening brace...
2 |     todo!();
3 | }
  | - ...matches this closing brace
4 |
5 | fn other(_: i32)) {}
  |                 ^ unexpected closing delimiter

it seems more helpful now.

@estebank
Copy link
Contributor

Let's close this ticket after adding these two cases to the test suite then. I'm sure there are outstanding issues in this area, but we can rest assuredhope that we'll get tickets for those more specific cases, while the tests keep us from regressing silently (hopefully, lets make sure the tests are fully annotated so its harder to do that by accident).

@estebank estebank added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. E-help-wanted Call for participation: Help is requested to fix this issue. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue and removed D-confusing Diagnostics: Confusing error or lint that should be reworked. labels Sep 15, 2023
@bors bors closed this as completed in d5e7df3 Sep 22, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Sep 22, 2023
Rollup merge of rust-lang#116034 - chenyukang:yukang-98601-add-ui-testcase, r=estebank

add UI test for delimiter errors

Fixes rust-lang#98601
from rust-lang#98601 (comment)
r? `@estebank`
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-parser Area: The lexing & parsing of Rust source code to an AST E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-help-wanted Call for participation: Help is requested to fix this issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. S-has-mcve Status: A Minimal Complete and Verifiable Example has been found for this issue 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.

5 participants