-
Notifications
You must be signed in to change notification settings - Fork 235
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 proc-macro external type support (#1531) #1600
Conversation
b1b7318
to
70f0431
Compare
@bendk I just tried to build with this branch and hit a number of errors. Most of them related to an error type called KotlinI'm getting This happens when the error is procmacro and when its udl imported with Swift
|
70f0431
to
e95aa4f
Compare
Thanks for testing this out with some real-world examples. It's really nice to get concrete feedback.
|
thankyou!! |
fixtures/metadata/src/tests.rs
Outdated
@@ -132,12 +132,15 @@ mod test_type_ids { | |||
#[test] | |||
fn test_user_types() { | |||
check_type_id::<Person>(Type::Record { | |||
crate_name: "uniffi_fixture_metadata".into(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think uniffi meta calls the same concept module_path
, which seems yet more general than this - any reason to not use that same concept - IIUC that would mean we just use a name from a sub-module, whereas crate_name
implies only the root?
uniffi_meta/src/child_types.rs
Outdated
fn child_types_mut(&mut self) -> Vec<&mut Type>; | ||
} | ||
|
||
impl ChildTypesMut for crate::Metadata { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've nearly turned TypeIterator
into a trait before, which would make even more sense here I think? Is there any reason to not do that here? ie, define the trait as a more general IterTypes
and later we might let it be used in more places (eg, in that example above)
Although really, I don't quite understand how this differs from the above example? I guess because we don't actually have a struct to represent Type::External
?
(hrmph - the fact this is mut
also seems odd. I'll look at this in more detail again soon)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand how a future multi-crate world looks:
- my module_path comment before is likely wrong - I guess it assumes a module inside a crate - ie, the top segment of the path probably is not a crate name.
- However, ISTM that a ComponentInterface has always been a representation of a crate, and the
MetadataGroup
struct reinforces that - it's a kind of "here are all the items in this crate/namespace, please explode a CI up. - Or maybe not? It's adding them to a CI, not creating a new one, in which case this patch does make some sense, although ISTM that the various Metadata items and the uniffi_bindgen::interface:: structs would also need them at some point?
- And it further means simple name resolution can't be done with just a name - ie, you can't reject 2 interfaces with the name
Foo
unless they are in the same crate - which isn't handled here (but you would want to reject builtin names without considering the module name, and presumably you would not want these types duplicated per crate)
uniffi_meta/src/lib.rs
Outdated
name: String, | ||
}, | ||
Record { | ||
crate_name: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I touched on this before, but there does seem some mismatch here - Type::Record has a crate_name
field, whereas RecordMetadata
has a module_path
field - are these the same concept? Having it in the metadata makes more sense IMO - the Type
doesn't really need to know this - ISTM that what does need to know this is the uniffi_bindgen::interface::*
structs, which is closely related to the metadata.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing crate_name
to module_path
makes sense to me.
I think it's reasonable to encode the crate_name/module path in the Type
metadata is that metadata is shared between all crates. Functions from one crate should see that type as a Type::Record
, while functions from another crate should see it as a Type::External
.
However, maybe a better system would be to lean into the concept of type identifiers and only store a path to the type definition. I'm picturing something like:
- There's a
TypeDefinition
enum with variants for record, object, etc. - There's a
TypeDefinitionUniverse
that maps full module paths, including the crate name, toTypeDefinition
s - The
TYPE_RECORD
,TYPE_ENUM
, etc. constants that are used for function signature arguments are replaced withTYPE_USER
, which is followed by a module path string that keys intoTypeDefinitionUniverse
. - All of this can be used to construct a
ComponentInterface
for each crate
Side note: currently module_path
is just a crate name, because of some reasons that I forgot. I believe it should be possible to make it a full path though.
e95aa4f
to
b5f1e6b
Compare
@mhammond I rebased and implemented the changes that I could remember us discussing. Is there anything more to do for this PR? (I'll make sure to squash before merging). |
b5f1e6b
to
bc507fd
Compare
fc0ba42
to
280fe84
Compare
Moved the FfiConverter implementation for callback interfaces and traits from a declarative macro in `uniffi_core` to a proc-macro in `uniffi_macros`. This offers better `UniFfiTag` handling. Now `FfiConverter` is implemented for all tags for callback interfaces/traits that are wrapped with the proc-macro attributes. This matches the behavior for other types and will help fix mozilla#1531. Changed the object FfiConverter to be inside a proc-macro, rather than using the `Interface` trait. This more flexibility and avoids conflicting impls. For example, this allows for both the objects and trait interface `FfiConverters`s to be implemented on `Arc<T>`. I also think it's nicer with less indirection. One drawback is that libraries can't implement `FfiConverter` on Arc<T> because of the orphan rules. To get around this, I added the `FfiConverterArc` trait. Other changes: - Adding `module_path` to the user type metadata - Added an proc-macro -> proc-macro external type test by copying the ext-types fixture into another one that uses proc macros.
280fe84
to
f004180
Compare
This allows proc-macro based libraries to use types defined in UDL files. It also paves the way for proc-macros to support remote types from non-UniFFI types.
I'm hoping we can sneak this in to the
0.24
release.