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

Tracking Issue for #![feature(non_exhaustive_omitted_patterns_lint)] #89554

Open
11 of 14 tasks
Nadrieril opened this issue Oct 5, 2021 · 20 comments
Open
11 of 14 tasks

Tracking Issue for #![feature(non_exhaustive_omitted_patterns_lint)] #89554

Nadrieril opened this issue Oct 5, 2021 · 20 comments
Labels
A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-non_exhaustive_omitted_patterns_lint `#![feature(non_exhaustive_omitted_patterns_lint)]` F-non_exhaustive `#![feature(non_exhaustive)]` S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Nadrieril
Copy link
Member

Nadrieril commented Oct 5, 2021

This is a tracking issue for the non_exhaustive_omitted_patterns lint.
The feature gate for the issue is #![feature(non_exhaustive_omitted_patterns_lint)].

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also used as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

Unresolved Questions

Implementation history

@Nadrieril Nadrieril added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns F-non_exhaustive_omitted_patterns_lint `#![feature(non_exhaustive_omitted_patterns_lint)]` T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Oct 5, 2021
@Nadrieril Nadrieril added the F-non_exhaustive `#![feature(non_exhaustive)]` label Oct 5, 2021
@DevinR528

This comment was marked as resolved.

@Nadrieril
Copy link
Member Author

In terms of the "documentation" bullet-point, the lint should turn up automatically on that list and I think that's enough. So we'll only need to check that it says the right thing once we've ironed out the name and message etc.

@DevinR528
Copy link
Contributor

The question about not filtering doc(hidden) (and maybe this was only me feeling like it was odd) is kind of resolved for me since we unified the behavior so I no longer have the argument that filtering doc(hidden) variants/fields out of error and warning messages is not the norm (now it is done for all exhaustiveness checking).

I'm also curious about the stability of warning/error messages, I assume rust makes no guarantee so they can be improved quickly.

@Nadrieril
Copy link
Member Author

Yeah, don't think there's any formal guarantee and they do change in small ways regularly

bors added a commit to rust-lang-ci/rust that referenced this issue Mar 12, 2022
Fix exposing fields marked unstable or doc hidden

Closes rust-lang#89837

Work towards rust-lang#89554

Filter fields that are marked `doc(hidden)` or are unstable with that feature turned off. This brings structs and enums into alignment behavior-wise when emitting warning/errors about pattern exhaustiveness/reachability.

