Start emitting unions in stable mode

[fitzgen]

They're in stable now: https://blog.rust-lang.org/2017/07/20/Rust-1.19.html

Basically, we can go through all of the places where we check ctx.options().unstable_rust and use that to decide whether to use unions or not, and just always use the unions. We can also get rid of the BindgenUnion type now.

And finally, we can update the user guide's section on using unions.

I'm happy to mentor anyone who wants to pick this up.

[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. 😄

[tmfink]

Currently, we have a bool indicating stable vs. unstable.
Instead, should we have an enum that indicates the "flavor"/"epoch"/"channel" to support?

Bindgen code:

#[allow(non_camel_case_types)]
#[derive(Debug)]
enum RustVersion {
    Stable_1_0,
    Stable_1_15,
    Stable_1_20,
    Nightly,
}

const LATEST_STABLE: RustVersion = RustVersion::Stable_1_20;

Use case:

let bindings = bindgen::Builder::default()
        .rust_version(RustSupport::Stable_1_15)  // Specify Rust version here
        .header("wrapper.h")
        .generate()
        .expect("Unable to generate bindings");

This would allow users to upgrade bindgen versions without getting breaking changes in the generated Rust code (like using unions unless nightly or >= Stable 1.19).

This may be more trouble than it's worth. Thoughts?

[tmfink]

@highfive: assign me

[highfive]

Hey @tmfink! Thanks for your interest in working on this issue. It's now assigned to you!

[fitzgen]

Instead, should we have an enum that indicates the "flavor"/"epoch"/"channel" to support?

I think this is a good idea.

That said, I'd nitpick the enum to be something like:

pub enum TargetRust {
    // As far as available features goes, no difference between stable 1.X and
    // beta 1.X from our perspective...
    //
    // (major, minor)
    StableAndBeta(u8, u8),

    // Everything else...
    Nightly
}

Although ideally we would be able to turn individual nightly features on and off too. I suppose we could make the nightly variant be a struct variant with a bool for each feature?

[fitzgen]

And thanks for picking this up @tmfink ! Let me know if you have any questions :)

[tmfink]

No problem @fitzgen!

I would prefer that TargetRust has discrete values for each Stable/Beta like I described in my previous comment. That way:

• Code will not compile if a nonsensical variant like StableAndBeta(255, 255) were used because the equivalent variant would not be defined.
• I think that users would find it easier to select a TargetRust variant if an exhaustive list were defined. There would not be as many possible variants; we only need a new TargetRust variant when bindgen might use a new feature. For example, I don't think bindgen ever output code that used try!, so adding ? would not have required us to add a new target variant.

As for implementation, I think the interface should accept a RustFeatures struct that contains a bool for each feature. We can make the internal fields private and expose a public builder pattern constructor. We can impl From<TargetRust> for RustFeatures to make the conversions.

I think that TargetRust::Nightly should give a RustFeatures with all features enabled. If users want more control over which features to use in nightly, they can just manually build a RustFeatures struct with the builder pattern.

Another small detail: if we go with StableAndBeta(major, minor), lets use u32 to be safe--the overhead would not really matter.

[emilio]

Note that emitting unions have impact in stuff like whether a type can derive Debug or not, which is somewhat unfortunate...

I'd personally like to keep unions not the default, but I guess I can bite the bullet if needed.

[tmfink]

@emilio I feel like the language support of union is worth not being able to derive Debug. Using bindgen's stable union types is a bit awkward. Over time, we may get more support (lints, etc.) that make union easier to use.

[emilio]

Yeah, I guess that's fair.

[fitzgen]

Regarding feature flags and rust target versions: I think we really want to keep this as simple as possible to avoid the explosion of potential bugs from every combination of features. I agree we want to have a notion of the target rust version/channel and its supported features, but we should just do the minimum that achieves that IMO.

Regarding union making things not derive(Debug): an alleviation is to emit our own Debug impls when they can't be derived. Even something as simple as

impl ::std::fmt::Debug for Elephant {
    fn fmt(&self, f: &mut ::std::fmt::Formatter) -> ::std::fmt::Result {
        write!(f, "Elephant {{ ... }}")
    }
}

is likely good enough.

[fitzgen]

Fixed in #859

[tmfink]

We still need to update the book chapter on unions.

[tmfink]

@fitzgen I am working on the Using the Union Types Generated by Bindgen chapter of the user's guide.

[fitzgen]

@fitzgen I working on the Using the Union Types Generated by Bindgen chapter of the user's guide.

Awesome, thanks! 👍