Distinguish delim kind to decide whether to emit unexpected closing delimiter #138554
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
rustbot has assigned @compiler-errors. Use |
rustbot
added
S-waiting-on-review
|
r? compiler |
|
I changed this part last year, will review this PR later. r? @chenyukang |
chenyukang
reviewed
Apr 11, 2025
tests/ui/parser/issues/issue-67377-invalid-syntax-in-enum-discriminant.stderr
Outdated
Show resolved
Hide resolved
chenyukang
requested changes
Apr 11, 2025
rustbot
added
S-waiting-on-author
chenyukang
reviewed
Apr 11, 2025
chenyukang
reviewed
Apr 11, 2025
|
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:
|
|
@rustbot author |
|
Reminder, once the PR becomes ready for a review, use |
|
Thanks, I will revise it soon! |
xizheyin
changed the title
|
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 In the third commit, I make a distinction between the different delimiters before triggering a |
xizheyin
commented
Apr 12, 2025
rustbot
added
S-waiting-on-review
chenyukang
reviewed
Apr 15, 2025
|
I've combined them into one line so it's more aesthetically pleasing. Since the |
This comment has been minimized.
This comment has been minimized.
estebank
reviewed
Jun 26, 2025
|
I marked potential matched unclosed delimiter when emit |
|
@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. |
|
That makes sense. Different situations are treated differently. I will investigate further. |
estebank
reviewed
Jul 1, 2025
estebank
reviewed
Jul 1, 2025
estebank
reviewed
Jul 1, 2025
|
☔ The latest upstream changes (presumably #143526) made this pull request unmergeable. Please resolve the merge conflicts. |
Signed-off-by: xizheyin <[email protected]>
xizheyin
force-pushed
the
issue-138401
branch
2 times, most recently
from
1c4382c to
ce088f0
Compare
estebank
reviewed
Jul 8, 2025
|
Thanks for the guidance, I've learned much, this diagnosis involves a lot of regression testing so it's a little harder to deal with. |
chenyukang
reviewed
Jul 9, 2025
chenyukang
reviewed
Jul 9, 2025
chenyukang
reviewed
Jul 9, 2025
| error: unexpected closing delimiter: `}` | ||
| --> $DIR/issue-68987-unmatch-issue-3.rs:8:1 | ||
| | | ||
| LL | fn f(i: u32, j: u32) { | ||
| | - the closing `}` may be intended to match this |
There was a problem hiding this comment.
this new hint is unnecessary, how can we remove it?
since the root cause is only a missing '(', which we have already reported.
There was a problem hiding this comment.
estebank suggest me to add some hint. the closing ) may match this {, so we can get the general scope of how to add [. But I understood it. It should be "the mismatched closing ) may be meant for this opening delimiter".
See #138554 (comment) #138554 (comment)
There was a problem hiding this comment.
even if we change the words into the mismatched closing ) may be meant for this opening delimiter, it will also make a similar confusion like the origin issue #138401 described?
user will be confused why ')' meant for this opennning '{' ?
There was a problem hiding this comment.
this remind me maybe we should reconsider whether the original issue is worth to be resolved, the original hint in the issue description:
error: mismatched closing delimiter: `)`
--> ...\test.rs:1:27
|
1 | pub fn foo(x: i64) -> i64 {
| ^ unclosed delimiter
2 | x.abs)
| ^ mismatched closing delimiter
seems somehow confused, but it helps narrow the scope of span, if there are multiple nested scope, I think it's helpful in those scenarios.
chenyukang
reviewed
Jul 9, 2025
| error: unexpected closing delimiter: `}` | ||
| --> $DIR/issue-68987-unmatch-issue-2.rs:14:1 | ||
| | | ||
| LL | async fn obstest() -> Result<> { | ||
| | - the closing `}` may be intended to match this |
There was a problem hiding this comment.
similar one.
it's better to remove it.
chenyukang
reviewed
Jul 9, 2025
tests/ui/parser/issues/issue-67377-invalid-syntax-in-enum-discriminant.stderr
Show resolved
Hide resolved
chenyukang
reviewed
Jul 9, 2025
| LL | V = [PhantomData; { [ () ].len() ].len() as isize, | ||
| | - missing open `[` for this delimiter | ||
| ... | ||
| LL | mod b { | ||
| | - unclosed delimiter | ||
| LL | enum Bug { |
There was a problem hiding this comment.
this hint may makes user try to add a '}', while the root cause is we should remove the { where old diagnostic reported.
There was a problem hiding this comment.
This is because we merged the mismatched diagnostic into another diagnostic. The old diagnostic gave the wrong information because the real reason was the missing {} in { [].len() }. However, I think this PR is meant to address the duplicate error message, this false message can be addressed in another PR. :)
|
I need to clarify the direction of the modification. |
per my understanding, the issue #138554 (comment) refers to is introduced by the code in this PR, not a old one? |
|
Oh! That makes sense. This seems to require finer control. |
chenyukang
reviewed
Jul 10, 2025
Signed-off-by: xizheyin <[email protected]>
xizheyin
commented
Jul 10, 2025
There was a problem hiding this comment.
There should be no regression this time.
The only thing I did was this: if the missing open hint is reported in report_missing_open_delim, it will not generate a separate mismatched error, reducing a lot of confusing errors. Meanwhile, this doesn't eat up the information in the mismatched error.
chenyukang
reviewed
Jul 10, 2025
| | - missing open `[` for this delimiter | ||
| | - - missing open `[` for this delimiter | ||
| | | | ||
| | the last unclosed delimiter before unmatched delimiter |
There was a problem hiding this comment.
it's more clear only report as unclosed delimiter as before?
this is the mismatched scenario, means we have the same number openning delimiter and closing delimiter, but they are just not matched, for example:
({)) we have 2 open delimiter and 2 close delimiter, but it's a mismatched pair here
[{[)}] we have 3 open delimiter and 3 close, but there is a mismatchd pair
so in mismatch scenario, seems we should only report mismatch as before.
only for the scenario, that openning delimiter and closing delimiter is not same, for example:
[)] here we have 1 open, and 2 close delimiter, it's safe to report missing a `(`.
There was a problem hiding this comment.
the issue described in #138401 is a special scenario that we are missing a openning delimiter:
pub fn foo(x: i64) -> i64 {
x.abs)
}it's
{)}
There was a problem hiding this comment.
Thank you, I understand it. I'll try to do it only in this case.
|
The job Click to see the possible cause of the failure (guessed by this bot) |
chenyukang
reviewed
Jul 10, 2025
| LL | #![w,) | ||
| | ^ ^ mismatched closing delimiter | ||
| | | | ||
| | unclosed delimiter |
There was a problem hiding this comment.
this is a typical mismatched one,
the right fix maybe [w,] or (w,),
it depends on the specific scenario.
but the new current hint is suggesting the second fix or adding another extra (?
seems the old diagnostics is better.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #138401