The non_exhaustive_omitted_patterns lint changed

[Nadrieril]

Hi! In rust-lang/rust#116734 I changed how the lint works to make it more consistent. A consequence is that it no longer makes sense to set the lint level on a specific arm like was recommended before:

match Bar::A {
    Bar::A => {},
    #[warn(non_exhaustive_omitted_patterns)]
    _ => {},
}

It is now recommended to set it on the whole match, or even globally.

#[warn(non_exhaustive_omitted_patterns)]
match Bar::A {
    Bar::A => {},
    _ => {},
}

I didn't realize there was heavy usage of this lint when I made the change, but I understand syn recommends it to avoid breakage? Once rust-lang/rust#117094 lands, setting the lint level on a single arm will raise an error, so the users can update. I thought I'd warn you before, also lmk if I should wait or do it differently. If it's really bad I can emulate the old behavior for a while.

[dtolnay]

Thank you for the heads up, and for your ongoing contributions to this lint!

From reading the summary of rust-lang/rust#116734, I have a hard time making sense of what this lint, as newly formulated, is intended to detect.

The first code snippet suggests that maybe it triggers on "any _ pattern whose matched value could possibly contain within it any enum variant that belongs to a non_exhaustive enum", which seems like a useful formulation.

The second code snippet tells me my first guess cannot be right, because that match contains a wildcard pattern that can match variants of a non_exhaustive enum, but the explanation indicates a lint is not expected to trigger in that case.

My second guess is that it triggers on "any match whose scrutinee contains a 'place' whose type is a non_exhaustive enum, and not every variant of that enum is individually mentioned in that place among all the match arms". But I have no idea how to tell whether this guess is on the right track.

Then from experimenting with some cases myself, I figured out my second guess cannot be right, because a lint does not trigger for match Some(Enum::A) { Some(_) => (), None => () }. (Note: does this make the lint useless for empty non_exhaustive enums, such as syn::ImplRestriction?) My revised guess is that at least one enum variant needs to be matched by name and at least one by wildcard pattern. So the lint is "any match whose scrutinee contains a 'place' whose type is a non_exhaustive enum, at least one variant of that enum is explicitly matched in that place, and at the same time not every variant of that enum is matched in that place among all the match arms". Is that accurate, or could you help me write a more correct statement?

I am starting to think that in the pursuit of being useful, this lint may be getting too smart in a way that makes it challenging for users to use correctly, especially if the failure mode for using it incorrectly is that silently it does nothing when a variant is added upstream.

It's challenging to check whether you have used it correctly. I think you'd need to fork the dependency, introduce a new variant for the non_exhaustive enum you care about, get the dependency's code to compile again by inserting cases into all its exhaustive matches of that enum (this is likely the most time-consuming step), use a patch to depend on the fork, and finally be able to check whether the expected lint appears in your crate.

Compare against the original formulation in rust-lang/rust#81657: lint that triggers if "a specific arm is reachable at runtime for any reason, assuming the match itself is reachable". Compare how this is trivial for a user to use correctly; if they care about finding out about new variants in a particular 'place', they can add an arm that matches if and only if a currently non-existent variant appears. It's easy to predict whether or not the lint will trigger without a nuanced understanding of how it's formulated.

As I recall, the original reason for implementing as a smart deny(non_exhaustive_omitted_patterns) over a simple deny(reachable) was in order to work well as a module-level / crate-level lint. In other words #![deny(non_exhaustive_omitted_patterns)] is a thing that makes sense while #![deny(reachable)] makes no sense.

But in hindsight, is module-level non_exhaustive_omitted_patterns a real use case? From my experience in the prettyplease crate, we occasionally match syn::Expr exhaustively, but more often match syn::Expr non-exhaustively.

[Nadrieril]

Thank you for the heads up, and for your ongoing contributions to this lint!

<3

So the lint is "any match whose scrutinee contains a 'place' whose type is a non_exhaustive enum, at least one variant of that enum is explicitly matched in that place, and at the same time not every variant of that enum is matched in that place among all the match arms". Is that accurate, or could you help me write a more correct statement?

Yep, precisely! Couldn't have said it better.

EDIT: upon rereading I might have misread you; allow me to be pedantic: the lint is "any match whose scrutinee contains a 'place' whose type is a non_exhaustive enum, at least one variant of that enum is explicitly present as a pattern for that place, and at the same time not every variant of that enum is present as a pattern for that place among all the match arms". There are no considerations of matching anymore, just presence or absence of Enum::Variant(..) patterns.

The reason I require at least one variant is for consistency. If I didn't, match Some(Enum::A) { None => (), Some(_) => () } would warn, but not match Some(Enum::A) { _=> () } because we don't dig into wildcards deeper than required by existing patterns. And we can't dig deeper else we'd dig forever. Maybe that consistency is not worth the surprise? I don't have a strong opinion. And yeah that makes the lint useless for empty enums; I'm happy to change that if you think it's better without.

The intuition I'm aiming for is "look at the match, do you see all variants or not?". I think you're greatly overestimating how predictable the old formulation was; imo the rewrite is a ton more predictable. Just an example: in what follows the first match used to trigger the lint and the second didn't, for reasons internal to how exhaustiveness proceeds. rust-lang/rust#116734 has more examples in the tests, specifically this file.

match (true, x) {
    (true, Enum::A) => {}
    (true, Enum::B) => {}
    (false, Enum::C) => {}
    _ => {}
}
match (x, true) {
    (Enum::A, true) => {}
    (Enum::B, true) => {}
    (Enum::C, false) => {}
    _ => {}
}

It's challenging to check whether you have used it correctly.

Can't you just remove an arm that mentions one of the variants and see if the lint triggers?

"look at the match, do you see all variants or not?"

Now that I think about it, that could be an implementation strategy: just look at all the variants mentioned in the match, with no extra cleverness. That would be annoying to implement tho in terms of tracking the types.

As I recall, the original reason for implementing as a smart deny(non_exhaustive_omitted_patterns) over a simple deny(reachable) was in order to work well as a module-level / crate-level lint

I recall a different reason: in the following, deny(reachable) would say that (Foo::A, false) is caught by the wildcard, but we don't care since all the variants are mentioned. For our uses this would require a useless (Foo::A, false) branch to appease the lint.

match x {
  (Foo::A, true) => {}
  (Foo::B, _) => {}
  #[deny(reachable)]
  _ => {}
}