New non_exhaustive_omitted_patterns lint exposes unstable and hidden enum variants.

[m-ou-se]

See #86809 (comment)

The non_exhaustive_omitted_patterns lint exposes #[unstable] and #[doc(hidden)] variants.

E.g. for io::Error:

error: some variants are not matched explicitly
  --> src/main.rs:47:9
   |
47 |         _ => {}
   |         ^ pattern `Uncategorized` not covered
   |
note: the lint level is defined here
  --> src/main.rs:4:12
   |
4  |     #[deny(non_exhaustive_omitted_patterns)]
   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: ensure that all variants are matched explicitly by adding the suggested match arms
   = note: the matched value is of type `ErrorKind` and the `non_exhaustive_omitted_patterns` attribute was found

But Uncategorized is both hidden and unstable, and should not be considered part of the public api.

It'd be good to fix this before it hits stable in 1.57.

[m-ou-se]

(cc @Nadrieril)

[m-ou-se]

I'm not sure if the exact semantics for this lint in case of unstable and hidden variants have been discussed. E.g. whether it should only hide them when the enum comes from a different crate, or whether to warn about #[unstable] variants for which the #![feature] has been enabled.

[Nadrieril]

Is there an enum in std that has #[unstable] variants but isn't #[non_exhaustive] by any chance?
EDIT: I grepped through std and there's only c_void

[DevinR528]

The problem is that std::io::ErrorKind, because it is non_exhaustive, triggers the lint with a NonExhaustive constructor in the list of missing patterns so it will only ever say that it's missing _ (wildcard). This behavior has hidden the need to filter for these variants or care about them at all, as far as the enum lints go.

I was able to get this to give me an odd warning but if you match on something that is the c_void type you either get _ wildcard because your match looks like match c_void {} or you have to turn on the feature so you can add a variant, which then the feature is on so it won't test either way.
error E0423, when constructing the c_void enum without a variant gives

error[E0423]: expected value, found enum `std::ffi::c_void`
  --> src/lib.rs:46:11
   |
46 |     match std::ffi::c_void {};
   |           ^^^^^^^^^^^^^^^^
   |
help: you might have meant to use one of the following enum variants
   |
46 |     match std::os::raw::c_void::__variant1 {};
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
46 |     match std::os::raw::c_void::__variant2 {};
   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Something like this could be done to filter the witness in is_useful before we lint for non_exhaustive_omitted_patterns?

witnesses.retain(|p| {
    if let rustc_middle::thir::PatKind::Variant { adt_def, variant_index, .. } = &*p.kind {
        let did = adt_def.variants[*variant_index].def_id;

        let x = cx.tcx.lookup_stability(did);
        if let Some(Stability { feature, level: StabilityLevel::Unstable { .. }, .. }) = x {
            return cx.tcx.stability().active_features.contains(&feature);
        }
    }
    true
});

As far as doc(hidden) since no other lints take this into account I don't see why this should. IMHO it seems like treating variants marked doc hidden any other way than simply ignoring their docs would be odd 🤷

[Nadrieril]

We'll want to completely ignore those variants even for exhaustiveness checking, so it's best to just not list unstable variants in SplitWildcard::new().
doc(hidden) is more subtle because as you say we don't want to change the logic in that case. But I think we can still omit them from the lints. For that we can do like you did in apply_constructor in your previous PR: filter out those variants from the witnesses list, and replace them with a _.

[DevinR528]

But I think we can still omit them from the lints. For that, we can do like you did in apply_constructor in your previous PR: filter out those variants from the witnesses list, and replace them with a _.

So if an enum (should we do struct fields too?) has a doc(hidden) attribute on a variant and is not crate local we now treat it like it's non_exhaustive and just return a witnesses list of Wildcard?

enum Foo {
    A, B,
    #[doc(hidden)]
    C,
}

// Different crate
match Foo::A {
    Foo::A => {}
    Foo::B => {}
}

match Foo::A {
    Foo::A => {}
    Foo::C => {}
}

Will now give this for the first match (used to be C not _)

error[E0004]: non-exhaustive patterns: `_` not covered
  --> /home/devinr/aprog/rust/__forks__/rust/src/test/ui/rfc-2008-non-exhaustive/reachable-unstable.rs:38:11
   |
