Add option to generate safe and unsafe conversions for rustified enums by jbaublitz · Pull Request #2908 · rust-lang/rust-bindgen (original) (raw)

(My previous comment was intended to reply to your "two functions", rather than the --use-enum-ctypes-on-fns one; our messages crossed :)

However, if you just use the Plain_ctype everywhere, that's not cumbersome anymore

Exactly, that is why I think using the Rust enum for parameters may not be great, unless the expectation is that users avoid it in some cases. So they would probably still not use it as their "default" or "catch all" case, i.e. --... *.

Yeah I think that the extra convenience of using the rustified enum would only work if you want to expose the enum itself. And if you want to expose safe bindings for functions that use the c enum, then you have to do those yourself accordingly to the invariants of the library itself.

However, I could see a case where you'd want to offer both the rustified enum and safe wrappers to those functions.

Hmm... What do you mean by safe wrappers? i.e. the raw functions would still need to be marked unsafe, even if we took Rust enums, so the user would still need to wrap it.

That's true, even if the function takes the rustified enum then you still have to figure out its safety requirements and expose it accordingly. So you'd still have to do things like

mod bindings { unsafe extern "C" { pub fn takes_plain(plain: Plain); } }

/// Safety: It is safe, believe me. pub fn takes_plain(plain: Plain) { unsafe { bindings::takes_plain(plain) } }

So, having to add an extra plain.to_raw_ctype() or whatever to obtain the C value seems like little effort. Maybe @emilio or @kulp have a better idea on why bindings of functions that take C enums become functions that take Rustified enums instead of using the integer type directly as they have been around for longer.

Having said that, I would like to eventually have a [[safe]] attribute on the C side that allows us to do things like that -- for the kernel for instance, we are now at the point were starting to add annotations on the C side for several things may be OK for some subsystems at least. Similarly, we could have a [[enum ...]] thing that gives bindgen this information without having to maintain the lists outside the source code. I don't know if nowadays libclang preserves unknown attributes -- if so, we could already do it without having to modify and wait for Clang.

That sounds like a sensible addition. In the meantime, it is already possible to embed html-like tags in the comments of the headers and bindgen can read those to introduce custom behavior.

The other extreme is to have an "easy-to-use" mode, where by default all is provided: the raw C type and its constants, plus a Rustified enum type, plus a non-exhaustive one, plus the conversion functions (fallible, infallible and unsafe) to convert to both of those... Then users can use whatever they think it is best for each enum, without having to know about the bindgen kinds, and it is sound. Then when they are sure how they use each enum, they can restrict it to one kind via the arguments (or via attributes if we ever get there) so that they avoid bindgen generating code that will not be needed. So it would be easy to start using, and allow people to customize later. But it would be more "expensive" by default.

Yeah ideally you should be able to decide if a C enum should be translated in different ways without interference between each representation. I think the API would need some bike-shedding, specially from the CLI side where there isn't much flexibility. Maybe the idea of having a bindgen.toml file where we can store more structured information would be useful here as you could do something like

[[enum_style]] regex = "Foo" rustified = { generate = true, try_from_raw = true, from_raw_unchecked = true } constified = true

Then anything that matches the Foo regex would get all of those immediately. Maybe a CLI equivalent could be --enum-style "Foo=rustified(try_from_raw,from_raw_unchecked),constified"

It would also be nice to be able to replace the multitude of --newtype-enum --newtype-global-enum, --constified-enum, etc, by a single option that can be extended. This would also give @jbaublitz some flexibility as it wouldn't be necessary to include the raw type constants with this feature as those would be enabled automatically if you pass the extra constified style.