Default to generating constified enums, rather than generating Rust enums

[fitzgen]

Right now, we translate C/C++ enums into Rust enums by default. This is problematic because it is OK for C/C++ code to return some int that isn't one of the enum variants, and that is well defined for C/C++; however, that is UB in Rust and can lead to miscompilations. People shouldn't be using Rust enums unless they have complete control of the C/C++ code (ie not using a shared library and have audited the C/C++ code themselves). Therefore, it definitely shouldn't be the default!

The changes involved are to

• replace the Builder::constified_enum method with a Builder::rustified_enum method in src/lib.rs

• replace the --constified-enum flag with a --rustified-enum flag in src/options.rs

• update the way we determine if an enum is constified or a rust enum in src/codegen/mod.rs. Do a git grep constified src/codegen to see the relevant code paths.

• For all the test header files in tests/headers/* that contain enums and do not have a --constified-enum flag in the // bindgen-flags: comment, add a --rustified-enum .* to the // bindgen-flags: comment

• Remove --constified-enum whatever flags from all test headers in the tests/headers/* files

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

[jprusakova]

@highfive I am working on this issue.

[fitzgen]

Hi @jprusakova -- still hacking on this? Anything I can do to help out?

[derekdreery]

I'm slightly concerned about using const enums, because their size is not well defined.

(From the spec for C11)

Each enumerated type shall be compatible with char, a signed integer type, or an unsigned integer type. The choice of type is implementation-defined, but shall be capable of representing the values of all the members of the enumeration.

This means that you can't assume a particular size of an enum

[fitzgen]

@derekdreery, We still have this complication when generating Rust enums (the enum has to have some kind of physical size, which we choose via repr), but we additionally have the footgun where the C function may return a value that isn't defined as a variant of the enum, which is UB on the Rust side. That is, by changing defaults from compiling C enums into Rust enums to compiling them to a set of const values, we now have one footgun instead of two.

As far as choosing a size goes, regardless which Rust code we're emitting, we rely on libclang to tell us what it has chosen. If clang and gcc or msvc disagree on the C enum's representation, there isn't much that bindgen can do automatically. Making the correct decision in this situation requires knowledge that bindgen doesn't have, only the user does. Therefore, it is up to the user to use replacement types, mark the enum as opaque, or blacklist the enum and use --raw-line/Builder::raw_line to insert a replacement.

[derekdreery]

Doesn't #[repr(C)] match C behaviour on the target platform (choosing the correct size depending on the number of variants. I always assumed that rust took care of this if you used repr(C).)?

[fitzgen]

I was under the impression that repr(C) on an enum meant that it was just a discriminant, and didn't imply anything about size, which was up to repr(u8) etc. After reading a bit more, it isn't clear to me if this is supposed to be guaranteed or not.

rust-lang/rust#28925 seems relevant, where at least some folks are expecting the behavior you're describing, but it doesn't seem to always be correct.

[derekdreery]

Yeah, that discussion is illuminating. You cannot possibly encode every algorithm in every compiler. But it also means that C libraries are not necessarily ABI compatible with each other. I guess the lesson is "don't use enums in C", and we in rust have to do the best we can (by using large types and hoping for the best :/). In that case constants are definitely better than enums - apart from anything else they are simpler.

EDIT: don't expose enums.

[emilio]

Well, libraries use to be compatible. Some of them have hacks to force enum size:

https://github.com/assimp/assimp/blob/1b4cbcc6adbfc13b8766c92e2de4cf8a18c31755/include/assimp/metadata.h#L71

[emilio]

But yeah, I agree that the situation is quite bad, you can compile with some compiler options to force this layout optimizations and what not, breaking all sort of stuff if it's not compiled the same way.

[Cldfire]

@highfive: assign me

(I started on this, have made progress, and think I will be able to finish it, hopefully today?).

[highfive]

It looks like this has already been assigned to someone. I'll leave the decision to a core contributor.

[fitzgen]

@Cldfire sounds good! This bug has been quiet, so I don't think @jprusakova is still working on it.

[Cldfire]

Status update: I have all tests passing locally and have pushed to my fork (https://github.com/Cldfire/rust-bindgen/commit/ca206562a2d9e891721a0d8e932f114393d596c0).

I still have a few things I want to think about before I make a PR, though, and I'm done working on it for tonight. I'll get back to it tomorrow night and / or Sunday.