LL | / enum Foo {
LL | |     A,
LL | |     B,
LL | |     #[doc(hidden)]
LL | |     C,
LL | | }
   | |_- `Foo` defined here
...
LL |       match Foo::A {
   |             ^^^^^^ pattern `_` not covered
   |
   = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
   = note: the matched value is of type `Foo`

but this for the second

error[E0004]: non-exhaustive patterns: `B` not covered
  --> /home/devinr/aprog/rust/__forks__/rust/src/test/ui/rfc-2008-non-exhaustive/reachable-unstable.rs:43:11
   |
LL | / enum Foo {
LL | |     A,
LL | |     B,
   | |     - not covered
LL | |     #[doc(hidden)]
LL | |     C,
LL | | }
   | |_- `Foo` defined here
...
LL |       match Foo::A {
   |             ^^^^^^ pattern `B` not covered
   |
   = help: ensure that all possible cases are being handled, possibly by adding wildcards or more match arms
   = note: the matched value is of type `Foo`

[Nadrieril]

Yeah exactly! Thanks for the detailed example.
What do you think we should do for the following?

match Foo::A {
    Foo::A => {}
}

I think "patterns B and _ not covered" is better than just "pattern _ not covered" even though it looks a bit weird. Maybe "patterns B and others not covered"?

I didn't know we could doc(hidden) a struct field, but if so then we can do something similar there yeah.

I don't feel competent to make that decision though. @m-ou-se do you know who I could ask something like that?

[DevinR528]

I like "Pattern B and others not covered". I also confirmed the doc(hidden) on a field of a struct thing in the playground so that would have to be taken care of.

Isn't this kind of a breaking change? Or at least a behavioral change. I'm not really qualified but... personally, I don't agree with changing doc(hidden)s warning/lint behavior. My reasoning is that it's a "semi hack" to make up for not having private enums and the things non_exhaustive gives you and as such should probably not be made more special case. Again other than being an avid rust user, I'm not speaking with any authority so grain of salt.

I started #89105 mostly so it's easier to talk about error messages and stuff.

[Nadrieril]

Yeah, I don't know what's the decision process for a small change in a lint message. I'll let whoever got assigned to that PR decide I guess ^^ (and learn for next time)

[camelid]

It'd be good to fix this before it hits stable in 1.57.

Hmm, should this lint be feature-gated so that things like this and its name can get worked out (I found the name a bit hard to understand) without time pressure? IIRC, oftentimes new lints are landed behind feature gates.

[Nadrieril]

Oh I didn't know that was a thing for lints. I'd like that, it's nontrivial enough that more tweaking might be necessary

[camelid]

@Nadrieril Take a look at ec20993; if you do that in reverse and apply it to this lint, I think it should feature-gate it ;)

[camelid]

I don't have enough time to shepherd a feature-gating PR, but this patch should hopefully work if you run ./x.py test --bless on the feature-gate test:

From 8178487a239a5649124ad84bfb3e85824d101e18 Mon Sep 17 00:00:00 2001
From: Noah Lev <[email protected]>
Date: Wed, 29 Sep 2021 13:03:16 -0700
Subject: [PATCH] Feature gate new `non_exhaustive_omitted_patterns` lint

---
 compiler/rustc_feature/src/active.rs                          | 3 +++
 compiler/rustc_lint_defs/src/builtin.rs                       | 2 ++
 compiler/rustc_span/src/symbol.rs                             | 1 +
 .../feature-gate-non_exhaustive_omitted_patterns.rs           | 4 ++++
 4 files changed, 10 insertions(+)
 create mode 100644 src/test/ui/feature-gates/feature-gate-non_exhaustive_omitted_patterns.rs

diff --git a/compiler/rustc_feature/src/active.rs b/compiler/rustc_feature/src/active.rs
index f8b865e615c..9c0806dae16 100644
--- a/compiler/rustc_feature/src/active.rs
+++ b/compiler/rustc_feature/src/active.rs
@@ -678,6 +678,9 @@ pub fn set(&self, features: &mut Features, span: Span) {
     /// Allows `#[track_caller]` on closures and generators.
     (active, closure_track_caller, "1.57.0", Some(87417), None),
 
+    /// Allows using the `non_exhaustive_omitted_patterns` lint.
+    (active, non_exhaustive_omitted_patterns, "1.57.0", None, None),
+
     // -------------------------------------------------------------------------
     // feature-group-end: actual feature gates
     // -------------------------------------------------------------------------
diff --git a/compiler/rustc_lint_defs/src/builtin.rs b/compiler/rustc_lint_defs/src/builtin.rs
index 5830ce26fea..8a4d21f5d11 100644
--- a/compiler/rustc_lint_defs/src/builtin.rs
+++ b/compiler/rustc_lint_defs/src/builtin.rs
@@ -6,6 +6,7 @@
 
 use crate::{declare_lint, declare_lint_pass, FutureIncompatibilityReason};
 use rustc_span::edition::Edition;
+use rustc_span::symbol::sym;
 
 declare_lint! {
     /// The `forbidden_lint_groups` lint detects violations of
@@ -3510,4 +3511,5 @@
     pub NON_EXHAUSTIVE_OMITTED_PATTERNS,
     Allow,
     "detect when patterns of types marked `non_exhaustive` are missed",
+    @feature_gate = sym::non_exhaustive_omitted_patterns;
 }
diff --git a/compiler/rustc_span/src/symbol.rs b/compiler/rustc_span/src/symbol.rs
index 7cb4e18398c..6b89cb6b4aa 100644
--- a/compiler/rustc_span/src/symbol.rs
+++ b/compiler/rustc_span/src/symbol.rs
@@ -887,6 +887,7 @@
         nomem,
         non_ascii_idents,
         non_exhaustive,
+        non_exhaustive_omitted_patterns,
         non_modrs_mods,
         none_error,
         nontemporal_store,
diff --git a/src/test/ui/feature-gates/feature-gate-non_exhaustive_omitted_patterns.rs b/src/test/ui/feature-gates/feature-gate-non_exhaustive_omitted_patterns.rs
new file mode 100644
index 00000000000..9952b4b3283
--- /dev/null
+++ b/src/test/ui/feature-gates/feature-gate-non_exhaustive_omitted_patterns.rs
@@ -0,0 +1,4 @@
+#![deny(non_exhaustive_omitted_patterns)] //~ ERROR unstable
+#![allow(non_exhaustive_omitted_patterns)] //~ ERROR unstable
+
+fn main() {}
-- 
2.29.2

[Nadrieril]

I don't know how to shepherd that either >< Does there need to be a tracking issue and things like that? Would you know someone competent to ping?

[camelid]

I don't know how to shepherd that either >< Does there need to be a tracking issue and things like that? Would you know someone competent to ping?

Ah, well, I don't think there's not too much involved. I meant more that if CI fails and there are complications, I don't have time to shepherd that.

I think you could just apply the patch I posted, run x.py test --bless on the feature-gate test, and open a PR.

In terms of tracking issues, I think it's most important to just get it behind a feature gate first. You can always open a tracking issue and add it to the feature gate later.

[DevinR528]

I can work on adding the feature gate I'll open a PR and cc both of you (as long as that's ok?).

[Nadrieril]

Hm, can there be a struct that has some unstable or doc(hidden) fields? If yes then exhaustiveness will have incorrect behavior for that too; we might need another PR ><.

[DevinR528]

I did start investigating structs and unstable fields (it fails the same way the enums used to). I have tests that fail and hopefully I'll have time tomorrow or the day after to filter the unstable gated fields (and potentially doc(hidden) too) I can add it to #89105 or open a new one?

[camelid]

Also, what about unstable or doc(hidden) enum variant fields? They may need to be handled as well.

[DevinR528]

I'm pretty sure we get those for "free" once struct fields are taken care of (both unstable feature gating and doc(hidden)).

[camelid]

Ok, well I guess the testsuite will confirm that :)

[Nadrieril]

Oh yay do go ahead! I'd prefer a new PR, #89105 is almost done. Also once it is done we'll know what we want for the lint message, so then I can review and merge myself without bothering other team members.