Add option to generate safe and unsafe conversions for rustified enums #2908

jbaublitz · 2024-08-20T16:29:55Z

jbaublitz · 2024-08-20T21:26:50Z

@tgross35 @ojeda I could use some guidance with some of the behavior you would like to see in corner cases for this PR. Specifically I'm interested in the cases where constants that get generated even in the rustified case conflict with the constants @tgross35 requested in the issue. I have to investigate further, but I think I encountered a case with anonymous enums where this could theoretically happen. How do we want to address this? Another prefix for the constants? Something else?

ojeda · 2024-08-21T15:03:46Z

Another prefix for the constants?

I guess that could be a possibility, bindgen already has things like --anon-fields-prefix. If you find the example/case, that would be great to discuss what could be done.

bindgen-tests/tests/expectations/tests/issue-2646.rs

ojeda · 2024-08-21T15:30:12Z

bindgen-cli/options.rs

-    #[arg(long, value_name = "REGEX")]
-    rustified_non_exhaustive_enum: Vec<String>,
+    /// Mark any enum whose name matches the provided regex as a Rust enum. This parameter takes
+    /// options in the shape REGEX or REGEX=OPTIONS where OPTIONS can be a comma separated list of


So this disallows a regex without options but with a = character, right? I guess we don't need that, but just noting it.

Yes, that's correct. If you provide no equal sign, it uses the empty option set.

ojeda · 2024-08-21T15:32:23Z

bindgen-tests/tests/expectations/tests/issue-1554.rs

+pub type Planet_ctype = ::std::os::raw::c_uint;
+pub const Planet_earth: Planet_ctype = 0;
+pub const Planet_mars: Planet_ctype = 1;


So the PR is generating the constants for (some of) the existing Rust enum styles -- not sure if that is what others were expecting or not.

Oh, so are you saying that a non exhaustive rustified enum shouldn't generate constants? I'm happy to only do it if either try_from_raw or from_raw_unchecked is enabled.

jbaublitz · 2024-08-21T16:15:49Z

Another prefix for the constants?

I guess that could be a possibility, bindgen already has things like --anon-fields-prefix. If you find the example/case, that would be great to discuss what could be done.

So I think the only CI failure remaining is the case where the generated constant for rustified enums conflicts with an existing constant already generated by bindgen in previous versions. This may be resolved by your PR revision comment of only generating constants in cases where either try_from_raw or from_raw_unchecked is enabled. Would you prefer I take that approach?

ojeda · 2024-08-21T16:23:39Z

It is up to the maintainers (I don't really know what they would prefer or their policies! :) -- I added that comment mainly because I was surprised by all the tests changing, i.e. I thought we were adding a new feature/mode/... that wouldn't necessarily change existing output/users.

In any case, if we are hitting a case here when the constants are added, does it mean that, if you enable try_from_raw/from_raw_unchecked, we would still hit that, right? (i.e. if we add the failing test with one of those enabled). In that case, I imagine it may be a good idea to give users a way to workaround it somehow, even if there is currently no test for that that happens to fail. Which is the test that fails?

jbaublitz · 2024-08-21T16:30:13Z

The test that is consistently failing is the constify-enum.h header. My understanding of what is happening is that Rustified enums generate constants for each variant of the enum with the type of the rustified enum. The problem seems to appear in the case where basically we generate these constants the same way in both the code I added and the code that provides these variant constants ([ENUM_TYPE]_[VARIANT_NAME]). This could easily be avoided by adding a _ctype suffix to the constants that I generate, but that doesn't really answer the question of whether we want to be generating these constants for any case other than try_from_raw and from_raw_unchecked. However, I see your point that even if we only do it in the case of our new options, that could still cause conflicts in certain places. I'm happy to do either.

pvdrz · 2024-09-24T13:34:38Z

@jbaublitz I think I'm still leaning towards the option of putting those constants as associated constants under the enum's impl block. That would keep them in a namespace that has a lower chance of collisions.

jbaublitz · 2024-10-23T19:36:16Z

@pvdrz I'll give it a shot. Part of the problem there is just a naming problem. For example, in most cases the variant of the enum will be named the same thing as the constant without some sort of suffix or prefix. Because they're constants it may just be sufficient to rely on the case being different if we convert all of the constants to upper case in keeping with the Rust const case convention. However, if users decide to define an enum variant as upper case, I forsee some conflicts happening. I'll try running my initial try through CI and I'll see what fails.

ojeda · 2024-11-18T14:50:51Z

What is the type generated in functions receiving/returning each kind of enum in the examples? I am asking since the idea was to avoid UB (so we need the wider type) but have a way to transform that into the Rust enums generated here.

There is a bindgen/codegen/.mod.rs.swp file in the PR, by the way (but it does not allow me to add a comment to it).

ojeda · 2024-11-18T14:51:35Z

bindgen-tests/tests/headers/issue-2646.h

+    Both2 = -1,
+    Both3,
+};
+


It could be a good idea to add a few functions here, e.g. enum Plain f(enum Plain); and so on, to double-check the generated type in expectations.

Oof, it appears after adding some tests that it is converting it to the type that will result in undefined behavior. I'll have to take a look at how to avoid that. I think the code to handle that is in a place I haven't touched, so I'll have to track down where that's happening.

That is what I feared, yeah. Thanks!

So is what you're recommending only doing this in the case of TryFrom generation like with the constants?

I am not sure what you mean -- do you have an example?

Maybe, one alternative to roll this change slowly would be to add another flag like --use-enum-ctypes-on-fns or --return-enum-ctypes (name is a WIP 🤣) that when enabled, makes all functions return the ctype instead of the rustified enum. We could keep it disabled by default and eventually enable it by default as unsoundness is a pretty good basis for it.

@jbaublitz @ojeda what do you think?

So I've been thinking something: Let's say we have two functions (...)

How would a user call takes_plain with a value that is not in the Rust-side enum? i.e. a C function may need to be used with more values too. Of course, perhaps the bindgen user should avoid this kind of enum in this case.

How would a user call takes_plain with a value that is not in the Rust-side enum? i.e. a C function may need to be use with more values too.

I think the only case where a Rust user would call takes_plain with a value outside the rustified enum would be if there is a common pattern in the C library like:

// This value could be outside of `Plain` enum Plain value = returns_plain(); // But that's OK because this function already assumes that this could happen // and preemptively checks that internally takes_plain(value);

So doing that from the rust side would be weird as you'd have:

unsafe extern "C" { fn returns_plain() -> Plain_ctype; fn takes_plain(plain: Plain); } // This value could be outside of `Plain` let value: Plain_ctype = unsafe { returns_plain() }; // But here, we have to do an unsafe transmute that might cause UB // even though the call to the C function would be fine. unsafe { takes_plain(std::mem::transmute(value)) };

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

unsafe extern "C" { fn returns_plain() -> Plain_ctype; fn takes_plain(plain: Plain_ctype); } // This value could be outside of `Plain` let value: Plain_ctype = unsafe { returns_plain() }; // No weird transmute unsafe { takes_plain(value) };

Of course, perhaps the bindgen user should avoid this kind of enum in this case.

Yes maybe in that case you'd be better not using --rustified-enum at all. However, I could see a case where you'd want to offer both the rustified enum and safe wrappers to those functions.

(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. --... *.

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.

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.

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.

(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.

pvdrz · 2024-12-06T15:34:33Z

bindgen-tests/tests/expectations/tests/issue-2646.rs

+    }
+}
+impl FromRawUnchecked {
+    pub type FromRawUnchecked_ctype = ::std::os::raw::c_uint;


@jbaublitz maybe this type should just be ctype as it is already inside the impl block

I just read that having type on non-trait impl blocks is unstable: rust-lang/rust#8995

So sadly we have to keep this outside the impl block without changing the name.

pvdrz · 2024-12-06T15:35:01Z

bindgen-tests/tests/expectations/tests/issue-2646.rs

+}
+impl FromRawUnchecked {
+    pub type FromRawUnchecked_ctype = ::std::os::raw::c_uint;
+    pub const FRU1: FromRawUnchecked_ctype = 6;


~~and then here the type could be just FromRawUnchecked::ctype or even Self::ctype.~~

Edit: ignore (see #2908 (comment))

it might make sense to add a suffix to the names of these constants (like _raw) to avoid misuses

pvdrz · 2024-12-06T19:49:49Z

@jbaublitz @ojeda the discussion we've had so far here makes me think that we need to figure out a better way to integrate this feature as it has revealed some problems we have with the current enum representations we handle in bindgen.

I'm going to draft a proposal on how to rework the enum-style API bindgen has to address some of the shortcomings we've found here and if that proposal makes it into main then we can push this PR.

Thanks for all your effort here, I've really enjoyed the discussion and I hope we can get this shipped soon.

jbaublitz · 2024-12-07T17:26:26Z

@jbaublitz @ojeda the discussion we've had so far here makes me think that we need to figure out a better way to integrate this feature as it has revealed some problems we have with the current enum representations we handle in bindgen.

I'm going to draft a proposal on how to rework the enum-style API bindgen has to address some of the shortcomings we've found here and if that proposal makes it into main then we can push this PR.

Thanks for all your effort here, I've really enjoyed the discussion and I hope we can get this shipped soon.

Okay sounds good! Do you want me to address the concerns you brought up while I wait?

pvdrz · 2024-12-09T16:54:16Z

@jbaublitz @ojeda the discussion we've had so far here makes me think that we need to figure out a better way to integrate this feature as it has revealed some problems we have with the current enum representations we handle in bindgen.
I'm going to draft a proposal on how to rework the enum-style API bindgen has to address some of the shortcomings we've found here and if that proposal makes it into main then we can push this PR.
Thanks for all your effort here, I've really enjoyed the discussion and I hope we can get this shipped soon.

Okay sounds good! Do you want me to address the concerns you brought up while I wait?

So, for now I think the safest would be to just remove the raw ctype constants from this implementation. I think you can still access them by doing the respective Enum::variant as ctype anyway. We will introduce them in the future by allowing users to pick more than one enum representation. In that particular case, it would be constified_enum + rustified_enum.

Regarding the function signatures, I wouldn't touch that on this PR either. We can introduce that change later.

jbaublitz · 2024-12-16T22:23:24Z

Just a note that I've spent today trying to track down this failure, but I've been unable to reproduce it on my version of libclang. Tomorrow I'm going to try to reproduce on the version of libclang in CI.

jbaublitz · 2024-12-17T15:43:25Z

Okay @pvdrz, it's passing CI and I think I've made all of the requested changes. Any additional feedback or is this good to go?

jbaublitz force-pushed the issue-2646 branch from fdd49ef to 3768644 Compare August 20, 2024 16:30

jbaublitz marked this pull request as draft August 20, 2024 16:33

ojeda mentioned this pull request Aug 20, 2024

bindgen wanted features & bugfixes Rust-for-Linux/linux#353

Open

27 tasks

ojeda added the rust-for-linux Issues relevant to the Rust for Linux project label Aug 20, 2024

jbaublitz force-pushed the issue-2646 branch 2 times, most recently from efd0708 to e2b79ac Compare August 21, 2024 13:56

ojeda reviewed Aug 21, 2024

View reviewed changes

jbaublitz force-pushed the issue-2646 branch 7 times, most recently from 8c41d55 to 52851c7 Compare August 21, 2024 16:11

jbaublitz force-pushed the issue-2646 branch 3 times, most recently from 390527d to 699f11c Compare August 21, 2024 16:21

jbaublitz force-pushed the issue-2646 branch from 699f11c to 797ac8b Compare September 3, 2024 15:31

jbaublitz force-pushed the issue-2646 branch from 797ac8b to bd3a560 Compare October 23, 2024 19:35

ojeda reviewed Nov 18, 2024

View reviewed changes

ojeda mentioned this pull request Nov 18, 2024

Add Result<(), error_enum> enum binding #2980

Open

jbaublitz force-pushed the issue-2646 branch from bd3a560 to a7c5097 Compare December 5, 2024 18:48

pvdrz requested changes Dec 6, 2024

View reviewed changes

This was referenced Dec 10, 2024

[RFC] Revamp interface for enum representations #3050

Open

Using rustified enums in function signatures is (almost?) always wrong #3051

Open

jbaublitz force-pushed the issue-2646 branch 3 times, most recently from 39f1b4a to 9012360 Compare December 16, 2024 14:45

Add option to generate safe and unsafe conversions for rustified enums

8581938

jbaublitz force-pushed the issue-2646 branch from 9012360 to 8581938 Compare December 17, 2024 15:25

jbaublitz marked this pull request as ready for review December 17, 2024 15:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add option to generate safe and unsafe conversions for rustified enums #2908

Add option to generate safe and unsafe conversions for rustified enums #2908

jbaublitz commented Aug 20, 2024

jbaublitz commented Aug 20, 2024

ojeda commented Aug 21, 2024

ojeda Aug 21, 2024

jbaublitz Aug 21, 2024

ojeda Aug 21, 2024

jbaublitz Aug 21, 2024

jbaublitz commented Aug 21, 2024

ojeda commented Aug 21, 2024

jbaublitz commented Aug 21, 2024 •

edited

Loading

pvdrz commented Sep 24, 2024

jbaublitz commented Oct 23, 2024

ojeda commented Nov 18, 2024

ojeda Nov 18, 2024

jbaublitz Dec 5, 2024

ojeda Dec 5, 2024

jbaublitz Dec 5, 2024

ojeda Dec 6, 2024

pvdrz Dec 6, 2024 •

edited

Loading

ojeda Dec 6, 2024 •

edited

Loading

pvdrz Dec 6, 2024 •

edited

Loading

ojeda Dec 6, 2024

pvdrz Dec 6, 2024

pvdrz Dec 6, 2024 •

edited

Loading

pvdrz Dec 6, 2024 •

edited

Loading

pvdrz Dec 6, 2024 •

edited

Loading

pvdrz Dec 6, 2024

pvdrz commented Dec 6, 2024

jbaublitz commented Dec 7, 2024

pvdrz commented Dec 9, 2024

jbaublitz commented Dec 16, 2024

jbaublitz commented Dec 17, 2024

Add option to generate safe and unsafe conversions for rustified enums #2908

Are you sure you want to change the base?

Add option to generate safe and unsafe conversions for rustified enums #2908

Conversation

jbaublitz commented Aug 20, 2024

jbaublitz commented Aug 20, 2024

ojeda commented Aug 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jbaublitz commented Aug 21, 2024

ojeda commented Aug 21, 2024

jbaublitz commented Aug 21, 2024 • edited Loading

pvdrz commented Sep 24, 2024

jbaublitz commented Oct 23, 2024

ojeda commented Nov 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdrz Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

ojeda Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

pvdrz Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdrz Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

pvdrz Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

pvdrz Dec 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pvdrz commented Dec 6, 2024

jbaublitz commented Dec 7, 2024

pvdrz commented Dec 9, 2024

jbaublitz commented Dec 16, 2024

jbaublitz commented Dec 17, 2024

jbaublitz commented Aug 21, 2024 •

edited

Loading

pvdrz Dec 6, 2024 •

edited

Loading

ojeda Dec 6, 2024 •

edited

Loading

pvdrz Dec 6, 2024 •

edited

Loading

pvdrz Dec 6, 2024 •

edited

Loading

pvdrz Dec 6, 2024 •

edited

Loading

pvdrz Dec 6, 2024 •

edited

Loading