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

check_match: refactor + improve non-exhaustive diagnostics for default binding modes #64271

Merged
merged 7 commits into from
Sep 11, 2019

Conversation

Centril
Copy link
Contributor

@Centril Centril commented Sep 7, 2019

Refactor check_match a bit with more code-reuse and improve the diagnostics for a non-exhaustive pattern match by peeling off any references from the scrutinee type so that the "defined here" label is added in more cases. For example:

error[E0004]: non-exhaustive patterns: `&mut &B` not covered
 --> foo.rs:4:11
  |
1 | enum E { A, B }
  | ---------------
  | |           |
  | |           not covered
  | `E` defined here
...
4 |     match x {
  |           ^ pattern `&mut &B` not covered
  |
  = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms

Moreover, wrt. "defined here", we give irrefutable pattern matching (i.e. in let, for, and fn parameters) a more consistent treatment in line with match.

r? @estebank

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 7, 2019
LL | | None,
LL | |
LL | | }
| |_- `Opt` defined here
Copy link
Contributor

Choose a reason for hiding this comment

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

E0005 seem to not be pointing at the uncovered variants. This should also have at least a note about if let. I'll take a look at the code tomorrow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I wondered if that was by-design or not. If not, this would actually simplify the code and I think I know how to fix it. There's a good argument for why it should be by-design without full or-patterns but we will have those on nightly soon so that argument is probably invalid now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be ok to unify both, maybe with some extra help about how to write or-patterns (only if all variants have the currently covered field types), otherwise talk about if let. But this could be done as follow up work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've unified them a bit but I'd like to punt on the extra help or referring to if let. A follow up issue might be good to file however.

src/librustc/ty/util.rs Show resolved Hide resolved
src/librustc_mir/hair/pattern/check_match.rs Show resolved Hide resolved
src/librustc_mir/hair/pattern/check_match.rs Outdated Show resolved Hide resolved
LL | | None,
LL | |
LL | | }
| |_- `Opt` defined here
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it should be ok to unify both, maybe with some extra help about how to write or-patterns (only if all variants have the currently covered field types), otherwise talk about if let. But this could be done as follow up work.

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Sep 11, 2019

📌 Commit 20a2605 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 Sep 11, 2019
@bors
Copy link
Contributor

bors commented Sep 11, 2019

⌛ Testing commit 20a2605 with merge f0b58fc...

bors added a commit that referenced this pull request Sep 11, 2019
check_match: refactor + improve non-exhaustive diagnostics for default binding modes

Refactor `check_match` a bit with more code-reuse and improve the diagnostics for a non-exhaustive pattern match by peeling off any references from the scrutinee type so that the "defined here" label is added in more cases. For example:

```rust
error[E0004]: non-exhaustive patterns: `&mut &B` not covered
 --> foo.rs:4:11
  |
1 | enum E { A, B }
  | ---------------
  | |           |
  | |           not covered
  | `E` defined here
...
4 |     match x {
  |           ^ pattern `&mut &B` not covered
  |
  = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
```

Moreover, wrt. "defined here", we give irrefutable pattern matching (i.e. in `let`, `for`, and `fn` parameters) a more consistent treatment in line with `match`.

r? @estebank
@bors
Copy link
Contributor

bors commented Sep 11, 2019

☀️ Test successful - checks-azure
Approved by: estebank
Pushing f0b58fc to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 11, 2019
@bors bors merged commit 20a2605 into rust-lang:master Sep 11, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #64271!

Tested on commit f0b58fc.
Direct link to PR: #64271

🎉 rls on linux: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Sep 11, 2019
Tested on commit rust-lang/rust@f0b58fc.
Direct link to PR: <rust-lang/rust#64271>

🎉 rls on linux: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).
@Centril Centril deleted the non-exhaustive-peel-refs branch September 11, 2019 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

4 participants