Distinguish delim kind to decide whether to emit unexpected closing delimiter

[xizheyin]

Fixes #138401

[rustbot]

r? @compiler-errors

rustbot has assigned @compiler-errors.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[compiler-errors]

r? compiler

[chenyukang]

I changed this part last year, will review this PR later.

r? @chenyukang

[chenyukang]

I think this solution introduces some kind of regression on other scenarios(such as the cases I commented).

Maybe we should consider this as a special corner case and provide a special diagnostic for it, it is the pattern that one delimiter is missing the openning delimiter(it's in the inner part), except for this everything is ok, so we should suggest something like "missing openning delimiter ....".

such as:

{ ... {. ) ... }...} missing a inner (
{ ... [ ], ] ...} missing a inner [

[chenyukang]

@rustbot author

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[xizheyin]

Thanks, I will revise it soon!

[xizheyin]

This seems like quite a lot of changes, I'll comment in the code to make it easier to REVIEW.

In the first commit, I added the test.

In the second commit I did some clean. I changed a variable name, the original name was inaccurate, I used open_delimiters instead of open_braces for identifying blocks, one should compare spans, not just brace'{'.

In the third commit, I make a distinction between the different delimiters before triggering a unexpected closing delimiter error. But I found that missing open delim note is what triggers in unexpected closing delimiter error.
Therefore, to emit missing open delim note, I also moved missing open delim note to
mismatched closing delimiter error, which can be emitted without delaying to
unexpected closing delimiter error.
And I moved function make_unclosed_delims_error
to src/lexer/diagnostics.rs for clean. And some other clean.

[xizheyin]

I've combined them into one line so it's more aesthetically pleasing.

mismatched closing delimiter, may missing open `(`

Since the mismatched closing delimiter note and the unclosed delimiter note are related, it may be the better to keep it.

[chenyukang]

this is worse too, we are not sure we should replace the left { to ( or replace the right ) to }.

so, the better way is do not suggest any direction.

I think we should only suggest missing a open(or closing) delimiter in the scenario that if we added a open(or closing) delimiter, there will be no error.

[xizheyin]

I don't have a good way to judge whether adding the open delimiter is a better way. As in the previous implementation, there will always be some false positives.

If we have to make a balance, should we make suggestions as comprehensively as possible, or would we rather not make suggestions and minimize false positives?

Maybe I need to refactor this code more.

[chenyukang]

it's better to suggest like this:

missing open `(` for this delimiter

as the origin issue reported wrote in #138401

[chenyukang]

I have a look at the current code, current output is:

error: mismatched closing delimiter: `)`
 --> ...\test.rs:1:27
  |
1 | pub fn foo(x: i64) -> i64 {
  |                           ^ unclosed delimiter
2 |     x.abs)
  |          ^ mismatched closing delimiter

error: unexpected closing delimiter: `}`
 --> ...\test.rs:3:1
  |
2 |     x.abs)
  |          - missing open `(` for this delimiter
3 | }
  | ^ unexpected closing delimiter

error: aborting due to 2 previous errors

The first diagnostic message is reported at here(you moved it into lexer, it is ok):
https://github.com/chenyukang/rust/blob/c8f94230282a8e8c1148f3e657f0199aad909228/compiler/rustc_parse/src/parser/mod.rs#L1766-L1785

The second diagnostic is reported at here:
https://github.com/chenyukang/rust/blob/c8f94230282a8e8c1148f3e657f0199aad909228/compiler/rustc_parse/src/lexer/diagnostics.rs#L61

To resolve the issue in #138401, can we just remove the first diagnostic if we could report the second one? or at least reorder these two diagnostics, which could also reduce the confusion.

[xizheyin]

This part of the code conflicted with master, so I submitted the PR #140147 first to do some cleanup, otherwise the code for this PR would have been rather messy. @chenyukang

[wesleywiser]

@xizheyin now that #140147 has landed, does this PR need to be rebased?

[xizheyin]

Yes, I haven't had time to deal with this pr yet. Thanks for reminding me.😁

[chenyukang]

Yes, I haven't had time to deal with this pr yet. Thanks for reminding me.😁

They review some old PRs every week in the weekly meeting. If you don't plan to move forward with this PR for the time being, you can close it first and reopen it later.

This can prevent this PR from appearing in the review list.

[xizheyin]

yeah I will take it later. Let me remove it out of the review list first
@rustbot author

[xizheyin]

@rustbot ready

[xizheyin]

When missing open xxx for this delimiter is reported in the unexpected closing delimiter: xxx error, I remove the corresponding unmatched_delims so that make_unclosed_delims_error doesn't get double report.

[estebank]

First of all, thank you for all your changes and your patience to improve this output!

Do we have any test left where the lexer diagnostic is still emitted?

[xizheyin]

Yes, such as
https://github.com/xizheyin/rust/blob/issue-138401/tests/ui/parser/issues/issue-58856-2.stderr
https://github.com/xizheyin/rust/blob/issue-138401/tests/ui/parser/issues/issue-58856-1.stderr
https://github.com/xizheyin/rust/blob/issue-138401/tests/ui/parser/unclosed-delimiter-in-dep.stderr

I found 27 results after searching.

[estebank]

It might make sense to also collect the unclosed opening delimiter to be included in the later error:

error: unexpected closing delimiter: `}`
  --> $DIR/issue-68987-unmatch-issue-2.rs:14:1
   |
LL | async fn obstest() -> Result<> {
   |                                ^ the mismatched closing `)` matches this `{`
LL |     let obs_connect = || -> Result<(), MyError) {
   |                                               - missing open `(` for this delimiter
...
LL | }
   | ^ unexpected closing delimiter

[xizheyin]

I marked potential matched unclosed delimiter when emit unexpected closing delimiter in the new commit.
But I am not sure it's appropriate, I think it has the potential to introduce noise into complex samples with lots of mixed delimiters.

[estebank]

@xizheyin I see what you mean. I think some of the changes are a net positive, and some are not. We could try to get fancy and look at the number of unclosed delimiters we have in order to specialize the output. For cases where there's only a handful of mistakes, showing all of them is useful (like when someone deletes a line in the middle of a file). When there are "too many", we can only tell people "try to fix things, this is very broken". I'll try to take a look at this in more depth later with more specific feedback, but if you have the spare time to experiment with this idea, I think it can result on something quite usable.

[xizheyin]

That makes sense. Different situations are treated differently. I will investigate further.

[estebank]

This message has a typo, and I'd like to tweak the wording here a bit, I just don't have a good idea for what the ideal text would be in context.

[estebank]

Maybe something "the mismatched closing {token_str} may be meant for this opening delimiter"?

[estebank]

This should make the .stderr output a bit easier to read:

Suggested change

	#![c={#![c[)x
	//~ ERROR this file contains an unclosed delimiter
	//~v ERROR this file contains an unclosed delimiter
	#![c={#![c[)x

[estebank]

I feel like we should still have a label pointing at this opening {, stating that the closing ] matches it.