Opt in lint on reachable pattern

[dtolnay]

Filing to track #44109 (comment). Despite https://github.com/dtolnay/syn being an obvious use case for #[non_exhaustive] because we need to be able to add variants over time as new syntax is added to Rust, the thing that makes #[non_exhaustive] unusable is that there is no opt-in way for downstream to request test breakage when one of their wildcard arms becomes reachable.

Our requirements are:

1. It needs to be easy to match a syntax tree node nonexhaustively, the most common case.
2. It needs to be easy for code that uses Syn to opt in to a test failure when Syn adds a variant.
3. It needs to be hard for code that uses code that uses Syn (2 levels removed) to get a build failure when Syn adds a variant.

Item 2 is important for the small fraction of use cases that want to update their code to take into account new syntax, such as a prettyprinter. Item 3 is important so that code downstream of the code that wants to update is never broken in the interim.

Instead of #[non_exhaustive], the idiom we are currently using is to have a hidden variant __TestExhaustive and documenting the supported pattern for exhaustive matches as being:

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

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

This meets requirements 1 and 2, but not 3 because people by and large do not read documentation and will write a dumb exhaustive match that breaks on new variants.

Currently #[non_exhaustive] only meets requirements 1 and 3, but the only thing missing for it to meet 2 is the following lint:

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

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

where reachable is an allow-by-default lint on catch-all arms of a nonexhaustive match.

1. This meets requirement 1 because the lint is allow by default so you just match nonexhaustively and that's it.
2. This meets requirement 2 because you add this deny attribute to opt in to having your tests notify you of new variants. Note that this works even in the absence of any tests; we don't assume existence of a test case that actually exercises the new syntax at runtime.
3. This meets requirement 3 because it can never break crates downstream of the match, due to cap-lints=allow.

@Nadrieril

[Nadrieril]

That would be very easy to do. But conceptually it seems weird to lint for that ^^. Is there precedent for a lint like that?

[Mark-Simulacrum]

Is there a reason to cfg attr the deny? I think with cap lints it won't matter non-locally, though I suppose if you have a large workspace you may not be the maintainer of some of the proc macros.

Generally speaking, this does seem desirable - providing a localized warning in cases where folks do want to know of upstream expansion feels on topic for non_exhaustive.

[Nadrieril]

Is there a reason to cfg attr the deny?

If you make this lint warn in a whole file, then every single match branch in the file will trigger the lint. That would be unfortunate ^^.
That's also why I'm not a huge fan: it would trigger all the time. I'd prefer if we could find a more restricted lint, maybe only for wildcards and #[non_exaustive] together.

I think I'd like an attribute on the wildcard, like:

match expr {
    Expr::Array(e) => {...}
    Expr::Assign(e) => {...}
    ...
    Expr::Yield(e) => {...}
    #[non_exhaustive_fallback] _ => { /* some sane fallback */ }
}

That would trigger a warn-by-default lint. The reason I want the attribute on the wildcard is in case the match is nested:

match expr {
    Foo { x: Expr::Array(e), y: SomethingElse::Foo } => {...}
    ...
    Foo { x: #[non_exhaustive_fallback] _, y: SomethingElse::Foo } => { /* some sane fallback */ }
    Foo { x: _, y: _ } => { /* not a fallback */ }
}

[joshtriplett]

I like the idea of an allow-by-default lint that's specific to hitting the _ branch of a non-exhaustive enum. This gives people options: crates can change it to warn or deny at the top level, or on a specific match or match arm, or change the default but allow it in a specific case they don't care about. And cap-lints will make that entirely a crate-local decision.

[Nadrieril]

Oh yeah I like this. It seems I had misread the initial proposal.

[DevinR528]

This should now be closed because of #86809?

[Nadrieril]

I understand syn now uses the non_exhaustive_omitted_patterns lint to solve the problem described in the OP. I'm closing this, reopen if I got something wrong