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

RFC: exhaustive matching idiom #694

Closed
dtolnay opened this issue Aug 14, 2019 · 10 comments · Fixed by #962
Closed

RFC: exhaustive matching idiom #694

dtolnay opened this issue Aug 14, 2019 · 10 comments · Fixed by #962

Comments

@dtolnay
Copy link
Owner

dtolnay commented Aug 14, 2019

Here is the pattern I would like to document and recommend for use cases that require exhaustive matching (which is uncommon, but they exist):

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

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

This way we fail your tests but don't break your library when adding a variant. I believe this has all the benefits of an exhaustive enum and all the benefits of a nonexhaustive enum for only a little effort.

FYI @zerakun @phaylon @BurntSushi from the reddit thread.
FYI @mystor for brainstorming.
FYI @Centril because language support for nonexhaustive would need to have at least these benefits before I would want to pick it up.

@CAD97
Copy link
Contributor

CAD97 commented Aug 14, 2019

Just out of curiosity, is there a simple (ish) example of a case for external code to be matching enough cases for test-exhaustive matching to be used?

A visitor API is an obvious example, but I'd expect the visitor API to be provided by syn or whatever other crate is providing a nonexhsustive enum.

@BurntSushi
Copy link

Interesting. Presumably this means that the Nonexhaustive variant becomes a proper part of the public API? I'm not quite sure how I feel about that, but I can see how that would be a good thing.

I would also like to see use cases where it's critical/important for folks to do exhaustive matching on a non-exhaustive enum.

@dtolnay
Copy link
Owner Author

dtolnay commented Aug 14, 2019

For example a prettyprinter. I think there is room for a low-effort formatted printing library that just does a newline and indentation level on every open curly brace, with a small number of other heuristics. I would use such a thing here so we don't have to build all of rustfmt and don't have to rely on the system's rustfmt which is inconsistent across different developers' machines and sometimes missing from nightly.

@CAD97
Copy link
Contributor

CAD97 commented Aug 14, 2019

For the simple pretty printer example specifically, I'd expect it to work directly on TokenStream rather than on a syn AST.

That said, I could see the syntax version of #[non_exhaustive] (i.e. enum E { A, B, .. }) supporting this "warn about actual non exhaustion". With similar semantics to this: match e { A => {}, B => {}, .. } would warn on unmatched variants or if the enum is not nonexhaustive, and otherwise behave as _ => unimplemented!().

@dtolnay
Copy link
Owner Author

dtolnay commented Aug 15, 2019

I think the sweet spot for fast-compiling code generators is somewhere between what that TokenStream code does and what rustfmt does. For example things like the punctuation in closures really needs there to be a syntax tree for structure rather than just TokenStream.

let out = tokens.unwrap_or_else(|err| err.to_compile_error());
//                              ^^^^^

@Centril
Copy link

Centril commented Aug 15, 2019

FYI @Centril because language support for nonexhaustive would need to have at least these benefits before I would want to pick it up.

What #[non_exhaustive] on pub enum Foo { ... } (defined in crate A) does is to force crates depending on A to always have an arm with a pattern which will always match, e.g. _ => $expr or $ident => $expr.

This would let crate B have:

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

    #[cfg(test)]
    not_matched => unimplemented!("{:#?}", not_matched),

    #[cfg(not(test))]
    not_matched => { /* some sane fallback */ }
}

@dtolnay
Copy link
Owner Author

dtolnay commented Aug 15, 2019

@Centril that's definitely less good because downstream's tests will continue to pass even after we add a variant. The point is for them to notice that a variant has been added so that they can update their code, without breaking downstream's users in the interim.

@dtolnay
Copy link
Owner Author

dtolnay commented Aug 15, 2019

I had a chat with @Centril and here is my language based alternative proposal: rust-lang/rust#44109 (comment).

dtolnay added a commit that referenced this issue May 15, 2020
We'll fix this as tracked in #694.
@programmerjake
Copy link
Contributor

syn/src/expr.rs

Lines 247 to 251 in 436eb22

// Once `deny(reachable)` is available in rustc, Expr will be
// reimplemented as a non_exhaustive enum.
// https://github.com/rust-lang/rust/issues/44109#issuecomment-521781237
#[doc(hidden)]
__TestExhaustive(crate::private),

Maybe change the links in the code to instead point to rust-lang/rust#81657

@ModProg
Copy link
Contributor

ModProg commented Jan 26, 2022

Maybe change the links in the code to instead point to rust-lang/rust#81657

I think it would be rust-lang/rust#89554 as that is actually in work

Repository owner locked and limited conversation to collaborators Mar 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants