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

Add an allow-by-default lint that triggers on reachable catch-all / rest patterns #84332

Closed
jplatte opened this issue Apr 19, 2021 · 5 comments · Fixed by #86809
Closed

Add an allow-by-default lint that triggers on reachable catch-all / rest patterns #84332

jplatte opened this issue Apr 19, 2021 · 5 comments · Fixed by #86809
Assignees
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. A-patterns Relating to patterns and pattern matching 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

@jplatte
Copy link
Contributor

jplatte commented Apr 19, 2021

syn currently allows users to opt into having their tests fail if a match of one of the enums it defines is non-exhaustive:

match expr {
    Expr::Array(e) => {...}
    Expr::Assign(e) => {...}
    ...
    Expr::Yield(e) => {...}

    #[cfg(test)]
    Expr::__TestExhaustive(_) => unimplemented!(),
    #[cfg(not(test))]
    _ => { /* some sane fallback */ }
}

In the tracking issue for the #[non_exhaustive] attribute, it was discussed to allow this in a less hacky way, for truly non-exhaustive enums, with dtolnay suggesting a lint (#44109 (comment)) like this:

match expr {
    Expr::Array(e) => {...}
    Expr::Assign(e) => {...}
    ...
    Expr::Yield(e) => {...}

    #[cfg_attr(test, deny(reachable))]
    _ => { /* some sane fallback */ }
}

Later, a clippy issue was opened about the same thing and a PR opened to address it. However, that PR was closed because

the thing is that rustc already has exhaustiveness checks, and this is a lint that was proposed in a Rust RFC, so it would both be easy to implement in rustc and make sense. It feels weird to reimplement exhaustiveness checking in a partial way outside of rustc when we can have a perfect version of this lint in rustc.

rust-lang/rust-clippy#6328 (comment)

@jplatte
Copy link
Contributor Author

jplatte commented Apr 19, 2021

Unresolved questions:

  • Should this be an attribute on the match expression like the clippy lint from the PR, e.g. not_exhaustive_enough or an attribute on the rest pattern, e.g. reachable? The former would also make sense to be used in a larger context, e.g. warn on all matches with reachable catch-alls in an entire module, or even crate. IMHO, the former could easily be a footgun, so I favor the latter.
  • Should this attribute also be supported on struct matches? I actually have a use case for this¹, so would be happy to see this supported. If the lint is decided to be reachablerather than not_exhaustive_enough, this would first require the ability to use attributes on .. in struct patterns, which currently don't produce an error, but don't do anything (Attributes are silently ignored on .. in struct pattern #81282).

I would be happy to work on this if somebody can give me some pointers.

¹ allowing one crate to closely replicate a non-exhaustive struct from another crate, with some specific adjustments

@jonas-schievink jonas-schievink added A-lint Area: Lints (warnings about flaws in source code) such as unused_mut. A-patterns Relating to patterns and pattern matching 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. labels Apr 19, 2021
@jyn514 jyn514 added the A-exhaustiveness-checking Relating to exhaustiveness / usefulness checking of patterns label Apr 19, 2021
@Nadrieril
Copy link
Member

Is this covered by the discussion on issue #69930 or is it distinct?

@jplatte
Copy link
Contributor Author

jplatte commented May 14, 2021

No, I don't think so. That issue (and the surrounding discussion) is only about matches on non-exhaustive types in the crate in which they are defined, while this one is about matching external non-exhaustive types, where a catch-all branch can't be avoided but where it's still sometimes useful to know when it becomes reachable with a depedency upgrade.

@DevinR528
Copy link
Contributor

@rustbot claim

@Nadrieril
Copy link
Member

Nadrieril commented Sep 12, 2021

There's a subtlety I wanted to point out if someone finds this issue: deny(reachable) is subtly different from what I gather to be the intention of this issue. In the following example, the wildcard is reachable, so deny(reachable) on the wildcard would trigger. But I think the intent of this issue is that we shouldn't trigger on this match since clearly the user knows about all the variants of this enum.
The lint about to be merged in #86809 does the other thing, namely only triggers if some variant is not mentioned. It does not trigger on this example.

#[non_exhaustive]
enum Foo {
  A(bool),
  B
}
match foo {
  Foo::A(true) => {}
  Foo::B => {}
  _ => {}
}

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. A-patterns Relating to patterns and pattern matching 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

Successfully merging a pull request may close this issue.

5 participants