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

Common system for remote type handling (#1865) #2303

Closed
wants to merge 1 commit into from

Conversation

bendk
Copy link
Contributor

@bendk bendk commented Nov 9, 2024

(This is the continuation of #2087, now that #2150 has been merged)

Remote types present an issue because we can't implement FFI traits like Lower and Lift on them directly. The workaround is to only implement the FFI trait for the current crate. If another crate wants to use that impl, we need to implement it again by forwarding to the initial implementation.

Historically, UDL and proc-macros have approached this differently which has lead to a lot of extra complexity and confusion. This change aligns them on a common system: By default, we implement traits for all crates (a "blanket impl"). However, users can opt-in to only implementing it for the current crate ("a local impl`)

See the doc changes for a description of the new system.

This required a bunch of changes:

  • UDL now creates a blanket impl by default. The [Remote] attribute can be used to switch to a local impl.
  • Proc-macros get the #[remote] attribute macro, which allows users to create a local impl.
  • Custom types now create a blanket impl by default. The remote parameter can be used to create a local impl.
  • Added the remote_type! macro to which handles the case where you want to use a remote type and another crate has already created a local impl. This creates a local impl for your crate by forwarding to the other impl. Removed the use_udl_* macros, which were a kludge for doing the same thing.

Added a motivating example where anyhow::Error is used as an error type. Changed the error type bound to Display + Debug rather than Error. For some reason anyhow::Error doesn't actually implement error.

One feature that we've lost in defining callback/trait interfaces using traits defined in a remote crate. I think this is okay because I doubt anyone is using that feature.

@bendk bendk requested a review from a team as a code owner November 9, 2024 17:39
@bendk bendk requested review from jeddai and removed request for a team November 9, 2024 17:39
* Enums: `enum`
* Records: `record`, `dictionary` or `struct`
* Objects: `object`, `impl` or `interface`
* Traits: `trait`, `callback` or `trait_with_foreign`
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm hoping we can implement #2302 as a follow-up to remove the need for all of this, remove the legacy code to deal with [Rust=*] and typedef external, and generally clean up uniffi_udl


### Swift

For Swift, you must compile all generated `.swift` files together in a single
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we best mention how the user does this :)

Something uniffi-cli --language swift something something library mode?
(Long time since I took a look at the params to the binding generator)??

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I definitely agree, but I'm actually not sure what to advise a user. I know how we build our components, but I don't exactly want to recommend that to others. I think there's a cargo swift command that helps out with this, but don't know enough about that to recommend it.

The tutorial says "Note that these commands could be integrated as part of your gradle/Xcode build process", which seems like the best advice we have to offer. I'd love to improve this story, are you interested in writing something up?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok so my point is we need to guide user how to generate the swift bindings into a single module map.

Uniffi does this properly already for bindgen tests, by combining the modulemaps here:

let path = out_dir.join("combined.modulemap");

So I just think we need to surface that to the uniffi bindgen cli tool? At least optin by some flag

I see that 2 months ago you did some work on Swift bindgen,

* Generate a single modulemap for a library.

Perhaps you have already added support for merging of the generates swift bindings for multi crate setup?

Otherwise I think UniFFI Swift users will be hit with this issue:

#2087 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, so I made the uniffi-bindgen-swift CLI which has a way to create a combined modulemap and also a flag for XCFramework compatibility. It also allows generating only one kind of source file (.modulemap, .h, or .swift) at a time, so you can put different types in different directories. It's very flexible but I'm not sure what to recommend for concrete use cases: how should users integrate it with an XCode project? How should they create a SPM package? Are there other common build systems?

I'm not a Swift dev. I don't even know if I'm asking the right questions here. I would love it if someone who does know Swift would write up some docs on this. I'd be happy to help by describing what uniffi-bindgen-swift can do and adding whatever new features would simplify this process.

Copy link
Contributor

@praveenperera praveenperera Nov 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The combined module's would be very useful for me, because then I can split up my current single rust library to multiple ones in a workspace for better compile times: #2257 (comment).

I don't need to generate SPM package for the project I reference above (Cove) because its an app not a library.

But creating an SPM, I have a library for a different project written in rust, that published for SPM. The SPM zip is created in GitHub actions: https://github.com/bitcoinppl/bbqr-swift/blob/master/.github/workflows/publish-spm.yml

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To expand on Ben's comments, we aren't Swift experts at all, so we need things to be very explicit :)

Otherwise I think UniFFI Swift users will be hit with this issue:

This repo is a UniFFI Swift user and apparently isn't being hit by that issue. Maybe you are saying "Some Swift users", or "Swift users who use this particular pattern" or something else - but it seems that not all Swift users will, and I don't think the core maintainers have enough Swift experience to untangle that.

docs/manual/src/udl/remote_ext_types.md Outdated Show resolved Hide resolved
Remote types present an issue because we can't implement FFI traits like
`Lower` and `Lift` on them directly.  The workaround is to only
implement the FFI trait for the current crate.  If another crate
wants to use that impl, we need to implement it again by forwarding to
the initial implementation.

Historically, UDL and proc-macros have approached this differently which
has lead to a lot of extra complexity and confusion.  This change aligns
them on a common system:  By default, we implement traits for all crates
(a "blanket impl").  However, users can opt-in to only implementing it
for the current crate ("a local impl`)

See the doc changes for a description of the new system.

This required a bunch of changes:

- UDL now creates a blanket impl by default. The [Remote] attribute can
  be used to switch to a local impl.
- Proc-macros get the `#[remote]` attribute macro, which allows users to
  create a local impl.
- Custom types now create a blanket impl by default.  The `remote`
  parameter can be used to create a local impl.
- Added the `remote_type!` macro to which handles the case where you
  want to use a remote type and another crate has already created a
  local impl.  This creates a local impl for your crate by forwarding to
  the other impl.  Removed the `use_udl_*` macros, which were a kludge
  for doing the same thing.

Added a motivating example where `anyhow::Error` is used as an error
type.  Changed the error type bound to `Display + Debug` rather than
`Error`. For some reason `anyhow::Error` doesn't actually implement
error.

One feature that we've lost in defining callback/trait interfaces using
traits defined in a remote crate.  I think this is okay because I doubt
anyone is using that feature.
@bendk bendk closed this Nov 15, 2024
@bendk bendk deleted the push-xwmwtwtsyxuz branch November 15, 2024 15:58
@bendk
Copy link
Contributor Author

bendk commented Nov 15, 2024

Sorry, I had an issue with me jj repo that caused me to delete the branch. Github isn't letting me re-open the PR, so I created a new one: #2317.

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.

4 participants