-
-
Notifications
You must be signed in to change notification settings - Fork 14.4k
Recover parse gracefully from <const N>
#151099
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
Conversation
|
|
This comment has been minimized.
This comment has been minimized.
When a const param doesn't have a `: Type`, recover the parser state and provide a structured suggestion. This not only provides guidance on what was missing, but it also makes subsuequent errors to be emitted that would otherwise be silenced. ``` error: expected `:`, found `>` --> $DIR/incorrect-const-param.rs:26:16 | LL | impl<T, const N> From<[T; N]> for VecWrapper<T> | ^ expected `:` | help: you might have meant to write the type of the const parameter here | LL | impl<T, const N: /* Type */> From<[T; N]> for VecWrapper<T> | ++++++++++++ ```
0ca242e to
92db7b2
Compare
|
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks! r=me rollup with nitpick addressed in one way or another
| @@ -0,0 +1,45 @@ | |||
| // #84327 | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I get that it's nice to group all 3 cases from the issue but is it really necessary? We already have tests in separate files for the other 2 cases we've already addressed (in your PR: #151077, and in mine: #120570).
This doesn't increase test coverage unless I've missed sth. and the added test cases exercise things the preexisting tests don't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It exercises that the parser recovers correctly besides just providing the suggestion. I tend to group related checks either in the same file when possible, or with files with similar naming. I considered only having this case in the test and then a type error, but felt that having all of them (even if redundant because individual cases are already tested) would help in the future if someone makes changes that incidentally touches these to notice that there are similarly handled cases.
|
@bors r=fmease |
|
@bors rollup |
Recover parse gracefully from `<const N>` When a const param doesn't have a `: Type`, recover the parser state and provide a structured suggestion. This not only provides guidance on what was missing, but it also makes subsuequent errors to be emitted that would otherwise be silenced. ``` error: expected `:`, found `>` --> $DIR/incorrect-const-param.rs:26:16 | LL | impl<T, const N> From<[T; N]> for VecWrapper<T> | ^ expected `:` | help: you might have meant to write the type of the const parameter here | LL | impl<T, const N: /* Type */> From<[T; N]> for VecWrapper<T> | ++++++++++++ ``` r? @fmease Follow up to rust-lang#151077. Fix rust-lang#84327.
…uwer Rollup of 16 pull requests Successful merges: - #150585 (Add a context-consistency check before emitting redundant generic-argument suggestions) - #150586 (rustdoc: Fix intra-doc link bugs involving type aliases and associated items) - #150590 (Don't try to recover keyword as non-keyword identifier ) - #150817 (cleanup: remove borrowck handling for inline const patterns) - #150939 (resolve: Relax some asserts in glob overwriting and add tests) - #150962 (Remove `FeedConstTy` and provide ty when lowering const arg) - #150966 (rustc_target: Remove unused Arch::PowerPC64LE) - #150971 (Disallow eii in statement position) - #151016 (fix: WASI threading regression by disabling pthread usage) - #151046 (compiler: Make Externally Implementable Item (eii) macros "semiopaque") - #151099 (Recover parse gracefully from `<const N>`) - #151117 (Avoid serde dependency in build_helper when not necessary) - #151127 (Delete `MetaItemOrLitParser::Err`) - #151128 (Add temporary new bors e-mail address to the mailmap) - #151130 (resolve: Downgrade `ambiguous_glob_imports` to warn-by-default) - #151138 (Rename `rust.use-lld` to `rust.bootstrap-override-lld` in INSTALL.md) r? @ghost
…uwer Rollup of 15 pull requests Successful merges: - #150585 (Add a context-consistency check before emitting redundant generic-argument suggestions) - #150586 (rustdoc: Fix intra-doc link bugs involving type aliases and associated items) - #150590 (Don't try to recover keyword as non-keyword identifier ) - #150817 (cleanup: remove borrowck handling for inline const patterns) - #150939 (resolve: Relax some asserts in glob overwriting and add tests) - #150962 (Remove `FeedConstTy` and provide ty when lowering const arg) - #150966 (rustc_target: Remove unused Arch::PowerPC64LE) - #150971 (Disallow eii in statement position) - #151016 (fix: WASI threading regression by disabling pthread usage) - #151046 (compiler: Make Externally Implementable Item (eii) macros "semiopaque") - #151099 (Recover parse gracefully from `<const N>`) - #151117 (Avoid serde dependency in build_helper when not necessary) - #151127 (Delete `MetaItemOrLitParser::Err`) - #151128 (Add temporary new bors e-mail address to the mailmap) - #151138 (Rename `rust.use-lld` to `rust.bootstrap-override-lld` in INSTALL.md) r? @ghost
Rollup merge of #151099 - issue-84327-2, r=fmease Recover parse gracefully from `<const N>` When a const param doesn't have a `: Type`, recover the parser state and provide a structured suggestion. This not only provides guidance on what was missing, but it also makes subsuequent errors to be emitted that would otherwise be silenced. ``` error: expected `:`, found `>` --> $DIR/incorrect-const-param.rs:26:16 | LL | impl<T, const N> From<[T; N]> for VecWrapper<T> | ^ expected `:` | help: you might have meant to write the type of the const parameter here | LL | impl<T, const N: /* Type */> From<[T; N]> for VecWrapper<T> | ++++++++++++ ``` r? @fmease Follow up to #151077. Fix #84327.
…uwer Rollup of 15 pull requests Successful merges: - rust-lang/rust#150585 (Add a context-consistency check before emitting redundant generic-argument suggestions) - rust-lang/rust#150586 (rustdoc: Fix intra-doc link bugs involving type aliases and associated items) - rust-lang/rust#150590 (Don't try to recover keyword as non-keyword identifier ) - rust-lang/rust#150817 (cleanup: remove borrowck handling for inline const patterns) - rust-lang/rust#150939 (resolve: Relax some asserts in glob overwriting and add tests) - rust-lang/rust#150962 (Remove `FeedConstTy` and provide ty when lowering const arg) - rust-lang/rust#150966 (rustc_target: Remove unused Arch::PowerPC64LE) - rust-lang/rust#150971 (Disallow eii in statement position) - rust-lang/rust#151016 (fix: WASI threading regression by disabling pthread usage) - rust-lang/rust#151046 (compiler: Make Externally Implementable Item (eii) macros "semiopaque") - rust-lang/rust#151099 (Recover parse gracefully from `<const N>`) - rust-lang/rust#151117 (Avoid serde dependency in build_helper when not necessary) - rust-lang/rust#151127 (Delete `MetaItemOrLitParser::Err`) - rust-lang/rust#151128 (Add temporary new bors e-mail address to the mailmap) - rust-lang/rust#151138 (Rename `rust.use-lld` to `rust.bootstrap-override-lld` in INSTALL.md) r? @ghost
When a const param doesn't have a
: Type, recover the parser state and provide a structured suggestion. This not only provides guidance on what was missing, but it also makes subsuequent errors to be emitted that would otherwise be silenced.r? @fmease
Follow up to #151077. Fix #84327.