Improve missing @ in slice binding pattern diagnostics
#72534
Merged
Conversation
This file contains 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
rust-highfive
added
the
S-waiting-on-review
estebank
requested changes
May 24, 2020
src/librustc_parse/parser/mod.rs
Outdated
| expect_err | ||
| .span_suggestion_short( | ||
| self.token.span, | ||
| &format!("maybe missing `@` in binding pattern?"), |
There was a problem hiding this comment.
Suggested change
| &format!("maybe missing `@` in binding pattern?"), | |
| "maybe missing `@` in binding pattern?", |
There was a problem hiding this comment.
Ideally we would want to change the message to be something like "if you meant to bind the contents of the rest of the array pattern into ts, use @. It would also be beneficial to verify that we are in an array pattern (but if it makes the code much more convoluted, we can skip that check).
There was a problem hiding this comment.
0fdd31317a9c8d2fbc284546c711bc5d5139d83b
I was able to pull the name from the previous Ident token with pprust::token_to_string.
There was a problem hiding this comment.
It would also be beneficial to verify that we are in an array pattern (but if it makes the code much more convoluted, we can skip that check).
Is this valid in tuples too?
There was a problem hiding this comment.
There was a problem hiding this comment.
It looks like that message could be cleaned up too:
error: `..` patterns are not allowed here
--> src/main.rs:4:15
|
4 | (a, x@..) => {}
| ^^
|
= note: only allowed in tuple, tuple struct, and slice patterns
That note appears to be incorrect and the error should indicate @ .. are not allowed in the tuple, not ..?
There was a problem hiding this comment.
I checked this for tuple structs and the same message appears. I will open a new issue for this.
|
Thanks Esteban! Responses inline |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
estebank
reviewed
May 25, 2020
src/librustc_parse/parser/mod.rs
Outdated
| expect_err | ||
| .span_suggestion_short( | ||
| self.token.span, | ||
| &format!("maybe missing `@` in binding pattern?"), |
There was a problem hiding this comment.
estebank
reviewed
May 25, 2020
estebank
approved these changes
May 25, 2020
|
I believe that everything is addressed as of 04cc168 except the check that we are in a slice from your comment in #72534 (comment). I'll take a look into this tomorrow. Please let me know if you have any suggestions about where to start. |
|
[ No longer an issue. Addressed in https://github.com//pull/72534#issuecomment-634759671 ] These changes lead to an ICE when I try to compile the following: fn main() {
let x = (1, 2, 3);
match x {
(_a, 1, 1) => {}
(_a, ..) => {}
}
}
Backtrace
I don't see this on rustc 1.45.0-nightly (46e85b4 2020-05-24), rustc 1.44.0-beta.4 (02c25b3 2020-05-23), or rustc 1.43.1 (8d69840 2020-05-04). When I compile the following (with the fn main() {
let x = (1, 2, 3);
match x {
(_a, 1, 1) => {}
(_a, _x @ ..) => {}
}
}I get the expected error message that needs to be fixed (#72574) - notably, our changes here do not modify the diagnostics for tuples (or tuple structs, see next post): Thoughts? It is not clear to me what I am modifying that leads to this problem. The conditions for the new block of code are not met and it is not executed in the source that leads to an ICE. |
|
[ No longer an issue. Addressed in https://github.com//pull/72534#issuecomment-634759671 ] Tuple structs ICE too. I see the same ICE with the changes in this PR and the following source: struct Binder(i32, i32, i32);
fn main() {
let x = Binder(1, 2, 3);
match x {
Binder(_a, 1, 1) => {}
Binder(_a, ..) => {}
}
}I see the #72574 error message when this is changed to use |
|
Rebased on acfc558 and rebuilt stage 1 rustc from scratch without incremental compilation. This addressed the ICE issues for tuples and tuple structs above. If the CI passes here, I will squash commits and push. I think that we are gtg. |
… rest pattern add issue 72373 tests fmt test fix suggestion format Replacement, not insertion of suggested string implement review changes refactor to span_suggestion_verbose, improve suggestion message, change id @ pattern space formatting fmt fix diagnostics spacing between ident and @ refactor reference
|
CI passed. Squashed and pushed. #72534 (comment) was addressed by opening a new IR (#72574). I'll have a look at that in a new PR (#72677). I can confirm that these changes do not impact diagnostics for tuples or tuple structs with the gtg from my standpoint. |
|
@bors r+ rollup |
|
📌 Commit 593d1ee has been approved by |
bors
added
S-waiting-on-bors
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 29, 2020
Rollup of 11 pull requests Successful merges: - rust-lang#71633 (Impl Error for Infallible) - rust-lang#71843 (Tweak and stabilize AtomicN::fetch_update) - rust-lang#72288 (Stabilization of weak-into-raw) - rust-lang#72324 (Stabilize AtomicN::fetch_min and AtomicN::fetch_max) - rust-lang#72452 (Clarified the documentation for Formatter::precision) - rust-lang#72495 (Improve E0601 explanation) - rust-lang#72534 (Improve missing `@` in slice binding pattern diagnostics) - rust-lang#72547 (Added a codegen test for a recent optimization for overflow-checks=on) - rust-lang#72711 (remove redundant `mk_const`) - rust-lang#72713 (Whitelist #[allow_internal_unstable]) - rust-lang#72720 (Clarify the documentation of `take`) Failed merges: r? @ghost
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
May 30, 2020
Fix diagnostics for `@ ..` binding pattern in tuples and tuple structs Fixes rust-lang#72574 Associated rust-lang#72534 rust-lang#72373 Includes a new suggestion with `Applicability::MaybeIncorrect` confidence level. ### Before #### tuple ``` error: `..` patterns are not allowed here --> src/main.rs:4:19 | 4 | (_a, _x @ ..) => {} | ^^ | = note: only allowed in tuple, tuple struct, and slice patterns error[E0308]: mismatched types --> src/main.rs:4:9 | 3 | match x { | - this expression has type `({integer}, {integer}, {integer})` 4 | (_a, _x @ ..) => {} | ^^^^^^^^^^^^^ expected a tuple with 3 elements, found one with 2 elements | = note: expected tuple `({integer}, {integer}, {integer})` found tuple `(_, _)` error: aborting due to 2 previous errors ``` #### tuple struct ``` error: `..` patterns are not allowed here --> src/main.rs:6:25 | 6 | Binder(_a, _x @ ..) => {} | ^^ | = note: only allowed in tuple, tuple struct, and slice patterns error[E0023]: this pattern has 2 fields, but the corresponding tuple struct has 3 fields --> src/main.rs:6:9 | 1 | struct Binder(i32, i32, i32); | ----------------------------- tuple struct defined here ... 6 | Binder(_a, _x @ ..) => {} | ^^^^^^^^^^^^^^^^^^^ expected 3 fields, found 2 error: aborting due to 2 previous errors ``` ### After *Note: final output edited during source review discussion, see thread for details* #### tuple ``` error: `_x @` is not allowed in a tuple --> src/main.rs:4:14 | 4 | (_a, _x @ ..) => {} | ^^^^^^^ is only allowed in a slice | help: replace with `..` or use a different valid pattern | 4 | (_a, ..) => {} | ^^ error[E0308]: mismatched types --> src/main.rs:4:9 | 3 | match x { | - this expression has type `({integer}, {integer}, {integer})` 4 | (_a, _x @ ..) => {} | ^^^^^^^^^^^^^ expected a tuple with 3 elements, found one with 1 element | = note: expected tuple `({integer}, {integer}, {integer})` found tuple `(_,)` error: aborting due to 2 previous errors ``` #### tuple struct ``` error: `_x @` is not allowed in a tuple struct --> src/main.rs:6:20 | 6 | Binder(_a, _x @ ..) => {} | ^^^^^^^ is only allowed in a slice | help: replace with `..` or use a different valid pattern | 6 | Binder(_a, ..) => {} | ^^ error[E0023]: this pattern has 1 field, but the corresponding tuple struct has 3 fields --> src/main.rs:6:9 | 1 | struct Binder(i32, i32, i32); | ----------------------------- tuple struct defined here ... 6 | Binder(_a, _x @ ..) => {} | ^^^^^^^^^^^^^^^^^^^ expected 3 fields, found 1 error: aborting due to 2 previous errors ``` r? @estebank
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #72373
Includes a new suggestion with
Applicability::MaybeIncorrectconfidence level.Before:
After:
r? @estebank