cfg features for enum variants

[jeff-k]

This is a fix for #4411. #[cfg] attributes are propagated to the generated python classes as described in this comment: #4411 (comment)

It only covers the case for simple enums but a similar solution could be extended for complex enums.

[mejrs]

Thanks!

I like the implementation, I just have a couple of points that need addressing:

1. This PR uses attrs as variable and field names. Existing code uses cfg_attrs. This code should too, for consistency.
2. Please add a test to ensure that something like

#[pyclass]
enum Empty{
    #[cfg(any())]
    Disabled,
}

does not compile. You can add it to tests/ui/invalid_pyclass_enum.rs

[jeff-k]

So in the case where all of the variants get disabled, like

#[pyclass]
enum Empty{
    #[cfg(any())]
    Disabled,
}

it does fail to compile, but not with the nice error: #[pyclass] can't be used on enums without any variants message.

Instead we get:

error[E0004]: non-exhaustive patterns: type `&Empty` is non-empty
  --> src/lib.rs:27:1
   |
27 | #[pyclass]
   | ^^^^^^^^^^
   |
note: `Empty` defined here
  --> src/lib.rs:28:6
   |
28 | enum Empty {
   |      ^^^^^
   = note: the matched value is of type `&Empty`
   = note: references are always considered inhabited
   = note: this error originates in the attribute macro `pyclass` (in Nightly builds, run with -Z macro-backtrace for more info)
help: ensure that all possible cases are being handled by adding a match arm with a wildcard pattern as shown
   |
27 ~ #[pyclass] {
28 +     _ => todo!(),
29 + }
   |

For more information about this error, try `rustc --explain E0004`.

Which I suppose is related to the reason why we have the first error message but I don't think we can know at this time which features are enabled and can't test for empty enums from disabled variants.

The error message we do get is because the 2 generated match arms in impl_simple_enum wind up empty. For fun I added a wildcard arm:

            fn __pyo3__repr__(&self) -> &'static str {
                match self {
                    #(#variants_repr)*
                    _ => unreachable!(),
                }
            }

and the generated code does compile but I don't think that could lead to a helpful way to report an invalid enum. The resulting python module silently does not have the empty enum class.

I'm not sure what to do. I could see a user defining an enum where every variant is covered by a cfg attribute and triggering this. It will still fail to compile but for the wrong reason so I'm not sure how critical this is.

I suppose we could work out the annihilating combination of variant cfg attributes and if they're met trigger a compile error?

For example, with an enum like

enum MyEnum {
    #[cfg(feature = "x")]
    VariantOne,
    #[cfg(feature = "y")]
    VariantTwo,
}

collecting the cfg features and generating this:

#[cfg(all(not(feature = "x"), not(feature = "y")))]
compile_error!("All variants of enum MyEnum have been disabled by cfg attributes");

Or is this just an edge case.

[mejrs]

The error message we do get is because the 2 generated match arms in impl_simple_enum wind up empty. For fun I added a wildcard arm:

            fn __pyo3__repr__(&self) -> &'static str {
                match self {
                    #(#variants_repr)*
                    _ => unreachable!(),
                }
            }

and the generated code does compile but I don't think that could lead to a helpful way to report an invalid enum.

In this case, just dereference the scrutinee - maybe we should do this to avoid emitting this particular error

            fn __pyo3__repr__(&self) -> &'static str {
                match *self {
                    #(#variants_repr)*
                }
            }

Anyway, we should not rely on E0004. The rules around references to uninhabited types are still being worked out and such code may start compiling in the future.

I suppose we could work out the annihilating combination of variant cfg attributes and if they're met trigger a compile error?

Yes, I think this is a good idea.

Or is this just an edge case.

I can believe that someone would hit this eventually and waste a lot of time debugging ;)

Maybe we could reconsider allowing empty enums. I wasn't involved with that feature so I'm not sure there is a compelling reason to disallow it.

[jeff-k]

I've pushed a draft solution that constructs a #variant_cfg_check token stream that's inserted at the top of the generated class code.

If there is at least one variant that is not annotated with a cfg attribute or if there are zero variants then this token stream is quote! {}. Otherwise:

    quote! {
        #[cfg(all(#(#conditions),*))]
        ::core::compile_error!(concat!("All variants of enum `", stringify!(#cls), "` have been disabled by cfg attributes"));
    }

Here's an example:

#[pyclass(eq, eq_int)]
#[derive(PartialEq)]
enum MyEnum {
    #[cfg(not(any(feature = "x", feature="y")))]
    VariantOne,
    #[cfg(all(feature = "y", feature="optional_feature"))]
    VariantTwo,
}

$ cargo build --features y
...
error: All variants of enum `MyEnum` have been disabled by cfg attributes
  --> src/lib.rs:32:1
   |
32 | #[pyclass(eq, eq_int)]
   | ^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: this error originates in the attribute macro `pyclass` (in Nightly builds, run with -Z macro-backtrace for more info)

I've also dereferenced the scrutinees so that the body of the generated class is syntactically valid and the E0004 doesn't also occur. I haven't investigated why I don't see an unreachable code warning but I think in theory we are liable to get one, as happens in this example:

warning: unreachable statement
  --> src/lib.rs:68:5
   |
67 |     let _y = match *uninhabited { };
   |              ---------------------- any code following this expression is unreachable
68 |     println!("unreachable");
   |     ^^^^^^^^^^^^^^^^^^^^^^^ unreachable statement
   |
   = note: `#[warn(unreachable_code)]` on by default
   = note: this warning originates in the macro `println` (in Nightly builds, run with -Z macro-backtrace for more info)

I don't have any input on what adding unconstructable types to python bindings would look like. I think python's typing.Never was added in 3.11 so I suppose there's at least a notion of it.

Is this style consistent with pyo3?

I will continue with making tests

[mejrs]

Suggested change

	return quote! {};
	// There's at least one variant of the enum without cfg attributes,
	// so the check is not necessary
	return quote! {};

[mejrs]

We should probably mention why it's not allowed:

Suggested change

	error: All variants of enum `AllEnumVariantsDisabled` have been disabled by cfg attributes
	error: #[pyclass] can't be used on enums without any variants - all variants of enum `AllEnumVariantsDisabled` have been configured out by cfg attributes

[jeff-k]

Ok, good. I see we don't put #[pyclass] in backticks elsewhere in this module's compiler errors, too, so I haven't added them.

[mejrs]

It would be nice to have a better span for this error, cls.span() should do.

[mejrs]

Thanks!