Fix: non_exhaustive_omitted_patterns by filtering unstable and doc hidden variants

[DevinR528]

Fixes: #89042

Now that #86809 has been merged there are cases (std::io::ErrorKind) where unstable feature gated variants were included in warning/error messages when the feature was not turned on. This filters those variants out of the return of SplitWildcard::new.

Variants marked doc(hidden) are filtered out of the witnesses list in Usefulness::apply_constructor.

Probably worth a perf run 🤷 since this area can be sensitive.

[rust-highfive]

r? @jackh726

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

[Nadrieril]

Nice, that was quick! I'd just ask for separate tests for the unstable part and the doc(hidden) part. It's cleaner to define our enum than to rely on ErrorKind since it will change over time.

unstable is tricky because it can normally only be defined within std. This seems to work:

#![feature(staged_api)]
#![stable(feature = "stable_test_feature", since = "1.0.0")]

#[unstable(feature = "unstable_test_feature", issue = "none")]
pub enum Foo {
	...
}

Look at e.g. src/test/ui/pattern/usefulness/empty-match.rs and src/test/ui/pattern/usefulness/auxiliary/empty.rs for how to use an external file (which will be compiled as a different crate) in a test.

[Nadrieril]

Btw I'm only revieweing the logic, I don't feel competent to review the actual diagnostic change.
Actually, could you add the tests first, so we see the diagnostic changes in the following commits (you seem quite competent at editing commits; if not don't worry)?

[DevinR528]

Hopefully, these commits are clear enough and I didn't go the other way (splitting them too much so they are hard to interpret as a whole).

[Nadrieril]

Woo, great! LGTM.

I expect a perf degradation because of the extra stability test. The doc(hidden) test is on a diagnostics-only path so won't matter. I expect that match-stress-enum will particularly suffer because it has a ton of variants.
Given that this is a correctness fix we might have to accept the perf regression anyway.

@bors try @rust-timer queue

[rust-timer]

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

[bors]

⌛ Trying commit 6cd3f8afa896b6ad831e7ed202a34ef13a35371c with merge 7504632cc026b5926702afbf29cbd36579ad93fb...

[bors]

☀️ Try build successful - checks-actions
Build commit: 7504632cc026b5926702afbf29cbd36579ad93fb (7504632cc026b5926702afbf29cbd36579ad93fb)

[rust-timer]

Queued 7504632cc026b5926702afbf29cbd36579ad93fb with parent ac2d9fc, future comparison URL.

[rust-timer]

Finished benchmarking commit (7504632cc026b5926702afbf29cbd36579ad93fb): 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.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

[Nadrieril]

@DevinR528 if you're curious, my general rules for splitting commits: each commit should compile and make sense on its own (I also commit the test outputs), and if two changes make sense individually I commit them separately. So I often end up with a ton of small commits ^^'. I haven't completely figured out how to limit that

[DevinR528]

@Nadrieril cool thanks for the commt feedback/ideas! I will defiantly keep that in mind/try to use that as a guide.

Of course, no perf problems when we both expect them 🙄, you just never know.

[Nadrieril]

Merge conflict incoming because of #88950 . It's a rather massive change, but the main difference for this PR is that I've replaced Fields::wildcards(pcx, ctor).apply(pcx, ctor) with DeconstructedPat::wild_from_ctor(pcx, ctor) and Pat::wildcard_from_ty(ty) with DeconstructedPat::wildcard(ty). That PR replaces Pat with my own version that knows about constructors and fields, which should make things simpler.

[DevinR528]

I started rebasing will hopefully have time to finish it today and I'll check out #89382 (comment) (but open separate PR to fix it)

[rust-timer]

Queued aa7536dbf98257fbaa2f6b38c1acb694f6c3cf9a with parent 9a75781, future comparison URL.

[rust-timer]

Finished benchmarking commit (aa7536dbf98257fbaa2f6b38c1acb694f6c3cf9a): comparison url.

Summary: This change led to small relevant regressions 😿 in compiler performance.

• Small regression in instruction counts (up to 0.4% on full builds of cranelift-codegen)

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

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR led to changes in compiler perf.

Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please fix the regressions and do another perf run. If the next run shows neutral or positive results, the label will be automatically removed.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf +perf-regression

[Nadrieril]

Phew, took us a lot of tries but we're there. Any remaining comments @oli-obk? Otherwise r=me after squashing the commits.

[oli-obk]

@bors r=Nadrieril

[bors]

📌 Commit 1c88c9463d1b2a2962485387e8b6d0e85f0a0c0d has been approved by Nadrieril

[Nadrieril]

Hm you pushed the squashed commits after bors recorded approval. I think this should unaccept the PR but bors hasn't noticed yet. Let's unaccept and reaccept.

@bors r-

[Nadrieril]

@bors r+

[bors]

📌 Commit 2a042d6 has been approved by Nadrieril

[DevinR528]

Should I have done something differently or did bors mess up?

[Nadrieril]

🤷‍♀️ Neither, I just clarified a potential confusion around which commit is getting merged.

[bors]

⌛ Testing commit 2a042d6 with merge d7c97a0...

[bors]

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

[rust-timer]

Finished benchmarking commit (d7c97a0): 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