Add option to generate safe and unsafe conversions for rustified enums

[jbaublitz]

Closes #2646

[jbaublitz]

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

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.

[jbaublitz]

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]

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]

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]

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

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

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

[pvdrz]

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

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

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

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]

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?