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

Accurate witnesses for non-exhaustive pattern matching #14731

Closed
wants to merge 7 commits into from
Closed

Accurate witnesses for non-exhaustive pattern matching #14731

wants to merge 7 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jun 7, 2014

This PR is changing the error messages for non-exhaustive pattern matching to include a more accurate witness, i.e. a pattern that is not covered by any of the ones provided by the user. Example:

fn main() {
    match (true, (Some("foo"), [true, true]), Some(42u)) {
        (false, _, _) => (),
        (true, (None, [true, _]), None) => (),
        (true, (None, [false, _]), Some(1u)) => ()
    }
}
/tmp/witness.rs:2:2: 6:3 error: non-exhaustive patterns: (true, (core::option::Some(_), _), _) not covered
/tmp/witness.rs:2   match (true, (Some("foo"), [true, true]), Some(42u)) {
/tmp/witness.rs:3       (false, _, _) => (),
/tmp/witness.rs:4       (true, (None, [true, _]), None) => (),
/tmp/witness.rs:5       (true, (None, [false, _]), Some(1u)) => ()
/tmp/witness.rs:6   }

As part of that, I refactored some of the relevant code and carried over the changes to fixed vectors from the previous PR.

I'm putting it out there for now but the tests will be red.

@ghost ghost changed the title Accurate witnesses for non-exhaustive pattern matching [WIP] Accurate witnesses for non-exhaustive pattern matching Jun 8, 2014
@alexcrichton
Copy link
Member

This is an awesome feature to have, thanks so much for working in this area!

@huonw
Copy link
Member

huonw commented Jun 8, 2014

Travis failed.

@ghost
Copy link
Author

ghost commented Jun 8, 2014

This should turn green now.

@alexcrichton Not a problem at all, more than happy to help out getting Rust to 1.0. :-)

@huonw Addressed your comments.

@ghost
Copy link
Author

ghost commented Jun 12, 2014

I rebased against the recent Gc changes. Should be green, not sure where that rustdoc failure comes from but it seems unrelated.

@alexcrichton
Copy link
Member

cc @nikomatsakis

@ghost
Copy link
Author

ghost commented Jun 15, 2014

Whilst working on this stuff I noticed there's a surprising amount of logic duplication across typeck and codegen for patterns. I have another branch that vastly improves the situation but I'll refrain from stuffing it here so as not to make the review harder.

@pnkfelix
Copy link
Member

This is exciting work.

I however do not like the change in the error reports where they now use fully qualified absolute paths for the enum variants, rather than just using either the last element on the path, or (preferably) the shortest relative path that would work in the context of the match in question. As an example of what I am objecting to, see the various changes in the compile-fail tests, such as this here: https://github.com/rust-lang/rust/pull/14731/files#diff-4

(where of course it is an improvement that you tell the user that they need a tuple of nones; I just would prefer to see it printed as (None, None), especially if None is in scope.)

@nikomatsakis
Copy link
Contributor

I will read over the code. I'd also appreciate feedback from @edwardw.

@nikomatsakis
Copy link
Contributor

I "spot-checked" and found that everything made sense, but I think I will have to invest a large amount of time to really say that I understand deeply all the changes. Based on what I saw, though, this seems quite nice, and I'm willing to r+ pending satisfaction of @pnkfelix and @edwardw's complaints. (That said, I personally am satisfied with the error messages as they are.)

One thing I would like to see is an expansion of the non-exhaustive-match tests. They seem fairly shallow. I'd like to see e.g. non-exhaustive-vectors embedded in enums and other strange things.

@nikomatsakis
Copy link
Contributor

(Regarding the tests, I recognize that's a pre-existing problem.)

Jakub Wieczorek added 6 commits June 20, 2014 17:08

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
Fixed #4321
String patterns should have a single constructor of arity 0.
@ghost
Copy link
Author

ghost commented Jun 20, 2014

I addressed most of the comments, thanks very much for the review, everyone! And it looks like some of @edwardw's inline comments are gone due to my obsessive rebasing. Apologies for that!

@nikomatsakis Right, I added one more test case with a few examples of nested destructuring. I agree that these parts of the compiler are not the easiest to navigate. I'll see if I can document some of it.

@edwardw Right, I like that idea. Would you mind if I did this separately? It shouldn't be too hard to collect more than one witness but I need to think this through.

@edwardw
Copy link
Contributor

edwardw commented Jun 20, 2014

Awesome work! And I agree, the exhaustible and non-exhaustible vector patterns can be done separately.

@ghost
Copy link
Author

ghost commented Jun 20, 2014

Looks like #13793...

@ghost
Copy link
Author

ghost commented Jun 20, 2014

I skipped that test on Windows as it seems it's not a legitimate failure.

bors added a commit that referenced this pull request Jun 21, 2014
…richton

This PR is changing the error messages for non-exhaustive pattern matching to include a more accurate witness, i.e. a pattern that is not covered by any of the ones provided by the user. Example:

```rust
fn main() {
	match (true, (Some("foo"), [true, true]), Some(42u)) {
		(false, _, _) => (),
		(true, (None, [true, _]), None) => (),
		(true, (None, [false, _]), Some(1u)) => ()
	}
}
```

```sh
/tmp/witness.rs:2:2: 6:3 error: non-exhaustive patterns: (true, (core::option::Some(_), _), _) not covered
/tmp/witness.rs:2 	match (true, (Some("foo"), [true, true]), Some(42u)) {
/tmp/witness.rs:3 		(false, _, _) => (),
/tmp/witness.rs:4 		(true, (None, [true, _]), None) => (),
/tmp/witness.rs:5 		(true, (None, [false, _]), Some(1u)) => ()
/tmp/witness.rs:6 	}
```

As part of that, I refactored some of the relevant code and carried over the changes to fixed vectors from the previous PR.

I'm putting it out there for now but the tests will be red.
@bors bors closed this Jun 21, 2014
@ghost ghost deleted the pattern-matching-refactor branch July 2, 2014 23:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants