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 proc-macro external type support (#1531) #1600

Merged
merged 1 commit into from
Jun 29, 2023

Conversation

bendk
Copy link
Contributor

@bendk bendk commented Jun 15, 2023

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.

@bendk bendk requested a review from a team as a code owner June 15, 2023 23:29
@bendk bendk requested review from jhugman and removed request for a team June 15, 2023 23:29
@bendk bendk added this to the v0.24.0 milestone Jun 15, 2023
@bendk bendk requested a review from a team June 16, 2023 00:04
uniffi_macros/src/util.rs Outdated Show resolved Hide resolved
uniffi_macros/src/record.rs Outdated Show resolved Hide resolved
@bendk bendk force-pushed the proc-macro-external branch 2 times, most recently from b1b7318 to 70f0431 Compare June 20, 2023 18:30
@stan-irl
Copy link
Contributor

stan-irl commented Jun 21, 2023

@bendk I just tried to build with this branch and hit a number of errors. Most of them related to an error type called ApiErr that I reuse in different crates as an external for funcs that return Result<Foo, ApiErr>:

Kotlin

I'm getting unsupported type for error: External when generating kotlin: https://github.com/mozilla/uniffi-rs/blob/main/uniffi_bindgen/src/bindings/kotlin/gen_kotlin/mod.rs#L238.

This happens when the error is procmacro and when its udl imported with uniffi::use_udl_error!(dependent_crate, ErrorType); - i tried both

Swift

  • UniFfiForeignExecutor is declared in every crate that has an async method which causes duplicate declaration error. Im not sure if that belongs here or maybe its a dedicated issue
  • I get 'lift' is inaccessible due to 'private' protection level on FfiConverterTypeApiErr.lift where ApiErr is the external error I mentioned above. Thats happening in the following generated swift:
private func uniffiFutureCallbackHandlerVoidTypeApiErr(
    rawContinutation: UnsafeRawPointer,
    returnValue _: UInt8,
    callStatus: RustCallStatus
) {
    let continuation = rawContinutation.bindMemory(
        to: CheckedContinuation<Void, Error>.self,
        capacity: 1
    )

    do {
        try uniffiCheckCallStatus(callStatus: callStatus, errorHandler: FfiConverterTypeApiErr.lift).  <------ HERE
        continuation.pointee.resume(returning: ())
    } catch {
        continuation.pointee.resume(throwing: error)
    }
}

@badboy badboy modified the milestones: v0.24.0, v0.24.1+ Jun 21, 2023
@bendk
Copy link
Contributor Author

bendk commented Jun 22, 2023

@stan-irl

Thanks for testing this out with some real-world examples. It's really nice to get concrete feedback.

  • I believe the error issue is a general one. I got the same issue with UDL files with this branch. Let's discuss in External Errorsupport #1605.
  • I believe the issue with duplicate definitions is solved by Swift external type module config #1404, but that's gotten fairly bitrotted and I need to do a rebase pass on it.
  • I just pushed a commit to fix the visibility issue with lift.

@bendk bendk mentioned this pull request Jun 22, 2023
@stan-irl
Copy link
Contributor

thankyou!!

@@ -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(),
Copy link
Member

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?

fn child_types_mut(&mut self) -> Vec<&mut Type>;
}

impl ChildTypesMut for crate::Metadata {
Copy link
Member

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

Copy link
Member

@mhammond mhammond left a 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)

name: String,
},
Record {
crate_name: String,
Copy link
Member

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.

Copy link
Contributor Author

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, to TypeDefinitions
  • The TYPE_RECORD, TYPE_ENUM, etc. constants that are used for function signature arguments are replaced with TYPE_USER, which is followed by a module path string that keys into TypeDefinitionUniverse.
  • 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.

@bendk
Copy link
Contributor Author

bendk commented Jun 29, 2023

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

@bendk bendk force-pushed the proc-macro-external branch 2 times, most recently from fc0ba42 to 280fe84 Compare June 29, 2023 13:27
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.
@bendk bendk merged commit f004180 into mozilla:main Jun 29, 2023
4 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants