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

Improve unexpected close and mismatch delimiter hint in TokenTreesReader #104012

Merged
merged 2 commits into from
Jan 28, 2023

Conversation

chenyukang
Copy link
Member

@chenyukang chenyukang commented Nov 5, 2022

Fixes #103882
Fixes #68987
Fixes #69259

The inner indentation mismatching will be covered by outer block, the new added function report_error_prone_delim_block will find out the error prone candidates for reporting.

@rustbot
Copy link
Collaborator

rustbot commented Nov 5, 2022

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 5, 2022
@chenyukang
Copy link
Member Author

chenyukang commented Nov 5, 2022

This is an issue I found from rustc dev experience, for example this file:
c3.txt

We have an extra '{' at line 1420, but the first error report from compiler is:

    --> e3.rs:2605:3
     |
38   | impl<'a, 'tcx> FnCtxt<'a, 'tcx> {
     |                                 - unclosed delimiter
...
110  |                                  sugg_span: Span| {
     |                                                   - this delimiter might not be properly closed...
...
240  |         };
     |         - ...as it matches this but it has different indentation
...
2605 | }
     |   ^

error: expected expression, found `let` statement
    --> e3.rs:1421:12
     |
1421 |         && let Some((fields, substs)) =
     |            ^^^

The first diagnostic's tip this delimiter might not be properly closed is misleading, this fix will remove it, then the second diagnostic help us narrowing the scope of root cause.

@chenyukang chenyukang marked this pull request as draft November 7, 2022 13:52
@chenyukang
Copy link
Member Author

Oops, find a better solution....

@chenyukang chenyukang force-pushed the yukang/fix-103882-deli-indentation branch from 83c0934 to 1a8e69a Compare November 7, 2022 16:07
@chenyukang chenyukang changed the title Only use indentation to detect mismatch delimiter when the line starting with delimiter TokenTreesReader now can find the correct mismatch delimiter pairs Nov 7, 2022
@chenyukang chenyukang marked this pull request as ready for review November 7, 2022 16:10
@chenyukang
Copy link
Member Author

@estebank
I find out a better solution for this issue 😁

@chenyukang
Copy link
Member Author

It will report out this, we have narrowed the scope of root-cause:

error: this file contains an unclosed delimiter
  --> src/test/ui/parser/deli-ident-issue-1.rs:27:65
   |
7  | impl dyn Demo {
   |               - unclosed delimiter
...
19 |         && let Some(c) = num {
   |                              - this delimiter might not be properly closed...
...
24 |     }
   |     - ...as it matches this but it has different indentation
...
27 | fn main() { } //~ ERROR this file contains an unclosed delimiter
   |                                                                 ^

@chenyukang
Copy link
Member Author

chenyukang commented Nov 7, 2022

But we still can not handle well for this kind of case:

fn f(i: u32, j: u32) {
    let res = String::new();
    let mut cnt = i;
    while cnt < j {
        write!&mut res, " ");
    }
}

Current error is:

error: unexpected closing delimiter: `}`
 --> bug3.rs:7:1
  |
1 | fn f(i: u32, j: u32) {
  |                      - this delimiter might not be properly closed...
...
6 |     }
  |     - ...as it matches this but it has different indentation
7 | }
  | ^ unexpected closing delimiter

error: mismatched closing delimiter: `)`

And this code:

async fn obstest() -> Result<> {
    let obs_connect = || -> Result<(), MyError) {
        async {
        }
    }

    if let Ok(version, scene_list) = obs_connect() {

    } else {

    }
}

report:

error: unexpected closing delimiter: `}`
  --> bug2.rs:12:1
   |
12 | }
   | ^ unexpected closing delimiter

error: mismatched closing delimiter: `)`
 --> bug2.rs:1:32
  |
1 | async fn obstest() -> Result<> {
  |                                ^ unclosed delimiter
2 |     let obs_connect = || -> Result<(), MyError) {
  |                                               ^ mismatched closing delimiter

error: aborting due to 2 previous errors

This is because we currently may use ')' to match '{' as a close delimiter.
To fix this need more work, I have a working branch and need more tweaks.

@wesleywiser
Copy link
Member

I'm not very familiar with the reporting code here so

r? @estebank

@estebank estebank assigned estebank and unassigned wesleywiser Nov 10, 2022
@chenyukang chenyukang force-pushed the yukang/fix-103882-deli-indentation branch from 19a7695 to 01babdc Compare November 11, 2022 11:30
@chenyukang
Copy link
Member Author

chenyukang commented Nov 11, 2022

@estebank

I made some more changes in another branch, I'm not sure whether it's Ok for merging it.
chenyukang@479fdb5

The main idea is reducing later errors when delimiter mismatch error happens.

It is somewhat common to ignore all but the first error when you have mismatched delimiters.

I read the source code, we exit when unexpected closing delimiter:

Err(self.close_delim_err(delim))

So for code like this:

}

fn main() {
    x = 1;
}

Compiler will exit with the only first error:

error: unexpected closing delimiter: `}`
 --> p/f.rs:2:1
  |
2 | }
  | ^ unexpected closing delimiter

error: aborting due to previous error

But we only report an error when this error: this file contains an unclosed delimiter happens:

In real dev practice, if the file is somehow very long, unclosed delimier will make parser totally undertand wrongly on AST, so it will give out many errors, such as the case in my comment: #104012 (comment).

My change also returning Err on this line, and we can avoid later noises.

I'm not sure whether we have any other special considertaion about this, seems there are some recovery code in TokenTreesReader, but actually we can not recovery from delimiter errors, because there are too many possible ways to fix a mismatch or unclosing error, compiler can only give proper diagnostics for user.

@estebank
Copy link
Contributor

Yeah, let's create a PR for chenyukang@479fdb5 and land it. Sadly delimiter recovery is quite hard and the current behavior isn't what's best for our users.

@chenyukang chenyukang force-pushed the yukang/fix-103882-deli-indentation branch from 01babdc to 1f1062c Compare January 18, 2023 15:49
@chenyukang
Copy link
Member Author

Conflict is fixed,
@estebank any more comments for this PR?

@petrochenkov petrochenkov self-assigned this Jan 25, 2023
@chenyukang chenyukang force-pushed the yukang/fix-103882-deli-indentation branch 2 times, most recently from d030739 to 18c2937 Compare January 26, 2023 05:39
@petrochenkov
Copy link
Contributor

I didn't read too much into the details of report_suspicious_mismatch_block, but the test diffs look fine.
@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2023
@chenyukang chenyukang force-pushed the yukang/fix-103882-deli-indentation branch from a1fc3d5 to 8f2ce1e Compare January 27, 2023 11:56
@petrochenkov
Copy link
Contributor

r=me after addressing #104012 (comment).

@chenyukang chenyukang force-pushed the yukang/fix-103882-deli-indentation branch 2 times, most recently from 7a3d767 to cd23323 Compare January 27, 2023 12:52
@chenyukang
Copy link
Member Author

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 27, 2023
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 27, 2023

📌 Commit cd23323 has been approved by petrochenkov

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 27, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 27, 2023
…indentation, r=petrochenkov

Improve unexpected close and mismatch delimiter hint in TokenTreesReader

Fixes rust-lang#103882
Fixes rust-lang#68987
Fixes rust-lang#69259

The inner indentation mismatching will be covered by outer block, the new added function `report_error_prone_delim_block` will find out the error prone candidates for reporting.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jan 28, 2023
…indentation, r=petrochenkov

Improve unexpected close and mismatch delimiter hint in TokenTreesReader

Fixes rust-lang#103882
Fixes rust-lang#68987
Fixes rust-lang#69259

The inner indentation mismatching will be covered by outer block, the new added function `report_error_prone_delim_block` will find out the error prone candidates for reporting.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 28, 2023
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104012 (Improve unexpected close and mismatch delimiter hint in TokenTreesReader)
 - rust-lang#104252 (Stabilize the const_socketaddr feature)
 - rust-lang#105524 (Replace libc::{type} with crate::ffi::{type})
 - rust-lang#107096 (Detect references to non-existant messages in Fluent resources)
 - rust-lang#107355 (Add regression test for rust-lang#60755)
 - rust-lang#107384 (Remove `BOOL_TY_FOR_UNIT_TESTING`)
 - rust-lang#107385 (Use `FallibleTypeFolder` for `ConstInferUnifier` not `TypeRelation`)
 - rust-lang#107391 (rustdoc: remove inline javascript from copy-path button)
 - rust-lang#107398 (Remove `ControlFlow::{BREAK, CONTINUE}`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e3048c7 into rust-lang:master Jan 28, 2023
@rustbot rustbot added this to the 1.69.0 milestone Jan 28, 2023
flip1995 pushed a commit to flip1995/rust that referenced this pull request Feb 10, 2023
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#104012 (Improve unexpected close and mismatch delimiter hint in TokenTreesReader)
 - rust-lang#104252 (Stabilize the const_socketaddr feature)
 - rust-lang#105524 (Replace libc::{type} with crate::ffi::{type})
 - rust-lang#107096 (Detect references to non-existant messages in Fluent resources)
 - rust-lang#107355 (Add regression test for rust-lang#60755)
 - rust-lang#107384 (Remove `BOOL_TY_FOR_UNIT_TESTING`)
 - rust-lang#107385 (Use `FallibleTypeFolder` for `ConstInferUnifier` not `TypeRelation`)
 - rust-lang#107391 (rustdoc: remove inline javascript from copy-path button)
 - rust-lang#107398 (Remove `ControlFlow::{BREAK, CONTINUE}`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 28, 2023
…, r=petrochenkov

Exit when there are unmatched delims to avoid noisy diagnostics

From rust-lang#104012 (comment)
r? `@petrochenkov`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 1, 2023
…, r=petrochenkov

Exit when there are unmatched delims to avoid noisy diagnostics

From rust-lang#104012 (comment)
r? ``@petrochenkov``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
7 participants