Add support for records (dictionaries / structs) to the proc-macro frontend #1360
Conversation
jplatte
force-pushed
the
jplatte/proc-macro-record
branch
from
5d39296 to
dbef14d
Compare
| #[uniffi::export] | ||
| fn make_object() -> Arc<Object> { | ||
| Arc::new(Object) | ||
| } |
There was a problem hiding this comment.
It's been nice seeing this feature grow. There's a quite a bit of functionality here and it's expressed in a simple and clean way.
| @@ -67,6 +67,9 @@ pub enum Type { | |||
| External { name: String, crate_name: String }, | |||
| // Custom type on the scaffolding side | |||
| Custom { name: String, builtin: Box<Type> }, | |||
| // An unresolved type inside a proc-macro exported function signature. | |||
| // Must be replaced by another type before bindings generation. | |||
There was a problem hiding this comment.
I think it's worth mentioning that this will only be a Record/Object/Enum, etc. and not a primitive type like u8 or String. I don't know it's an official UniFFI term, but I've been calling these "user types".
There was a problem hiding this comment.
I've updated it to "user-defined type".
| .unzip(), | ||
| None => { | ||
| let unimplemented = quote! { ::std::unimplemented!() }; | ||
| (unimplemented.clone(), unimplemented) |
There was a problem hiding this comment.
I think this could actually be implemented with an empty body or maybe a () statement. I don't think we have much use for unit structs as records, but is there a downside to implementing them?
There was a problem hiding this comment.
I don't know whether there is a downside to allowing this derive on unit structs, but note that this code path is also taken when the input is an enum or union. If it is, we also emit a compile error (line 71).
There was a problem hiding this comment.
Ahh, that makes a lot of sense.
| <#ty as ::uniffi::FfiConverter>::write(obj.#ident, buf); | ||
| }; | ||
| let try_read_field = quote! { | ||
| #ident: <#ty as ::uniffi::FfiConverter>::try_read(buf)?, |
There was a problem hiding this comment.
I like this as a first step. I guess the next step would be getting it to work with user types, where the type doesn't implement FfiConverter itself. I think that goes back to an issue that we discussed very early on.
Do you have a plan for this? If not, I could spend some time trying to get it work if that would be helpful.
There was a problem hiding this comment.
Yes, this goes back to what we discussed pretty early on. I would still prefer to not support "external" ffi-converter types, and instead encourage the newtype pattern where people were previously implementing FfiConverter with a RustType other than Self.
For the multi-crate setups I'm thinking about, neither would be required though; there's also the option of types having #[cfg_attr(feature = "uniffi", derive(uniffi::Record))] so a crate can optionally have a FfiConverter impl in an upstream crate.
There was a problem hiding this comment.
I do think that almost all of the time it's going to be one of those multi-crate setups where you control all the crates.
For completely external types, I can be convinced that requiring the newtype pattern is the best way to go. I'm mostly just concerned that there's some use case out there where this would be lead to very bad ergonomics. I'm going to put the question out there on #uniffi and see if anyone has a comment.
jplatte
force-pushed
the
jplatte/proc-macro-record
branch
from
dbef14d to
b18f127
Compare
Part of #1257.
Adds a
#[derive(uniffi::Record)]macro and updates#[uniffi::export]to allow usage of arbitrary types as long as they don't have generics and are available undercrate::uniffi_typesand have the right trait implementation.Outside of
uniffi::export, when generating bindings, the names of such types are resolved against the known type definitions from both UDL and derives likeRecord(Enummight come next).