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

Improve suggestion for tuple struct pattern matching errors. #81235

Merged
merged 3 commits into from
Feb 23, 2021
Merged

Improve suggestion for tuple struct pattern matching errors. #81235

merged 3 commits into from
Feb 23, 2021

Conversation

reese
Copy link
Contributor

@reese reese commented Jan 21, 2021

Closes #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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 21, 2021
@rust-log-analyzer

This comment has been minimized.

Currently, when a user uses a struct pattern to pattern match on
a tuple struct, the errors we emit generally suggest adding fields
using their field names, which are numbers. However, numbers are
not valid identifiers, so the suggestions, which use the shorthand
notation, are not valid syntax. This commit changes those errors
to suggest using the actual tuple struct pattern syntax instead,
which is a more actionable suggestion.
Copy link
Contributor

@estebank estebank left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small tweaks requested, otherwise r=me.

Comment on lines +1443 to +1456
match self.tcx.sess.source_map().span_to_snippet(field.pat.span) {
Ok(f) => {
// Field names are numbers, but numbers
// are not valid identifiers
if variant_field_idents.contains(&field.ident) {
String::from("_")
} else {
f
}
}
Err(_) => rustc_hir_pretty::to_string(rustc_hir_pretty::NO_ANN, |s| {
s.print_pat(field.pat)
}),
}
Copy link
Contributor

@estebank estebank Jan 27, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code is fair (as you're just copying from existing logic), but ideally we would not be using span_to_snippet here. (No need to change this now.)

compiler/rustc_typeck/src/check/pat.rs Outdated Show resolved Hide resolved
compiler/rustc_typeck/src/check/pat.rs Outdated Show resolved Hide resolved
Comment on lines 4 to 7
LL | if let S { a, b, c, d } = S(1, 2, 3, 4) {
| ^^^^^^^^^^^^^^^^ help: use the tuple variant pattern syntax instead: `S(a, b, c, d)`
| -^^^^^^^^^^^^^^^
| |
| help: use the tuple variant pattern syntax instead: `(a, b, c, d)`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The span in all of the tests' suggestions seems to now be wrong. The resulting code would be (a, b, c, d) instead of the desired S(a, b, c, d).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The suggestion here seemed like you were suggesting to remove the path from the help text. It seems like I misunderstood the intent there, what were you suggesting to use instead of the path?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My proposal is to use span_suggestion_verbose to have a separate suggestion subwindow, but for the suggestion to be correctly displayed you need to make a span trimming the S from the beginning. The suggestion has to have the correct span for it to be rendered correctly.

In the case highlighted above, you had a span pointing at S { a, b, c, d }. You now have a span pointing at S. Creating a new span that starts at the end of S's span, you'll have a span pointing at { a, b, c, d } which would do then render correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, having that suggestion with span_suggestion_verbose makes a lot of sense. Thanks for the clarification, still figuring out all the options for these diagnostics 😅

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 22, 2021

📌 Commit d8540ae has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 22, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request 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 added a commit to rust-lang-ci/rust that referenced this pull request Feb 23, 2021
Rollup of 12 pull requests

Successful merges:

 - rust-lang#79423 (Enable smart punctuation)
 - rust-lang#81154 (Improve design of `assert_len`)
 - rust-lang#81235 (Improve suggestion for tuple struct pattern matching errors.)
 - rust-lang#81769 (Suggest `return`ing tail expressions that match return type)
 - rust-lang#81837 (Slight perf improvement on char::to_ascii_lowercase)
 - rust-lang#81969 (Avoid `cfg_if` in `std::os`)
 - rust-lang#81984 (Make WASI's `hard_link` behavior match other platforms.)
 - rust-lang#82091 (use PlaceRef abstractions more consistently)
 - rust-lang#82128 (add diagnostic items for OsString/PathBuf/Owned as well as to_vec on slice)
 - rust-lang#82166 (add s390x-unknown-linux-musl target)
 - rust-lang#82234 (Remove query parameters when skipping search results)
 - rust-lang#82255 (Make `treat_err_as_bug` Option<NonZeroUsize>)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8e51bd4 into rust-lang:master Feb 23, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 23, 2021
@reese reese deleted the rw-tuple-diagnostics branch February 25, 2021 00:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad suggestion for tuple struct used in pattern with struct syntax and missing fields
6 participants