Parse lets as if they were items for diagnostics. - #159044
Conversation
|
cc @rust-lang/rustfmt The parser was modified, potentially altering the grammar of (stable) Rust cc @fmease |
|
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.
| .parse_item( | ||
| ForceCollect::Yes, | ||
| AllowConstBlockItems::Yes, | ||
| StmtWouldBeAllowed::NoOrUnknown, |
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.
There was a problem hiding this comment.
yeah, I don't like we need to change parse_item everywhere, seems not worth for this diganostic.
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?
There was a problem hiding this comment.
In the absence of outside advice, I have split a new file parser/function.rs out of parser/item.rs to satisfy tidy. I will be happy to make that change in a separate PR if that is preferred.
| 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 |
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 |
There was a problem hiding this comment.
I could try to add this new item kind to this PR if desired.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This is needed to keep `item.rs` under the length limit.
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.
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
Sorry to be doing this but I am too swamped IRL to review a diff with this size. @rustbot reroll |
|
Note that this PR would be much smaller if:
I would welcome any guidance as to whether and how to do those things. |
|
☔ The latest upstream changes (presumably #159814) made this pull request unmergeable. Please resolve the merge conflicts by rebasing. |
There was a problem hiding this comment.
Sorry for the delay, I tried to take a look at this but this change is too big for an area that I am not very familiar with. I just have a small comment.
@rustbot reroll
There was a problem hiding this comment.
This seems weird because there is no let statement here 🤔
There was a problem hiding this comment.
This is a .fixed file: it is an edited version of the corresponding .rs file which shows the result of applying a suggestion. This particular suggestion replaced let with const. So, it is correct that the //~^ ERROR shows an error that is not in the file.
| | ^^^ | ||
| | | ||
| = note: `let` cannot be used to define global variables | ||
| = help: consider using `static` and a `Mutex` instead of `let mut` |
There was a problem hiding this comment.
why we remove the url link in old stderr, something like
error: expected item, found keyword `let`
--> tests/ui/parser/let/suggest-const-for-global-var.rs:1:1
|
1 | let X: i32 = 12;
| ^^^
| |
| `let` cannot be used for global variables
| help: consider using `static` or `const` instead of `let`
|
= note: for a full list of items that can appear in modules, see <https://doc.rust-lang.org/reference/items.html>
There was a problem hiding this comment.
The link is part of an error path primarily meant for the parser telling the user “You gave me something I don’t recognize at all. Here are the things I do recognize.” But in this case, we know exactly what the syntax means, and it’s just not valid in this context. So, telling the user to browse the full list, looking for something that resembles what they want, is not useful.
That said, it would be easy to put this link back. I just don’t think we should.
| macro_rules! m { | ||
| () => { | ||
| let //~ ERROR macro expansion ignores keyword `let` and any tokens following | ||
| loop //~ ERROR macro expansion ignores keyword `loop` and any tokens following |
There was a problem hiding this comment.
not sure why you change this test.
There was a problem hiding this comment.
Because using let here would no longer exercise the code path that this test is meant to exercise.
it's better to split the moving of function parsing to another seperate PR, it's better to review. |
I have just done that: #160040 |
Split function parsing out of `item.rs` to a new module. `item.rs` is approaching the tidy-enforced length limit, and this will make room for new additions. I have split this change out of PR rust-lang#159044 to reduce its complexity (and because this large code move quickly becomes stale).
Split function parsing out of `item.rs` to a new module. `item.rs` is approaching the tidy-enforced length limit, and this will make room for new additions. I have split this change out of PR rust-lang#159044 to reduce its complexity (and because this large code move quickly becomes stale).
Split function parsing out of `item.rs` to a new module. `item.rs` is approaching the tidy-enforced length limit, and this will make room for new additions. I have split this change out of PR rust-lang#159044 to reduce its complexity (and because this large code move quickly becomes stale).
Split function parsing out of `item.rs` to a new module. `item.rs` is approaching the tidy-enforced length limit, and this will make room for new additions. I have split this change out of PR rust-lang#159044 to reduce its complexity (and because this large code move quickly becomes stale).
View all comments
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.