Avoid branch in `Display` `match` statement for empty enums

[tvercruyssen]

trafficstars

This removes a match arm that will never be triggered for non empty enums. So now only if their are no arms in the enum will the code for empty enums be generated.

This changes no functionally and is a bug fix.

[JelteF]

Can you add regression test for the case that this fix is solving

[tvercruyssen]

While trying to work out a test case to proof the match arm was unreachable if the enum was non empty. This was needed as proc-macro-not-showing-unreachable-match-arm makes it so you can't rely on the compiler to catch this.

I came accros the thread that in the end concludes:

It's also already undefined behaviour to create an enum not specified by a variant

reference issue: https://github.com/rust-lang/rust/issues/4499#issuecomment-39196072

So you simply cannot construct an empty enum and as such I think I should maybe change this PR to remove to code to handle Display for them. It makes no sense to have a Display derive on an enum that cannot be constructed and as such never be printed.

The only test case of this just checks that you can derive Display on an empty enum here: EmptyEnum.

If we remove the _ => Ok(()) this will no longer compile. But as an empty enum has no use as far as I can see I see no problem with this. This code is causing unneeded an cause unreachable match arms for all non empty enums that derive Display.

So in conclusion I think this PR should be renamed and instead remove that code to no longer handle empty enums that cannot be created (unless using UB).

If you do wish to keep this code that serves (at least in my opion) then I would at least take this improvment in the code as this causes the code to still compile and no emit that unreachable match arm in non empty enum cases.

Sadly this is impossible to test as far as I can see unless we expand the macro before calling stringify and extract match all match arms from that but that is impossible see this question that you cannot control expansion order of macros.

TL;DR this code is makes it so a case that should never occur no longer causes other happening cases to generate more code that needed. As in generating an unreachable match arm.

[JelteF]

Thanks for the detailed comment. Removing the empty enum support sounds like the best approach indeed then, please update the PR to do so. I'm preparing a 1.0.0 release anyway, so if people rely on this at least its support will be removed in a major release.

[tvercruyssen]

Just for refrence this is what a crate like thiserror does in case of an empty enum

#![feature(prelude_import)]
#[prelude_import]
use std::prelude::rust_2021::*;
#[macro_use]
extern crate std;
enum E {}
#[allow(unused_qualifications)]
impl std::error::Error for E {}
#[allow(unused_qualifications)]
impl std::fmt::Display for E {
    fn fmt(&self, __formatter: &mut std::fmt::Formatter) -> std::fmt::Result {
        #[allow(unused_variables, deprecated, clippy::used_underscore_binding)]
        match *self {}
    }
}
#[automatically_derived]
impl ::core::fmt::Debug for E {
    fn fmt(&self, f: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
        unsafe { ::core::intrinsics::unreachable() }
    }
}

source code being:

#[derive(thiserror::Error, Debug)]
enum E {}

Do you think we can change the

    Ok(quote! {
        impl #impl_generics #trait_path for #name #ty_generics #where_clause
        {
            #[allow(unused_variables)]
            #[inline]
            fn fmt(&self, _derive_more_display_formatter: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
                #helper_struct

                match self {
                    #arms
                }
            }
        }
    })

match self => match *self, this is what thiserror does and will make empty enums compile the reason for this can be seen in the error you get in the notes references are always considered inhabited . So when you match on *self instead the compiler looks at the enum instead to see if their are any variants considering their are none an empty match is valid code.

I'll make this chances that I've described here. If you think the reasoning here is unclear or their are better solutions I'll be happy to adjust the PR more.

Should I also change the title now the PR purpose is changed is commit messages okay (if you squash merge the PR title won't be seen in the history -- I think ?)

[tvercruyssen]

Changing self => *self will have implications for the match arms and inner arguments not being references anymore which will create problems.

So I see 2 solutions we can use here: 1.

    let body = if arms.is_empty() {
        quote! {
            #helper_struct

            match self {
                #arms
            }
        }
    } else {
        quote! { Ok(()) }
    };

    Ok(quote! {
        impl #impl_generics #trait_path for #name #ty_generics #where_clause
        {
            #[allow(unused_variables)]
            #[inline]
            fn fmt(&self, _derive_more_display_formatter: &mut ::core::fmt::Formatter) -> ::core::fmt::Result {
                #body
            }
        }
    })

2. we don't implement Display at all. It has no usage so maybe that is the better solution than an empty one ?

[tvercruyssen]

I implemented option 2. If you would rather have option 1 I'll be happy to change to it. The code that will be used for option 1 is above.

I have added the impls crate to write an easy test for the Display trait. This dependency is optional and is only enabled when testing the display feature.

Lastly I've formatted Cargo.toml where their where a few "beauty faults" that I assume you would like fixed. If not I will revert them.

[tvercruyssen]

If you don't want the impls dependency than I can impl it using traits myself. But it's clearer and better coded using impls and considering it only gets compiled for that test and is not a big crate. I think it's worth adding it.

[tyranron]

@tvercruyssen

Changing self => *self will have implications for the match arms and inner arguments not being references anymore which will create problems.

Why?

match self {
    Self::Some(val) => ...
}

just becomes

match *self {
    Self::Some(ref val) => ...
}

This shouldn't be a burden.

The code like Ok(()) is misleading for the compiler, imho. By receiving match *self {} the type checker explicitly understands that it won't be executed ever, which is good.

It makes no sense to have a Display derive on an enum that cannot be constructed and as such never be printed.

It still may have sense for trait bounds in generics contexts where PhantomData is involved, where you don't create values, but proving something on type-level. This is what empty enums are often used for.

[tvercruyssen]

@tvercruyssen

Changing self => *self will have implications for the match arms and inner arguments not being references anymore which will create problems.

Why?

match self {
    Self::Some(val) => ...
}

just becomes

match *self {
    Self::Some(ref val) => ...
}

This shouldn't be a burden.

The code like Ok(()) is misleading for the compiler, imho. By receiving match *self {} the type checker explicitly understands that it won't be executed ever, which is good.

It makes no sense to have a Display derive on an enum that cannot be constructed and as such never be printed.

It still may have sense for trait bounds in generics contexts where PhantomData is involved, where you don't create values, but proving something on type-level. This is what empty enums are often used for.

I agree and changed the PR accordingly. Now we use match *self and ref for fields. I've added a test that checks that empty enums also implement display now. The check is at compile time so you don't get nice test failures. But this avoids the dependency I added above and makes the test relatively straight forward by using supertraits.

[tyranron]

@tvercruyssen for the nightly backtrace errors we should wait #195 being resolved. This will happen soon.

@JelteF additionally, could you move Required from "Test Rust 1.36.0 on ..." to "Test Rust 1.56.0 on ..." CI jobs? Thanks!

[tvercruyssen]

I've changed the title of the PR to reflect what the current changes will do.