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

Bad suggestion for tuple struct used in pattern with struct syntax and missing fields #80174

Closed
estebank opened this issue Dec 19, 2020 · 4 comments · Fixed by #81235
Closed
Assignees
Labels
A-diagnostics Area: Messages for errors, warnings, and lints A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-bug Category: This is a bug. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@estebank
Copy link
Contributor

estebank commented Dec 19, 2020

For the following code:

struct S(i32, f32);
enum E {
    S(i32, f32),
}
fn main() {
    let x = E::S(1, 2.2);
    match x {
        E::S {} => {}
    }
    let y = S(1, 2.2);
    match y {
        S {} => {}
    }
}

we currently emit

error[E0027]: pattern does not mention fields `0`, `1`
 --> src/main.rs:8:9
  |
8 |         E::S {} => {}
  |         ^^^^^^^ missing fields `0`, `1`
  |
help: include the missing fields in the pattern
  |
8 |         E::S { 0, 1 } => {}
  |              ^^^^^^^^
help: if you don't care about these missing fields, you can explicitely ignore them
  |
8 |         E::S { .. } => {}
  |              ^^^^^^

error[E0027]: pattern does not mention fields `0`, `1`
  --> src/main.rs:12:9
   |
12 |         S {} => {}
   |         ^^^^ missing fields `0`, `1`
   |
help: include the missing fields in the pattern
   |
12 |         S { 0, 1 } => {}
   |           ^^^^^^^^
help: if you don't care about these missing fields, you can explicitely ignore them
   |
12 |         S { .. } => {}
   |           ^^^^^^

but struct patterns cannot refer to tuple field names with numbers, as the parser expects identifiers.

I think that 1) the parser should be changed to accept the positions, but ultimately reject them, suggesting to use the tuple syntax and 2) that the suggestion should be for the tuple syntax to begin with.

@estebank estebank added A-diagnostics Area: Messages for errors, warnings, and lints T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. D-papercut Diagnostics: An error or lint that needs small tweaks. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. labels Dec 19, 2020
@JohnTitor JohnTitor added the C-bug Category: This is a bug. label Dec 30, 2020
@reese
Copy link
Contributor

reese commented Jan 17, 2021

Hey folks, I'd like to work on this ticket. One clarifying question: what's the best way to suggest proper syntax in this case? We can just tell them to use tuple syntax, but is there an effective way to give them an example? Something like this test for E0769 would be helpful, but I don't believe we'd know how many fields to expect while parsing. Is just suggesting tuple syntax like this enough here?

error[E0769]: expected field identifier, found tuple index
  --> src/main.rs:8:9
  |
8 |         E::S { 0, 1 } => {}
  |         ----   ^
  |         |
  |         while parsing the fields for this pattern
  |
   = help: use the tuple variant pattern syntax instead

For cases that parse but are caught in the type checker (where we currently return missing fields '0', '1'), I'd think suggesting tuple syntax and pointing back to the tuple definition (like this) would probably be enough, but happy to hear other thoughts.

@reese
Copy link
Contributor

reese commented Jan 17, 2021

@rustbot claim

@estebank
Copy link
Contributor Author

@reese hi! Thanks for taking this on!

I believe the ideal way to deal with this would be to make the Foo { 0, 1 } use valid in the parser and emit an explicit error when encountering it in a later step of the compiler. But that work is tangentially related (but important to tackle) to the one described on the ticket: this ticket was originally intended to stop suggesting Foo { 0, 1 } and instead suggest Foo(_, _) or similar. I think that the later is easier to implement, but if we can tackle both cases (accepting Foo { 0, 1 } syntactically with a suggestion to use Foo(a, b) and fixing the suggestion we currently give) that would be ideal. Would you be interested in doing both? It will certainly give you a good view of different parts of the compiler :)

@reese
Copy link
Contributor

reese commented Jan 18, 2021

Ah okay, I see what you mean. Sure, I can definitely do both! Thanks for clarifying 👍

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Feb 23, 2021
Improve suggestion for tuple struct pattern matching errors.

Closes rust-lang#80174

This change allows numbers to be parsed as field names when pattern matching on structs, which allows us to provide better error messages when tuple structs are matched using a struct pattern.

r? `@estebank`
@bors bors closed this as completed in 8e51bd4 Feb 23, 2021
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 A-suggestion-diagnostics Area: Suggestions generated by the compiler applied by `cargo fix`. C-bug Category: This is a bug. D-incorrect Diagnostics: A diagnostic that is giving misleading or incorrect information. D-papercut Diagnostics: An error or lint that needs small tweaks. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants