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

Support for #[non_exhaustive] enums #1593

Closed
thunderbiscuit opened this issue Jun 13, 2023 · 5 comments
Closed

Support for #[non_exhaustive] enums #1593

thunderbiscuit opened this issue Jun 13, 2023 · 5 comments
Labels
enhancement New feature or request uniffi

Comments

@thunderbiscuit
Copy link

thunderbiscuit commented Jun 13, 2023

Our code is using enums in an upstream library that are marked as #[non_exhaustive]. We are currently writing wrappers around these to make them work, but I'm wondering if there is interest in eventually supporting them natively in uniffi. Cheers!

┆Issue is synchronized with this Jira Task
┆Issue Number: UNIFFI-282

@badboy badboy added the enhancement New feature or request label Jun 14, 2023
@bendk
Copy link
Contributor

bendk commented Dec 14, 2023

I don't think the issue is so much the non_exhaustive attribute, but the fact that the enums come from a different crate. Since the enums are defined in a different crate, we can't derive/define the trait implementations needed to lift/lower them directly. What I think you would want is some way to define an enum in your own crate, which mirrors the enum in the other crate, derive uniffi::Enum on that, and use something like custom types to have UniFFI convert between the two.

The non_exhaustive attribute creates a slight wrinkle there, but AFAICT the bigger issue is that custom types don't support this use case at all. Is that correct from your POV? Can you share which enum you're trying to use? I want to do some work on custom types in the new year, and some concrete use cases would be great.

@tnull
Copy link

tnull commented Dec 18, 2023

I don't think the issue is so much the non_exhaustive attribute, but the fact that the enums come from a different crate. Since the enums are defined in a different crate, we can't derive/define the trait implementations needed to lift/lower them directly.

No, for public enums without non_exhaustive defining UniFFI types in a downstream crate works perfectly fine, we've done that successfully for quite a while now, and there is not even a UniffiCustomTypeConverter implementation necessary.

What I think you would want is some way to define an enum in your own crate, which mirrors the enum in the other crate, derive uniffi::Enum on that, and use something like custom types to have UniFFI convert between the two.

Sure, we could newtype all the affected types. However, this would in turn mean that these newtype'd types are not compatible anymore with the original types. As we use this to implement projects that are libraries themselves, having each library expose another incompatible newtype around shared types coming from a common ancestor crate is a huge pain.
Adding and maintaining all these newtypes is also a bunch of work that would be entirely unnecessary if there was support for non_exhaustive enums.

The non_exhaustive attribute creates a slight wrinkle there, but AFAICT the bigger issue is that custom types don't support this use case at all. Is that correct from your POV? Can you share which enum you're trying to use? I want to do some work on custom types in the new year, and some concrete use cases would be great.

No, as stated above, this is not correct. A really simple example would be the Network type of the rust-bitcoin crate: https://docs.rs/bitcoin/latest/bitcoin/network/enum.Network.html
Many projects depend on this Rust library and quite a few of them use UniFFI. Up until recently, we could just pub use bitcoin::Network in a crate's lib.rs and define the corresponding enum Network in the UDL. However, recently non_exhaustive was added to this enum, which would require us to needlessly wrap it in an newtype/UniffICustomTypeConverter which comes with all the issues mentioned above. The solution here is for UniFFI to support non_exhaustive types.

@mhammond
Copy link
Member

This simple patch works fine:

diff --git a/fixtures/proc-macro/src/lib.rs b/fixtures/proc-macro/src/lib.rs
index d1f299178..54768a835 100644
--- a/fixtures/proc-macro/src/lib.rs
+++ b/fixtures/proc-macro/src/lib.rs
@@ -184,6 +184,7 @@ pub enum MaybeBool {
     Uncertain,
 }
 
+#[non_exhaustive]
 #[repr(u8)]
 #[derive(uniffi::Enum)]
 pub enum ReprU8 {

it would probably help is we had a concrete problem to solve here - ideally via a patch to one of our fixtures so we can see exactly what the problem is and have the start of a vehicle to test any possible fixes.

@bendk
Copy link
Contributor

bendk commented Dec 19, 2023

No, for public enums without non_exhaustive defining UniFFI types in a downstream crate works perfectly fine, we've done that successfully for quite a while now, and there is not even a UniffiCustomTypeConverter implementation necessary.

I think I understand now. There's a difference between how we handle this with UDL and with proc-macros and I keep mixing the two up. I think this should be fairly easy to fix right now, with UDL. However, the decision we make in #1865 might change that.

bendk added a commit to bendk/uniffi-rs that referenced this issue Jan 3, 2024
UDL types can now have the `[NonExhaustive]` attribute.  If present, we
add a default branch arm to make things compile correctly with
non-exhaustive enums.  I didn't want to make this the default behavior,
since in the normal case we want missing variants to result in compile
errors.

In theory, this also adds the `non_exhaustive` attribute for
proc-macros.  However, there's no way to test this because
`non_exhaustive` only matters for types defined in remote crates and
there's currently no proc-macro support for those.
bendk added a commit to bendk/uniffi-rs that referenced this issue Jan 3, 2024
UDL types can now have the `[NonExhaustive]` attribute.  If present, we
add a default branch arm to make things compile correctly with
non-exhaustive enums.  I didn't want to make this the default behavior,
since in the normal case we want missing variants to result in compile
errors.

In theory, this also adds the `non_exhaustive` attribute for
proc-macros.  However, there's no way to test this because
`non_exhaustive` only matters for types defined in remote crates and
there's currently no proc-macro support for those.

One slightly silly part of this is the flow of metadata.  It doesn't
make much sense as a metadata field, since it doesn't affect bindings
generation at all.  However, it needs to be because of the way that data
flows from `uniffi_udl` -> `uniffi_meta` -> `uniffi_bindgen` ->
`uniffi_macros`.
bendk added a commit to bendk/uniffi-rs that referenced this issue Jan 3, 2024
UDL types can now have the `[NonExhaustive]` attribute.  If present, we
add a default branch arm to make things compile correctly with
non-exhaustive enums.  I didn't want to make this the default behavior,
since in the normal case we want missing variants to result in compile
errors.

In theory, this also adds the `non_exhaustive` attribute for
proc-macros.  However, there's no way to test this because
`non_exhaustive` only matters for types defined in remote crates and
there's currently no proc-macro support for those.

One slightly silly part of this is the flow of metadata.  It doesn't
make much sense as a metadata field, since it doesn't affect bindings
generation at all.  However, it needs to be because of the way that data
flows from `uniffi_udl` -> `uniffi_meta` -> `uniffi_bindgen` ->
`uniffi_macros`.
bendk added a commit to bendk/uniffi-rs that referenced this issue Jan 3, 2024
UDL types can now have the `[NonExhaustive]` attribute.  If present, we
add a default branch arm to make things compile correctly with
non-exhaustive enums.  I didn't want to make this the default behavior,
since in the normal case we want missing variants to result in compile
errors.

In theory, this also adds the `non_exhaustive` attribute for
proc-macros.  However, there's no way to test this because
`non_exhaustive` only matters for types defined in remote crates and
there's currently no proc-macro support for those.

One slightly silly part of this is the flow of metadata.  It doesn't
make much sense as a metadata field, since it doesn't affect bindings
generation at all.  However, it needs to be because of the way that data
flows from `uniffi_udl` -> `uniffi_meta` -> `uniffi_bindgen` ->
`uniffi_macros`.
bendk added a commit that referenced this issue Jan 5, 2024
UDL types can now have the `[NonExhaustive]` attribute.  If present, we
add a default branch arm to make things compile correctly with
non-exhaustive enums.  I didn't want to make this the default behavior,
since in the normal case we want missing variants to result in compile
errors.

In theory, this also adds the `non_exhaustive` attribute for
proc-macros.  However, there's no way to test this because
`non_exhaustive` only matters for types defined in remote crates and
there's currently no proc-macro support for those.

One slightly silly part of this is the flow of metadata.  It doesn't
make much sense as a metadata field, since it doesn't affect bindings
generation at all.  However, it needs to be because of the way that data
flows from `uniffi_udl` -> `uniffi_meta` -> `uniffi_bindgen` ->
`uniffi_macros`.
@tnull
Copy link

tnull commented Jan 24, 2024

Ah, I totally missed #1593 at the time, so excuse the late answer here: I can confirm that #1593 works for us and we could resolve above mentioned issues with the 0.26 release.

Thank you very much for the quick response! This issue can be closed I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request uniffi
Projects
None yet
Development

No branches or pull requests

6 participants