Rewrite non_exhaustive_omitted_patterns as a separate pass
#111651
Conversation
This makes things unnecessarily complicated for now
|
r? @eholk (rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit d5edcb1 with merge 390ac690ed0d597a9d018264c02b97ecad184aa3... |
|
☀️ Try build successful - checks-actions |
This comment has been minimized.
This comment has been minimized.
|
I'm starting to look over this now, but let me first ask to make sure I understand the intent of the lint. The idea is that you want to make sure you're matching against all currently known variants of an enum, right? In other words, |
|
Finished benchmarking commit (390ac690ed0d597a9d018264c02b97ecad184aa3): comparison URL. Overall result: ❌ regressions - ACTION NEEDEDBenchmarking 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 may lead to changes in compiler perf. Next Steps: If you can justify the regressions found in this try perf run, please indicate this with @bors rollup=never Instruction countThis is a highly reliable metric that was used to determine the overall result at the top of this comment.
Max RSS (memory usage)ResultsThis is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 642.522s -> 642.237s (-0.04%) |
rustbot
added
perf-regression
|
Yeah I think you got the idea. It's basically a "please remind me if someone adds a variant to this enum because I want to handle all of them". Very annoyed at the perf result, how can it be worse I just removed code and added a single top-level check :'( Will have to investigate |
| .0 | ||
| .iter() | ||
| .chain(once(pat)) | ||
| .map(DeconstructedPat::clone_and_forget_reachability) |
There was a problem hiding this comment.
Why is it okay not to forget reachability now? Is there a risk of having stale or incomplete reachability information?
There was a problem hiding this comment.
Nah this changes nothing, it's just that I hadn't implemented Clone to make sure no one cloned stuff by mistake. But it's inconvenient and there's not sensible situation where one would get confused. I have plans to clear that up more in the future
| match NonExhaustiveSingleVariant::A(true) { | ||
| NonExhaustiveSingleVariant::A(true) => {} | ||
| _ => {} | ||
| } | ||
|
|
||
| #[deny(non_exhaustive_omitted_patterns)] | ||
| // Don't lint if no variant is mentioned, because we can't do it consistently. |
There was a problem hiding this comment.
Take for example
match Some(&NonExhaustiveEnum::A) {
_ => {}
}In order to lint here I'd need to know that the enum exists at all, which means I'd need to dig into the _ pattern, before even knowing if there might be an interesting enum somewhere deep. I can't just dig into all wildcards like that though, that would destroy performance.
We could think about somehow computing ahead of time for all types whether they have some non_exhaustive enum deep down, but that doesn't seem worth the effort.
|
Alright so I'm kinda rewriting the exhaustiveness algorithm big time over in #111720 x). It's a rewrite I've been itching to do for years but could never get right. If it works this time, the performance argument I stated in the OP is moot. In fact what I'm doing here is a lot slower because it's a whole separate pass. So hum I don't know what to do anymore. There's still a discussion to be had about what we want the lint to do but I don't think this PR is the right solution anymore. I'll close this for now and maybe come back to it when I'm done with the other one. Thanks for taking the time to review! |
|
I revived this in #116734 |
Lint `non_exhaustive_omitted_patterns` by columns
This is a rework of the `non_exhaustive_omitted_patterns` lint to make it more consistent. The intent of the lint is to help consumers of `non_exhaustive` enums ensure they stay up-to-date with all upstream variants. This rewrite fixes two cases we didn't handle well before:
First, because of details of exhaustiveness checking, the following wouldn't lint `Enum::C` as missing:
```rust
match Some(x) {
Some(Enum::A) => {}
Some(Enum::B) => {}
_ => {}
}
```
Second, because of the fundamental workings of exhaustiveness checking, the following would treat the `true` and `false` cases separately and thus lint about missing variants:
```rust
match (true, x) {
(true, Enum::A) => {}
(true, Enum::B) => {}
(false, Enum::C) => {}
_ => {}
}
```
Moreover, it would correctly not lint in the case where the pair is flipped, because of asymmetry in how exhaustiveness checking proceeds.
A drawback is that it no longer makes sense to set the lint level per-arm. This will silently break the lint for current users of it (but it's behind a feature gate so that's ok).
The new approach is now independent of the exhaustiveness algorithm; it's a separate pass that looks at patterns column by column. This is another of the motivations for this: I'm glad to move it out of the algorithm, it was akward there.
This PR is almost identical to rust-lang#111651. cc `@eholk` who reviewed it at the time. Compared to then, I'm more confident this is the right approach.
Lint `non_exhaustive_omitted_patterns` by columns
This is a rework of the `non_exhaustive_omitted_patterns` lint to make it more consistent. The intent of the lint is to help consumers of `non_exhaustive` enums ensure they stay up-to-date with all upstream variants. This rewrite fixes two cases we didn't handle well before:
First, because of details of exhaustiveness checking, the following wouldn't lint `Enum::C` as missing:
```rust
match Some(x) {
Some(Enum::A) => {}
Some(Enum::B) => {}
_ => {}
}
```
Second, because of the fundamental workings of exhaustiveness checking, the following would treat the `true` and `false` cases separately and thus lint about missing variants:
```rust
match (true, x) {
(true, Enum::A) => {}
(true, Enum::B) => {}
(false, Enum::C) => {}
_ => {}
}
```
Moreover, it would correctly not lint in the case where the pair is flipped, because of asymmetry in how exhaustiveness checking proceeds.
A drawback is that it no longer makes sense to set the lint level per-arm. This will silently break the lint for current users of it (but it's behind a feature gate so that's ok).
The new approach is now independent of the exhaustiveness algorithm; it's a separate pass that looks at patterns column by column. This is another of the motivations for this: I'm glad to move it out of the algorithm, it was akward there.
This PR is almost identical to rust-lang#111651. cc `@eholk` who reviewed it at the time. Compared to then, I'm more confident this is the right approach.
Lint `non_exhaustive_omitted_patterns` by columns
This is a rework of the `non_exhaustive_omitted_patterns` lint to make it more consistent. The intent of the lint is to help consumers of `non_exhaustive` enums ensure they stay up-to-date with all upstream variants. This rewrite fixes two cases we didn't handle well before:
First, because of details of exhaustiveness checking, the following wouldn't lint `Enum::C` as missing:
```rust
match Some(x) {
Some(Enum::A) => {}
Some(Enum::B) => {}
_ => {}
}
```
Second, because of the fundamental workings of exhaustiveness checking, the following would treat the `true` and `false` cases separately and thus lint about missing variants:
```rust
match (true, x) {
(true, Enum::A) => {}
(true, Enum::B) => {}
(false, Enum::C) => {}
_ => {}
}
```
Moreover, it would correctly not lint in the case where the pair is flipped, because of asymmetry in how exhaustiveness checking proceeds.
A drawback is that it no longer makes sense to set the lint level per-arm. This will silently break the lint for current users of it (but it's behind a feature gate so that's ok).
The new approach is now independent of the exhaustiveness algorithm; it's a separate pass that looks at patterns column by column. This is another of the motivations for this: I'm glad to move it out of the algorithm, it was akward there.
This PR is almost identical to rust-lang#111651. cc `@eholk` who reviewed it at the time. Compared to then, I'm more confident this is the right approach.
Lint `non_exhaustive_omitted_patterns` by columns
This is a rework of the `non_exhaustive_omitted_patterns` lint to make it more consistent. The intent of the lint is to help consumers of `non_exhaustive` enums ensure they stay up-to-date with all upstream variants. This rewrite fixes two cases we didn't handle well before:
First, because of details of exhaustiveness checking, the following wouldn't lint `Enum::C` as missing:
```rust
match Some(x) {
Some(Enum::A) => {}
Some(Enum::B) => {}
_ => {}
}
```
Second, because of the fundamental workings of exhaustiveness checking, the following would treat the `true` and `false` cases separately and thus lint about missing variants:
```rust
match (true, x) {
(true, Enum::A) => {}
(true, Enum::B) => {}
(false, Enum::C) => {}
_ => {}
}
```
Moreover, it would correctly not lint in the case where the pair is flipped, because of asymmetry in how exhaustiveness checking proceeds.
A drawback is that it no longer makes sense to set the lint level per-arm. This will silently break the lint for current users of it (but it's behind a feature gate so that's ok).
The new approach is now independent of the exhaustiveness algorithm; it's a separate pass that looks at patterns column by column. This is another of the motivations for this: I'm glad to move it out of the algorithm, it was akward there.
This PR is almost identical to rust-lang#111651. cc `@eholk` who reviewed it at the time. Compared to then, I'm more confident this is the right approach.
Lint `non_exhaustive_omitted_patterns` by columns
This is a rework of the `non_exhaustive_omitted_patterns` lint to make it more consistent. The intent of the lint is to help consumers of `non_exhaustive` enums ensure they stay up-to-date with all upstream variants. This rewrite fixes two cases we didn't handle well before:
First, because of details of exhaustiveness checking, the following wouldn't lint `Enum::C` as missing:
```rust
match Some(x) {
Some(Enum::A) => {}
Some(Enum::B) => {}
_ => {}
}
```
Second, because of the fundamental workings of exhaustiveness checking, the following would treat the `true` and `false` cases separately and thus lint about missing variants:
```rust
match (true, x) {
(true, Enum::A) => {}
(true, Enum::B) => {}
(false, Enum::C) => {}
_ => {}
}
```
Moreover, it would correctly not lint in the case where the pair is flipped, because of asymmetry in how exhaustiveness checking proceeds.
A drawback is that it no longer makes sense to set the lint level per-arm. This will silently break the lint for current users of it (but it's behind a feature gate so that's ok).
The new approach is now independent of the exhaustiveness algorithm; it's a separate pass that looks at patterns column by column. This is another of the motivations for this: I'm glad to move it out of the algorithm, it was akward there.
This PR is almost identical to rust-lang/rust#111651. cc `@eholk` who reviewed it at the time. Compared to then, I'm more confident this is the right approach.
Nadrieril
added
the
F-non_exhaustive_omitted_patterns_lint
This is a rework of the
non_exhaustive_omitted_patternslint. It changes the semantics of the lint for two reasons:The new lint ignores considerations of exhaustiveness. Instead it looks at any part of the match that mentions some variants of a non_exhaustive enum, and triggers the lint if the match doesn't mention all the variants. That way users can ensure they update their code to mention all variants.
I also fixed some other inconsistencies in how the lint was implemented.
This separate pass is probably slow (but optional). I wouldn't recommend enabling the lint crate-wide. That said, correctness comes before performance, and I have plans that should make it much better.