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

Force ADT to be matched exhaustively (opposite of #[non_exhaustive]) #69930

Closed
Aaron1011 opened this issue Mar 11, 2020 · 7 comments
Closed

Force ADT to be matched exhaustively (opposite of #[non_exhaustive]) #69930

Aaron1011 opened this issue Mar 11, 2020 · 7 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-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Aaron1011
Copy link
Member

Aaron1011 commented Mar 11, 2020

The #[non_exhaustive] attribute prevents exhaustively matching an enum outside of the defining crate. However, it's sometimes useful to have the opposite of this behavior - that is, prevent non-exhaustively matching an enum within the defining crate.

For example, non-exhaustively matching on DefPathData or TyKind is often the wrong choice. By writing an exhaustive match (explicitly mentioning all variants), any additions/removals from the enum will require explicit acknowledgment at the match site, helping to prevent bugs.

Examples from rustc:

// Nothing to do for these. Match exhaustively so this fails to compile when new
// variants are added.
StatementKind::AscribeUserType(..)
| StatementKind::FakeRead(..)
| StatementKind::Nop
| StatementKind::Retag(..)
| StatementKind::StorageLive(..) => {}
}

// N.B., deliberately force a compilation error if/when new fields are added.
let TraitItemRef { id, ident, ref kind, span: _, ref defaultness } = *trait_item_ref;

// N.B., deliberately force a compilation error if/when new fields are added.
let ImplItem {
hir_id: _,
ident,
ref vis,
ref defaultness,
attrs,
ref generics,
ref kind,
span: _,
} = *impl_item;

It would be useful to have a compiler lint to enforce this programmatically.

As an initial implementation, we could add an unstable attribute #[rustc_exhaustive] and an internal lint EXHAUSTIVE_MATCH. This lint fires on any non-exhaustive match or destructuring let expression (not if let/while let for single enum variants) that match on an ADT annotated with #[rustc_exhaustive]. Since this would be a lint, it could be allowed on a case-by-case basis (e.g. performing an early exit on TyKind::Error or TyKind::Infer).

If this proves useful, it could be made available to all Rust programs after an RFC.

@Aaron1011 Aaron1011 added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Mar 11, 2020
@Aaron1011 Aaron1011 changed the title Force enum to be match exhaustively (opposite of #[non_exhaustive]) Force enum to be matched exhaustively (opposite of #[non_exhaustive]) Mar 11, 2020
@Aaron1011 Aaron1011 changed the title Force enum to be matched exhaustively (opposite of #[non_exhaustive]) Force ADT to be matched exhaustively (opposite of #[non_exhaustive]) Mar 11, 2020
@cuviper
Copy link
Member

cuviper commented Mar 11, 2020

This also seems similar to #[must_use].

@cuviper cuviper added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-lang Relevant to the language team, which will review and decide on the PR/issue. and removed C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 11, 2020
@Centril
Copy link
Contributor

Centril commented Mar 12, 2020

This lint fires on any non-exhaustive match or destructuring let expression (not if let/while let for single enum variants) that match on an ADT annotated with #[rustc_exhaustive].

The caveat about if let & while let makes this not the dual of #[non_exhaustive], which allows if let on a variant because of the implicit _ pattern from the else branch (it's implemented this way in the compiler due to desugaring to match). Also, if we see if let as a combination of if and let, then the behavior for let diverges in expression and statement form (meaning we cannot fuse the two some day).

I'm also skeptical of this as a property of the ADT itself, although I see why this was chosen. For example, while there are plenty of places where we would e.g. want to match exhaustively on ExprKind there are also situations where we don't care (e.g., in diagnostics). I suspect you suggested if let work this way because of the situational character, but that also weakens the feature.

As for implementation complexity, I defer to @Nadrieril & @varkor.

@Nadrieril
Copy link
Member

Nadrieril commented Mar 13, 2020

On a scale from 1 (only a few lines of code) to 5 (in-depth refactor), I'd expect 2-3 because it goes a bit against the grain of the uniformity of the exhaustiveness algorithm. It would be very easy to just detect uses of the _ pattern for a type with the #[rustc_exhaustive] attribute. It's more painful to detect only those uses of _ that occur after another concrete pattern.

@varkor
Copy link
Member

varkor commented Mar 13, 2020

I'm a bit dubious about how this would work in practice, because you're effectively special-casing the top level of pattern matching. Additionally, I'm not convinced that it's common to have data types which you always want to exhaustively match on. If you want to do an exhaustive match, I think comments telling future contributors not to add a wildcard is enough, and it can be left to the reviewer's judgement to decide whether or not a wildcard is warranted.

@Nadrieril
Copy link
Member

We may not need to special-case the top-level. For example, I would expect the following to trigger the lint:

match Some(x) {
	Some(StatementKind::AscribeUserType(..)) => {}
	_ => {}
}

@jonas-schievink jonas-schievink added A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns C-feature-request Category: A feature request, i.e: not implemented / a PR. and removed C-enhancement Category: An issue proposing an enhancement or a PR with one. labels May 22, 2020
@Nadrieril
Copy link
Member

This is no longer hard to implement at all. It would be doing the same work as the nonexhaustive_omitted_pattern lint.

Only remains to decide if we want this :D

@Nadrieril
Copy link
Member

Nadrieril commented Dec 1, 2023

I think this would belong better as a clippy lint. Given the inactivity on this issue, I'm taking the initiative to close it. Feel free to reopen (or comment) if someone still want this, or to ping me if you open a clippy feature proposal for it.

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-feature-request Category: A feature request, i.e: not implemented / 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