Emit #[non_exhaustive] for Rustified enums?

[auscompgeek]

It'd be good to have an option to emit #[non_exhaustive] on Rustified enums.

[emilio]

I agree, this would be a nice feature, and would be relatively straight-forward to implement.

I'd be happy to mentor it.

[highfive]

Hi! If you have any questions regarding this issue, feel free to make a comment here, or ask it in the #servo channel in IRC.

If you intend to work on this issue, then add @highfive: assign me to your comment, and I'll assign this to you. 😄

[agodnic]

I'm not sure what the interface for this should look like. So far I came up with two ideas:

1. Adding an extra bool parameter on fn rustified_enum: this looks compact, but introduces a breaking change in the API.
2. Adding a new function to Builder: Could be something like fn rustified_enum_non_exhaustive (which would mark the given enums as #[non_exhaustive]).

[emilio]

Something to potentially consider is whether it's likely people want some but not all enums to be non-exhaustive. But the initial version could just probably be an all-or-nothing switch.

(2) sounds nicer to me.

[auscompgeek]

Would it perhaps be better to flip the assumption and emit #[non_exhaustive] unless the user explicitly wants an exhaustive Rustified enum?

Obviously #[non_exhaustive] should not be emitted unless the Rust target is compatible (currently nightly).

[emilio]

Well, perhaps, but that'd be a breaking change, and note that rustified enum is already opt-in.

[agodnic]

Just made a PR for this, let me know if anything needs to be changed.

It looks like stabilization of non-exhaustive enums is gonna land soon, I guess the PR will stay un limbo until then.

[Lokathor]

Note that the #[non_exhaustive] attribute doesn't allow you to have an invalid bit pattern in an enum value.

[auscompgeek]

non_exhaustive is stabilised in 1.40 (currently beta).

[pheki]

I think that the documentation for rustified_enum is misleading: non_exaustive does not solve the issue rust-lang/rust#36927. Using an enum with a bit pattern with no corresponding variant is still UB. See comments 1 and 2, also in the nomicon.

I believe that the best way would be to return an option while converting the primitve to the enum type. Like the FromPrimitive trait, which can be derived using num-derive. Another option would be adding an Unknown variant, which would also need to be created manually when converting from the integer value.

[Lokathor]

Option and MyEnum::Unknown don't solve the fundamental problem that C can send a bit pattern to you that's not allowed for the enum and you're in UB land.

Using the "rustified enums" bindgen style is just extremely dangerous all the way through. I would 100% always tell people to avoid it.

[pheki]

@Lokathor If receiving directly the enum value, a struct with it or transmuting integers, I agree, but not if receiving the primitive and calling an function which matches the primitive and returns Unknown in the wildcard branch. Creating the value in rust and passing it to C is also fine.

#[derive(FromPrimitiveUnknown)]
enum Example {
    A = 0,
    B,
    C,
    #[from_primitive::unknown]
    Unknown = 0xFF,
}

// This would be auto generated by derive macro...
// Possibly this macro could instead implement From<u32> for Example
impl FromPrimitiveUnkown for Example {
    fn from_u32(n: u32) -> Example {
        use Example::*;
        match n {
            0 => A,
            1 => B,
            2 => C,
            _ => Unknown,
        }
    }
}

fn main() {
    let my_value = 1; // Received from C or network
    let my_value = Example::from_u32(my_value); // B
    let my_other_value = 20; // Received from C
    let my_other_value = Example::from_u32(my_other_value); // Unknown
    match my_value {
        ... // Exhaustive match!
    }
}

For example, I've got an use-case where the enum value is actually contained in an u8 inside a struct (I find this to be quite common, as C enums are always 32-bits...) and, although I'm converting from u8 to u32 and then matching the constified enum right now, deriving FromPrimitive or the imaginary FromPrimitiveUnkown would both be a lot nicer, since I'd then be able to match exhaustively on the enum (instead of non_exhaustively on the u32) and the compiler would catch any time I've missed a recently added value.

Simply deriving FromPrimitive would be enough, which could probably be supported if #1089 is implemented.

---

Going back on the documentation topic, maybe the docs for both rustified_enum and rustified_non_exhaustive_enum should be in the lines of: **Use this with caution**, if this is created in the FFI boundary with an invalid value, it will invoke undefined behaviour.

[Lokathor]

Yes, I agree with making it integers at the actual FFI bound and then converting into a rustified form with TryFrom or similar. That's what I've always done myself.

[auscompgeek]

There is also the newtype enum style I added a few months ago which could be used across an FFI safely. The only catch would be you lose the enum Debug derive.

[pheki]

@auscompgeek that's really nice!! I didn't know you could match on newtypes / transparent newtypes...

Can we please update the docs pointing to it though? I believe that non_exhaustive should ideally be deprecated removed from bindgen, as they only make things worse: it will look like it will handle invalid enum values but will invoke undefined behavior anyway, as the undefined behavior is invoked when creating an invalid enum, not matching it. See https://play.rust-lang.org/?version=stable&mode=release&edition=2018&gist=046392a181ffc03d1e120a72a7f32d18

I feel like the docs pointing to non_exhaustive are a real problem as people will likely believe that the actual solution is using #[non_exhaustive], possibly generating lots of undefined behavior in the ecosystem.

In my opinion, the docs for both rustified_enum and rustified_non_exhaustive_enum should read: **Use this with caution**, if this is created in the FFI boundary with an invalid value, it will invoke undefined behaviour. You probably want to use the newtype enum style instead of this one. I can create a PR if agreed.

[Lokathor]

non_exhaustive won't be deprecated in the language in general. It isn't meant for ffi at all, it's for API stability purposes.

If you meant "depricated in bindgen", it should be removed entirely really. It's essentially never ever correct to use in bindings.

[pheki]

Yes, I mean in bindgen, in rust its actually great for its intended purpose.

I thought to deprecate it as it wouldn't be a breaking change, hadn't realized that bindgen is still in 0.X... Also agree that removing is the best idea. 👍

Should I just open a PR? @emilio can you weigh in?

[emilio]

(thinking a bit out-loud, so sorry for the back and forth)

I can think of potential use cases for it, like binding against a library you control (so you know that there won't be out-of-range enum variants), but wanting the same forward-compat guarantees than #[non_exhaustive] gives to you.

But at that point you are equally served by the non-rustified enums... With one gotcha, which is the following: Does non_exhaustive allow to exhaustively match inside your crate? I guess it should. In that case, I think there's still one potential good use for #[non_exhaustive]. Whether that use-case is common enough or not to warrant support, I guess that's a call for @auscompgeek who proposed it or such.

In any case I agree it shouldn't be the recommended way, and that it should be properly explained that this doesn't prevent UB for out-of-range variants. I think for now I'd go with a note / warning / explanation on the docs (specially since the feature is not terribly hard to maintain).

[Lokathor]

non_exhaustive still allows for an exhaustive match within the crate that declared the type, yes.

[auscompgeek]

Yeah, my usecase is a third-party library that will likely add new variants to their enums in future. However now that we have the newtype enum style that will probably fit my usecase better.

Given we already have non_exhaustive implemented and it's a simple feature which others may find useful, I don't see a reason to remove it. I do agree better documentation warning about the implications would be best though (and maybe a pointer to the newtype style which might fit people's usecase better).

[pheki]

Now that the docs were fixed, can this issue be closed? #[non_exhaustive] is already being optionally emitted for rustified enums...

[emilio]

Yes, let's close this, thanks all!