cc `@Nadrieril`
jgallag88 added a commit to jgallag88/ptools that referenced this issue Apr 27, 2022
Update everything to latest except for nix. In nix v0.23, a number of
enums were marked #[non_exhaustive]
(nix-rust/nix#1474). Since we want exhaustive
matches in some cases, we will use v0.22 until
non_exhaustive_omitted_patterns_lint is stablized
(rust-lang/rust#89554)
jgallag88 added a commit to jgallag88/ptools that referenced this issue Apr 27, 2022
Update everything to latest except for nix. In nix v0.23, a number of
enums were marked #[non_exhaustive]
(nix-rust/nix#1474). Since we want exhaustive
matches in some cases, we will use v0.22 until
non_exhaustive_omitted_patterns_lint is stablized
(rust-lang/rust#89554)
jgallag88 added a commit to jgallag88/ptools that referenced this issue Apr 27, 2022
Update everything to latest except for nix. In nix v0.23, a number of
enums were marked #[non_exhaustive]
(nix-rust/nix#1474). Since we want exhaustive
matches in some cases, we will use v0.22 until
non_exhaustive_omitted_patterns_lint is stablized
(rust-lang/rust#89554)
jgallag88 added a commit to jgallag88/ptools that referenced this issue Apr 28, 2022
Update everything to latest except for nix. In nix v0.23, a number of
enums were marked #[non_exhaustive]
(nix-rust/nix#1474). Since we want exhaustive
matches in some cases, we will use v0.22 until
non_exhaustive_omitted_patterns_lint is stablized
(rust-lang/rust#89554)
@Kampfkarren
Copy link
Contributor

I would like to move forward on this as we are currently using this in selene, but gated by nightly-only CI.

The "Lint message needs to be improved" is doable.

Unsure how the lint name could be improved. CC @camelid since you brought up the concern

@Nemo157

This comment was marked as off-topic.

@camelid

This comment was marked as off-topic.

@camelid
Copy link
Member

camelid commented Jul 27, 2022

I think I've come up with a better name for the lint: what about non_exhaustive_wildcards or wildcards_for_non_exhaustive? Then denying it would look like #[deny(non_exhaustive_wildcards)] or #[deny(wildcards_for_non_exhaustive)]. I think that is more self-explanatory.

@joshtriplett joshtriplett added the S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR label Jul 27, 2022
@Nemo157

This comment was marked as off-topic.

@DevinR528
Copy link
Contributor

I don't have strong feelings either way, so whatever the outcome, I'll like it 😄 . My thought about non_exhaustive_wildcards is that it sounds like you are forbidding the use of wildcards. What about #[deny(missing_non_exhaustive_patterns)]?

@camelid
Copy link
Member

camelid commented Jul 31, 2022

I like your suggestion!

@Kampfkarren
Copy link
Contributor

I would like to file a complaint about the lint message here, unsure if you would want a separate bug report.

Currently, I get the following error message:

error: some variants are not matched explicitly
   |
note: the lint level is defined here
  --> selene-lib/src/rules/high_cyclomatic_complexity.rs:77:14
   |
77 |         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 `full_moon::ast::Value` and the `non_exhaustive_omitted_patterns` attribute was found

error: some variants are not matched explicitly
  |
  = help: ensure that all variants are matched explicitly by adding the suggested match arms
  = note: the matched value is of type `full_moon::ast::Expression` and the `non_exhaustive_omitted_patterns` attribute was found

error: some variants are not matched explicitly
    |
note: the lint level is defined here
   --> selene-lib/src/rules/high_cyclomatic_complexity.rs:180:18
    |
180 |             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 `full_moon::ast::Stmt` and the `non_exhaustive_omitted_patterns` attribute was found

error: could not compile `selene-lib` due to 3 previous errors

This error message is correct, but not helpful, and confused the person writing the pull request who hit this issue. It does not show what the variants are or where the match is. I would file a repro but I'm fairly sure non_exhaustive is ignored in the same crate, so it would not be doable on playground.

@camelid
Copy link
Member

camelid commented Sep 14, 2022

Hmm, it does seem like a bug that no source code is shown. Can you file an issue? I'm not sure I followed why the MCVE wouldn't be possible, but if you could provide something like that, that would be helpful.

@Kampfkarren
Copy link
Contributor

Sure. An MVCE is possible, but non_exhaustive is only enforced when the non_exhaustive item is in a separate crate, so it just can't be a playground.

@camelid
Copy link
Member

camelid commented Sep 14, 2022

Ah, I see what you mean. Yeah, an MCVE would be good even if it can't be on the playground.

dtolnay added a commit to dtolnay/linkme that referenced this issue Feb 19, 2023
    error: unknown lint: `non_exhaustive_omitted_patterns`
       --> impl/src/declaration.rs:297:43
        |
    297 |         #[cfg_attr(all(test, exhaustive), deny(non_exhaustive_omitted_patterns))]
        |                                           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
        |
        = note: the `non_exhaustive_omitted_patterns` lint is unstable
        = note: see issue #89554 <rust-lang/rust#89554> for more information
        = help: add `#![feature(non_exhaustive_omitted_patterns_lint)]` to the crate attributes to enable
        = note: `-D unknown-lints` implied by `-D warnings`
@Nadrieril
Copy link
Member Author

Nadrieril commented May 15, 2023

I just realized that there is a pretty fundamental limitation with this lint. Observe the following two cases:

#![deny(non_exhaustive_omitted_patterns)]
match (&x, true) {
    (NonExhaustiveEnum::A, true) => {}
    _ => {} // detected
}
match (true, &x) {
    (true, NonExhaustiveEnum::A) => {}
    _ => {} // not detected
}

The second is not detected because in the exhaustiveness checker as soon as we figure out that (false, _) makes the second branch reachable, we don't analyze it further. Something similar would happen with a Some(NonExhaustiveEnum::A) pattern.
In order for this lint to work properly with nested patterns, we would need to change the exhaustiveness algorithm to explore a lot more cases, which isn't feasible performance-wise.

An alternative would be a more naive version that doesn't interact with exhaustiveness. It would just check that each column mentions all variants. This means it would not trigger on the following, which we initially wanted. This feels fine to me.

#[deny(non_exhaustive_omitted_patterns)]
match foo {
    Alphabet::A => {}
    Alphabet::B if guard => {}
    _ => {}
}

EDIT: in fact since this doesn't interact with exhaustiveness it can be a clippy lint, which feels more appropriate

@Nadrieril
Copy link
Member Author

#116734 changed the behavior of the lint to match what I proposed above

@Nadrieril
Copy link
Member Author

Nadrieril commented Oct 23, 2023

Note: the lint level can no longer be put on a match arm, it needs to be on the whole match. This is a silent breaking change.

I thought it was ok because the feature is unstable, but turns out syn is using the lint and recommending to put the lint level on the match arm. I'll try to make the breaking change not silent at least.

EDIT: did that in #117094

bors added a commit to rust-lang-ci/rust that referenced this issue Oct 27, 2023
Warn users who set `non_exhaustive_omitted_patterns` lint level on a match arm

Before rust-lang#116734, the recommended usage of the [`non_exhaustive_omitted_patterns` lint](rust-lang#89554) was:
```rust
match Bar::A {
    Bar::A => {},
    #[warn(non_exhaustive_omitted_patterns)]
    _ => {},
}
```
After rust-lang#116734 this no longer makes sense, and recommended usage is now:
```rust
#[warn(non_exhaustive_omitted_patterns)]
match Bar::A {
    Bar::A => {},
    _ => {},
}
```

As you can guess, this silently breaks all uses of the lint that used the previous form. This is a problem in particular because `syn` recommends usage of this lint to its users in the old way. This PR emits a warning when the previous form is used so users can update.

r? `@cjgillot`
bors added a commit to rust-lang-ci/rust that referenced this issue Nov 4, 2023
Warn users who set `non_exhaustive_omitted_patterns` lint level on a match arm

Before rust-lang#116734, the recommended usage of the [`non_exhaustive_omitted_patterns` lint](rust-lang#89554) was:
```rust
match Bar::A {
    Bar::A => {},
    #[warn(non_exhaustive_omitted_patterns)]
    _ => {},
}
```
After rust-lang#116734 this no longer makes sense, and recommended usage is now:
```rust
#[warn(non_exhaustive_omitted_patterns)]
match Bar::A {
    Bar::A => {},
    _ => {},
}
```

As you can guess, this silently breaks all uses of the lint that used the previous form. This is a problem in particular because `syn` recommends usage of this lint to its users in the old way. This PR emits a warning when the previous form is used so users can update.

r? `@cjgillot`
@Nadrieril
Copy link
Member Author

Nadrieril commented Dec 8, 2023

I discovered something cool today: non_exhaustive_omitted_patterns will warn in the following case if we add variants to Enum, which an exhaustiveness-based check wouldn't catch:

#[warn(non_exhaustive_omitted_patterns)]
match (x, y) {
    (Enum::A, Enum::A) => true,
    (Enum::B, Enum::B) => true,
    _ => false,
}

I kind of want this for arbitrary enums now.

@Nadrieril
Copy link
Member Author

Maybe this could be a general #[warn(mention_all_variants_of(Enum))]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-non_exhaustive_omitted_patterns_lint `#![feature(non_exhaustive_omitted_patterns_lint)]` F-non_exhaustive `#![feature(non_exhaustive)]` S-tracking-ready-to-stabilize Status: This is ready to stabilize; it may need a stabilization report and a PR T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants