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

Do not exclusively suggest ; when , is also a choice #98796

Merged
merged 1 commit into from
Aug 4, 2022

Conversation

compiler-errors
Copy link
Member

Fixes #96791

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 2, 2022
@rust-highfive
Copy link
Collaborator

r? @michaelwoerister

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 2, 2022
@compiler-errors compiler-errors changed the title Do not suggest ; when , is also a choice Do not exclusively suggest ; when , is also a choice Jul 2, 2022
@michaelwoerister
Copy link
Member

I'm not quite sure this is an improvement in all cases. E.g. in the regression test:

fn main() {
    let v = [1
    2];
    //~^ ERROR expected one of `,`, `.`, `;`, `?`, `]`, or an operator, found `2`
}

Does suggesting ? make sense, for example?

I think this is something for the diagnostics team to decide.
r? rust-lang/diagnostics

@michaelwoerister
Copy link
Member

Also: Thanks for the PR, @compiler-errors! 😀

@compiler-errors
Copy link
Member Author

compiler-errors commented Jul 4, 2022

Does suggesting ? make sense, for example?

It doesn't make sense type-wise, but this is parsing, so it's as valid as suggesting ? anywhere else, imo.

We could limit this to just mention , and ;, but then we'd probably get another bug report like #96791 that (rightfully) says "the ? token is also valid here, why aren't we reporting it?"

@compiler-errors compiler-errors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jul 23, 2022
@compiler-errors
Copy link
Member Author

@estebank do you have some time to look at this PR, or should I re-roll?

@compiler-errors
Copy link
Member Author

r? diagnostics

@rust-highfive rust-highfive assigned oli-obk and unassigned estebank Aug 3, 2022
@estebank
Copy link
Contributor

estebank commented Aug 4, 2022

I have to say, I'm not sure this is a net positive, but it's also not a net negative given the output changes are not really worse?

@bors r+

@bors
Copy link
Contributor

bors commented Aug 4, 2022

📌 Commit 9d5c74e has been approved by estebank

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 Aug 4, 2022
@compiler-errors
Copy link
Member Author

@estebank: My thought process is that when , is one of the expected tokens alongside ;, then we're current parsing [ ..., and the heuristic of "semicolon is needed to end the statement" (which is what I think this logic is trying to account for) is probably incorrrect.

I could change this if you'd like to say something like ; or , since out of all the tokens, those are probably the most expected as I mentioned above.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 4, 2022
…iaskrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#98796 (Do not exclusively suggest `;` when `,` is also a choice)
 - rust-lang#99772 (Re-enable submodule archive downloads.)
 - rust-lang#100058 (Suggest a positional formatting argument instead of a captured argument)
 - rust-lang#100093 (Enable unused_parens for match arms)
 - rust-lang#100095 (More EarlyBinder cleanups)
 - rust-lang#100138 (Remove more Clean trait implementations)
 - rust-lang#100148 (RustWrapper: update for TypedPointerType in LLVM)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f6ea143 into rust-lang:master Aug 4, 2022
@rustbot rustbot added this to the 1.64.0 milestone Aug 4, 2022
@estebank
Copy link
Contributor

estebank commented Aug 5, 2022

@compiler-errors honestly, it's fine. What we'd ideally do is try to parse the rest of the expression and start parsing the following node to see which token would have helped, but to do that we need a lot of machinery that we don't currently have.

@compiler-errors compiler-errors deleted the no-semi-if-comma branch August 11, 2023 20:11
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
Development

Successfully merging this pull request may close these issues.

Misleading error for missing comma/semicolon in array literal
7 participants