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

Recover from more const arguments that are not wrapped in curly braces #107190

Merged
merged 1 commit into from
Jan 28, 2023

Conversation

fmease
Copy link
Member

@fmease fmease commented Jan 22, 2023

Recover from some array, borrow, tuple & arithmetic expressions in const argument positions that lack curly braces and provide a suggestion to fix the issue continuing where #92884 left off. Examples of such expressions: [], [0], [1, 2], [0; 0xff], &9, ("", 0) and (1 + 2) * 3 (we previously did not recover from them).

I am not entirely happy with my current solution because the code that recovers from [0] (coinciding with a malformed slice type) and [0; 0] (coinciding with a malformed array type) is quite fragile as the aforementioned snippets are actually successfully parsed as types by parse_ty since it itself already recovers from them (returning [⟨error⟩] and [⟨error⟩; 0] respectively) meaning I have to manually look for TyKind::Errs and construct a separate diagnostic for the suggestion to attach to (thereby emitting two diagnostics in total).

Fixes #81698.
@rustbot label A-diagnostics
r? diagnostics

@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. A-diagnostics Area: Messages for errors, warnings, and lints labels Jan 22, 2023
@@ -5,6 +5,22 @@ LL | fn f1<'a>(arg : Box<dyn X<Y = B = &'a ()>>) {}
| - ^ expected one of 7 possible tokens
| |
| maybe try to close unmatched angle bracket
|
help: expressions must be enclosed in braces to be used as const generic arguments
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm... this seems pretty strange

Copy link
Member Author

@fmease fmease Jan 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I'm pretty sure dyn is parsed as a normal identifier here and not as the edition-2018-specific keyword dyn since the test doesn't specify any edition thereby defaulting to 2015. Hence we suggest {dyn} and dyn> (where dyn is interpreted as the name of a type).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not saying that this isn't something I should fix. I will definitely try to improve this output if possible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the suggestion better on edition 2021?

Copy link
Member Author

@fmease fmease Jan 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it is (1 error instead of 2 & no (weird) suggestions):

error: expected one of `!`, `(`, `+`, `,`, `::`, `<`, or `>`, found `=`
 --> tests/ui/generic-associated-types/parse/trait-path-expected-token.rs:7:33
  |
7 | fn f1<'a>(arg : Box<dyn X<Y = B = &'a ()>>) {}
  |                               - ^ expected one of 7 possible tokens
  |                               |
  |                               maybe try to close unmatched angle bracket

error: aborting due to previous error

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It may be worthwhile teaching this recovery to ignore dyn identifiers (or perhaps all keyword identifiers), even if valid, just for better aesthetics.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do!

Copy link
Member Author

@fmease fmease Jan 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I've gone for a much simpler solution: I will only emit the suggestion if , or > succeeds the unbraced expression to avoid suggesting garbage if the input is garbage. I've added a small comment for that, too. Now there are no diagnostic regressions at all anymore.

compiler/rustc_parse/src/parser/path.rs Show resolved Hide resolved
@davidtwco
Copy link
Member

r? @compiler-errors

@rustbot rustbot assigned compiler-errors and unassigned davidtwco Jan 27, 2023
@fmease fmease marked this pull request as ready for review January 27, 2023 18:28
@compiler-errors
Copy link
Member

Cool, this seems valid.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 27, 2023

📌 Commit 80a1536 has been approved by compiler-errors

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
bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 28, 2023
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#107022 (Implement `SpecOptionPartialEq` for `cmp::Ordering`)
 - rust-lang#107100 (Use proper `InferCtxt` when probing for associated types in astconv)
 - rust-lang#107103 (Use new solver in `evaluate_obligation` query (when new solver is enabled))
 - rust-lang#107190 (Recover from more const arguments that are not wrapped in curly braces)
 - rust-lang#107306 (Correct suggestions for closure arguments that need a borrow)
 - rust-lang#107339 (internally change regions to be covariant)
 - rust-lang#107344 (Minor tweaks in the new solver)
 - rust-lang#107373 (Don't merge vtables when full debuginfo is enabled.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 260e048 into rust-lang:master Jan 28, 2023
@rustbot rustbot added this to the 1.69.0 milestone Jan 28, 2023
@fmease fmease deleted the fix-81698 branch January 28, 2023 10:01
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 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.

arrays as const parameters result in bad error messages
5 participants