Add non_exhaustive_omitted_patterns lint related to rfc-2008-non_exhaustive

[DevinR528]

Fixes: #84332

This PR adds non_exhaustive_omitted_patterns, an allow by default lint that is triggered when a non_exhaustive type is missing explicit patterns. The warning or deny attribute can be put above the wildcard _ pattern on enums or on the expression for enums or structs. The lint is capable of warning about multiple types within the same pattern. This lint will not be triggered for if let .. patterns.

// crate A
#[non_exhaustive]
pub struct Foo {
    a: u8,
    b: usize,
}
#[non_exhaustive]
pub enum Bar {
    A(Foo),
    B,
}

// crate B
#[deny(non_exhaustive_omitted_patterns)]
match Bar::B {
    Bar::B => {}
    _ => {}
}

#[warn(non_exhaustive_omitted_patterns)]
let Foo { a, .. } = Foo::default();

#[deny(non_exhaustive_omitted_patterns)]
match Bar::B {
    // triggers for Bar::B, and Foo.b
    Bar::A(Foo { a, .. }) => {}
    _ => {}
}

[rust-highfive]

r? @davidtwco

(rust-highfive has picked a reviewer for you, use r? to override)

[LeSeulArtichaut]

Is there a way to check if an AdtDef is local to the place it's being used

I think you can use adt_def.did.is_local()

In the clippy issue it was said there was a better way to find missing variants

The exhaustiveness checker (the thing that checks that matches are exhaustive, FWIW) lives in rustc_mir_build::thir::pattern, but I don't know if it exposes public APIs. I haven't really read your code or what the lint should do, but what you might to do instead is emit the lint in the exhaustiveness checker itself, instead of in a lint pass.

[DevinR528]

Perfect just what I was looking for thanks! I'll leave what I have in lints but start/continue working in the mod you mentioned above. What would make sense to me is add a call check_for_reachable_patterns_in_non_exhaustive here and then adding logic to this func for enums?

Seems to me if we could add a way to ignore _ wildcard matches in non_exhaustive struct/enum patterns we could get a list of UsefulnessReport::non_exhaustiveness_witnesses that has all the patterns missed for the non_exhaustive struct/enum. If we could make a new compute_match_usefulness maybe? I have a vague understanding of the code and where to at least look into doing this but if you have any pointers on how to do this that'd help!

[LeSeulArtichaut]

Okay, I actually read #84332 this time. Just to make sure I understand correctly: the goal is to lint on matches that are non exhaustive if you remove the _ wildcard, not just check that every variant is mentioned, correct?

Or put another way, this should trigger the lint?

/// Latins are still figuring this out, please stand by
#[non_exhaustive]
enum Alphabet {
    A,
    B,
}

match foo {
    Alphabet::A => {}
    // This has a guard, so the match isn't exhaustive, but both A and B are mentioned
    Alphabet::B if guard => {}
    _ => {}
}

[DevinR528]

Yeah that should trigger with

match foo {
    Alphabet::A => {}
    // This has a guard, so the match isn't exhaustive, but both A and B are mentioned
    Alphabet::B if guard => {}
    #[deny(reachable)]
    _ => {}
}

[davidtwco]

This is a good start. I've not checked the logic in too much detail, I will do after some initial changes are made.

There should only be a single lint name involved here - this currently uses reachable and reachable_patterns (reachable_patterns being the lint itself, and reachable being used to indicate a match expression ought to be being checked). I'd recommend changing this so that there's only reachable_patterns (perhaps renamed to non_exhaustive_reachable_patterns), that is allow-by-default. When you emit a lint, the logic for that will work out if it should be shown to the user based on the lint level, so you shouldn't need to check if the attribute is present or not, if the expression is annotated with a #[{allow,deny,warn}(reachable_patterns)] then it'll fire, if not, it won't.

Given that a new lint is a user-facing change, this probably warrants a major change proposal too.

[DevinR528]

I opened an MCP rust-lang/compiler-team#445, I hope I followed the structure ok enough.

The problem with the logic that is currently in the PR is that it only accounts for variants it will not take guards or nesting into account. It seems to me the options are

1. keep it a lint (as it is now) but use some of the usefulness code in the checks in the lint
2. emit the warning/error from inside of the various pattern checks in check_match.rs
3. Something better that I have no idea since I'm not super familiar with the code??

[davidtwco]

I opened an MCP rust-lang/compiler-team#445, I hope I followed the structure ok enough.

The problem with the logic that is currently in the PR is that it only accounts for variants it will not take guards or nesting into account. It seems to me the options are

