Skip to content

Opt in lint on reachable pattern #81657

Closed

Description

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Assignees

No one assigned

    Labels

    A-exhaustiveness-checkingRelating to exhaustiveness / usefulness checking of patternsA-lintArea: Lints (warnings about flaws in source code) such as unused_mut.T-langRelevant to the language team, which will review and decide on the PR/issue.

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions