Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jbaublitz
Copy link
Contributor

Closes #2646

@jbaublitz jbaublitz marked this pull request as draft August 20, 2024 16:33
@ojeda ojeda added the rust-for-linux Issues relevant to the Rust for Linux project label Aug 20, 2024
@jbaublitz
Copy link
Contributor Author

@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?

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

ojeda commented Aug 21, 2024

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 Outdated Show resolved Hide resolved
#[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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment on lines 4 to 6
pub type Planet_ctype = ::std::os::raw::c_uint;
pub const Planet_earth: Planet_ctype = 0;
pub const Planet_mars: Planet_ctype = 1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jbaublitz force-pushed the issue-2646 branch 7 times, most recently from 8c41d55 to 52851c7 Compare August 21, 2024 16:11
@jbaublitz
Copy link
Contributor Author

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?

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

ojeda commented Aug 21, 2024

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
Copy link
Contributor Author

jbaublitz commented Aug 21, 2024

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
Copy link
Contributor

pvdrz commented Sep 24, 2024

@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
Copy link
Contributor Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rust-for-linux Issues relevant to the Rust for Linux project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add enum option constified_enum + rustified_enum with conversions
3 participants