1. keep it a lint (as it is now) but use some of the [usefulness code](https://github.com/rust-lang/rust/blob/master/compiler/rustc_mir_build/src/thir/pattern/usefulness.rs?rgh-link-date=2021-07-10T01%3A01%3A45Z) in the checks in the lint

2. emit the warning/error from inside of the various pattern checks [in check_match.rs](https://github.com/rust-lang/rust/blob/master/compiler/rustc_mir_build/src/thir/pattern/check_match.rs?rgh-link-date=2021-07-10T01%3A01%3A45Z#L214)

3. Something better that I have no idea since I'm not super familiar with the code??

Let's not invest much more effort into this until the MCP has been looked at by the compiler team, I wouldn't want your time to go to waste if we took the wrong approach here or the MCP wasn't accepted.

[DevinR528]

Thanks for your help, I kinda figured waiting for the MCP was the next step. I have some rough logic to check enums in the usefulness code I was going to push it just so there are 2 commits that can be referenced for an idea of what to do (since they both aren't fully functional). I think if I can make the non_exhaustive variant work it'll be much closer to working for destructuring let stmts too, so I might fiddle with that be for I push another commit, should be by the end of the day.

[DevinR528]

This is now completely done in usefulness checking I removed the LateLintPass implementation. This is now a working lint called non_exhaustive_reachable_patterns!

The lint currently only works for enums and only top-level non_exhaustive enums

// works fine
#[non_exhaustive]
pub enum Bar {
    A,
    B(usize),
    C { field: Foo },
}

// does not work
pub enum VariantNonExhaustive {
    #[non_exhaustive]
    Bar { x: u32 },
    Baz(u32),
}

For the latter to work the lint would have to be implemented for structs which it is not yet.

The major change is to compute_match_usefulness and some of the functions/methods it calls

If we encounter a wildcard entry for a non_exhaustive enum we keep this information in PatCtxt (by way of is_useful -> Usefulness::apply_constructor) which passes it along to SplitWildcard::new to return the list of missing variants instead of Constructor::NonExhaustive, since we don't push the wildcard into the matrix we check that no real variants are missed.

The lint is defined in rustc_lint_defs. It uses the regular lint mechanics instead of checking for the attribute and level in the usefulness code. It emits the lint in check_match and the emitting code is here.

[bors]

☔ The latest upstream changes (presumably #80357) made this pull request unmergeable. Please resolve the merge conflicts.

[sunshowers]

Thank you for doing this!

Out of curiosity how does this handle matching multiple enums at once? For example match (enum_1, enum_2) where:

• both enums are non-exhaustive
• one of the enums is non-exhaustive but not the other

While thinking about this I came to the conclusion that it may not be possible to support adding the lint to the wildcard pattern itself, just to the parent statement -- could be wrong though.

[sunshowers]

BTW I wrote this up (along with alternatives) as an RFC at https://github.com/sunshowers/rfcs/blob/unknown-non-exhaustive/text/0000-unknown-non-exhaustive.md before being pointed to this PR :)

[bors]

⌛ Testing commit 33a06b7 with merge a3b66f0209c4d1ddd6ab7d656aa94e139ebb6b55...

[bors]

💔 Test failed - checks-actions

[ehuss]

@bors retry

#88977 - validate_maintainers is broken.

[rust-log-analyzer]

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

Build completed successfully in 0:00:40
+ /scripts/validate-toolstate.sh
Cloning into 'rust-toolstate'...
<Nothing changed>
HTTPError: HTTP Error 403: Forbidden
b'{"message":"Must have admin access to view repository collaborators.","documentation_url":"https://docs.github.com/rest/reference/repos#list-repository-collaborators"}'
Traceback (most recent call last):
  File "../../src/tools/publish_toolstate.py", line 302, in <module>
    validate_maintainers(repo, github_token)
  File "../../src/tools/publish_toolstate.py", line 93, in validate_maintainers
    'Accept': 'application/vnd.github.hellcat-preview+json',
  File "/usr/lib/python3.6/urllib/request.py", line 223, in urlopen
    return opener.open(url, data, timeout)
  File "/usr/lib/python3.6/urllib/request.py", line 532, in open
    response = meth(req, response)
  File "/usr/lib/python3.6/urllib/request.py", line 642, in http_response
    'http', request, response, code, msg, hdrs)
  File "/usr/lib/python3.6/urllib/request.py", line 570, in error
    return self._call_chain(*args)
  File "/usr/lib/python3.6/urllib/request.py", line 504, in _call_chain
    result = func(*args)
  File "/usr/lib/python3.6/urllib/request.py", line 650, in http_error_default
    raise HTTPError(req.full_url, code, msg, hdrs, fp)
urllib.error.HTTPError: HTTP Error 403: Forbidden

[bors]

⌛ Testing commit 33a06b7 with merge 2b5ddf3...

[bors]

☀️ Test successful - checks-actions
Approved by: Nadrieril
Pushing 2b5ddf3 to master...

[rust-timer]

Finished benchmarking commit (2b5ddf3): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

[m-ou-se]

The io::ErrorKind shows the unstable variants

It looks like that hasn't been solved. The ui test that was added even has unstable variants in the expected output.

Did that happen on purpose?

This goes against the point of #[unstable] and #[doc(hidden)]. Things like this would ideally have @rust-lang/libs-api sign off before merging, to make sure it doesn't break the tools we need to provide stability.

Opening an issue for this, as it should probably be solved before this lint hits stable.

Edit: Issue: #89042

[Nadrieril]

Ah shoot. That's on me. I assumed that the exhaustiveness code already handled those variants in the expected way so I didn't double-check. Shouldn't be too hard to solve thankfully

[pnkfelix]

Visiting for performance triage.

• Moderate improvement in instruction counts (up to -0.6% on full builds of unicode_normalization)

perhaps noise, but 🎉 nonetheless. :)