Parse lets as if they were items for diagnostics. #159044
Open
kpreid wants to merge 2 commits into
Open
Conversation
Collaborator
|
cc @rust-lang/rustfmt The parser was modified, potentially altering the grammar of (stable) Rust cc @fmease |
rustbot
added
the
S-waiting-on-review
rustbot
added
T-compiler
Collaborator
|
r? @fee1-dead rustbot has assigned @fee1-dead. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
This change provides a precise diagnostic in all cases of `let` appearing in a context where items but not statements are permitted, and `let` appearing after an item modifier keyword (`pub` or `final`). It provides a suggestion for replacing `let` with `const` (which should be usable interactively in rust-analyzer). It provides advice when the user might have thought `final` meant what it means in Java (immutable variable declaration). It also consolidates two existing, less powerful `let` diagnostics into the new code: one for module bodies, and one for `trait`/`impl` bodies. In order to be able to determine whether a `let` *would* parse if written properly, it adds a new parameter, `StmtWouldBeAllowed`. This is a bit intrusive and not ideal, but I couldn’t think of a better solution, other than extending `FnContext` to mean more than it currently does, which seems not necessarily wise.
kpreid
commented
Jul 9, 2026
| .parse_item( | ||
| ForceCollect::Yes, | ||
| AllowConstBlockItems::Yes, | ||
| StmtWouldBeAllowed::NoOrUnknown, |
Contributor
Author
There was a problem hiding this comment.
I don’t like that I had to add a new parameter to parse_item everywhere, but I don’t know what would be better. Probably a new distinct entry point function.
Contributor
Author
There was a problem hiding this comment.
Tidy doesn’t like how long this file is now. Should I split it in this PR? Put up a separate PR? Ignore?
| let error = match allow_suggest_stmt { | ||
| StmtWouldBeAllowed::Yes => { | ||
| // We are in a function, `const` block, or other context in which a `let` | ||
| // *would* be allowed, except that we must have parsed some item-modifier |
Contributor
Author
There was a problem hiding this comment.
Is there an established term for these keywords like pub and final that appear before the item-kind keyword? I picked “modifier” arbitrarily.
| } | ||
| }; | ||
|
|
||
| // FIXME: Ideally, we would have a `ItemKind::Err` that optionally defines a name but |
Contributor
Author
There was a problem hiding this comment.
I could try to add this new item kind to this PR if desired.
Collaborator
|
The job Click to see the possible cause of the failure (guessed by this bot) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Partially addresses #92615.
Fixes #101622.
This change provides a precise diagnostic in all cases of
letappearing in a context where items but not statements are permitted, andletappearing after an item modifier keyword (puborfinal).It provides a suggestion for replacing
letwithconst(which should be usable interactively in rust-analyzer). It provides advice when the user might have thoughtfinalmeant what it means in Java (immutable variable declaration).It also consolidates two existing, less powerful
letdiagnostics into the new code: one for module bodies, and one fortrait/implbodies.In order to be able to determine whether a
letwould parse if written properly, it adds a new parameter to the parsing functions,StmtWouldBeAllowed. This is a bit intrusive and not ideal, but I couldn’t think of a better solution, other than extendingFnContextto mean more than it currently does, which seems not necessarily wise.