Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rewrite non_exhaustive_omitted_patterns as a separate pass #111651

Closed
wants to merge 3 commits into from

Conversation

Nadrieril
Copy link
Member

This is a rework of the non_exhaustive_omitted_patterns lint. It changes the semantics of the lint for two reasons:

  • the previous implementation was inconsistent and hard to make consistent without impacting performance of exhaustiveness checking in general (discussed here);
  • this matches more closely the intent of the lint as I understand it, which is to help users keep their code up-to-date with upstream changes.

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.

@rustbot
Copy link
Collaborator

rustbot commented May 16, 2023

r? @eholk

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels May 16, 2023
@Nadrieril
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label May 16, 2023
@bors
Copy link
Contributor

bors commented May 16, 2023

⌛ Trying commit d5edcb1 with merge 390ac690ed0d597a9d018264c02b97ecad184aa3...

@bors
Copy link
Contributor

bors commented May 16, 2023

☀️ Try build successful - checks-actions
Build commit: 390ac690ed0d597a9d018264c02b97ecad184aa3 (390ac690ed0d597a9d018264c02b97ecad184aa3)

@rust-timer

This comment has been minimized.

@eholk
Copy link
Contributor

eholk commented May 16, 2023

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, #[non_exhaustive] means you must have a wildcard, while this lint makes sure that you are still doing an exhaustive match, even if it wouldn't be a compilation error if more variants were added in the future. Is that the idea?

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (390ac690ed0d597a9d018264c02b97ecad184aa3): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

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 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 @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-perf +perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
0.3% [0.2%, 0.3%] 5
Regressions ❌
(secondary)
3.0% [2.8%, 3.2%] 6
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.3% [0.2%, 0.3%] 5

Max RSS (memory usage)

Results

This 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.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
0.9% [0.9%, 0.9%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 642.522s -> 642.237s (-0.04%)

@rustbot rustbot added perf-regression Performance regression. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels May 16, 2023
Copy link
Contributor

@eholk eholk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still working on this, but I need to take a break so I wanted to post the couple of comments I have so far.

compiler/rustc_lint_defs/src/builtin.rs Show resolved Hide resolved
compiler/rustc_lint_defs/src/builtin.rs Show resolved Hide resolved
@Nadrieril
Copy link
Member Author

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

Copy link
Contributor

@eholk eholk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks pretty good to me! I'll hold off on sending it to bors so you can chase the performance issues if you want.

I did have a couple questions that I added.

.0
.iter()
.chain(once(pat))
.map(DeconstructedPat::clone_and_forget_reachability)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is it okay not to forget reachability now? Is there a risk of having stale or incomplete reachability information?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting! Why not?

Copy link
Member Author

@Nadrieril Nadrieril May 19, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Nadrieril
Copy link
Member Author

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!

@Nadrieril
Copy link
Member Author

I revived this in #116734

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2023
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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2023
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.
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2023
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.
@Nadrieril Nadrieril deleted the lint-omitted-by-columns branch October 21, 2023 01:39
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 21, 2023
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.
github-actions bot pushed a commit to rust-lang/miri that referenced this pull request Oct 22, 2023
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 Nadrieril added the F-non_exhaustive_omitted_patterns_lint `#![feature(non_exhaustive_omitted_patterns_lint)]` label Dec 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
F-non_exhaustive_omitted_patterns_lint `#![feature(non_exhaustive_omitted_patterns_lint)]` perf-regression Performance regression. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants