-
Notifications
You must be signed in to change notification settings - Fork 231
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
New custom type system #2150
New custom type system #2150
Conversation
14c9e19
to
de382d1
Compare
@mhammond I just added an upgrading guide for users. I'm not sure how to hook it in to the new doc system though. Does this mean updating the |
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.
oops, I had these pending but I think they are still worthwhile
docs/manual/src/udl/custom_types.md
Outdated
party libraries. This works by converting to and from some other UniFFI type to move data across the | ||
FFI. You must provide a `UniffiCustomTypeConverter` implementation to convert the types. | ||
Custom types allow you to extend the UniFFI type system by converting to and from some other UniFFI | ||
type to move data across the FFI. |
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.
This is the entry-point to custom type docs - so maybe add a para or 2 intro here - eg, describe our Guid/Handle types, how they wrap their types, what bindings see, before getting into the detail below. I'd even think about moving the custom_newtype
example above this.
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.
Great idea. I added an overview/motivation for custom types here and thought it looked good so I didn't move the custom_newtype
example. WDYT?
I'm not quite sure what you are asking - you need to list it in https://github.com/mozilla/uniffi-rs/blob/main/mkdocs.yml, but what's less clear is how this new file (which I think could maybe be renamed to just "upgrading.md"?) will function. Off-hand, I'd say our release process should be tweaked to say (a) remove old entries from this file and (b) have a quick check over the changelog for new items that should be added. Or did you have something else in mind? |
de382d1
to
97174d4
Compare
I was just looking for that YAML file, thanks! |
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 really like this adjustment — I feel like it works better with the switch to macros for the bindings for other types
97174d4
to
d62d864
Compare
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 spent more time trying to understand the bigger picture here than looking at the implementation details of this - which got me back to looking more at #2087 so see how all these pieces fit together. I think I'm getting my head around it and I like it!
Here's a possibly controversial take on the naming though:
I never really liked the old names very much (I think I might have introduced them),
and sadly I think I like the new ones even less - but I see an opportunity:
So here's the old world:
impl UniffiCustomTypeConverter for MyType {
type Builtin = UniFFIBuiltinType;
fn into_custom(val: Self::Builtin) -> uniffi::Result<Self> {
...
}
fn from_custom(obj: Self) -> Self::Builtin {
...
}
}
What I like about this is the liberal use of the term "builtin" and the fact that the types are explicit.
It's clear what Self
refers to and it's very clear what's the "builtin" vs the "custom".
Here's the new world:
// Expose `http::HeaderMap` to Uniffi.
uniffi::custom_type!(HeaderMap, Vec<HttpHeader>, {
from_custom: |obj| { ... }
try_into_custom: |val| { ... }
}
What I don't like about this is that the term "builtin" is no longer used and there are no types in the signatures.
Instead, just the term "custom" appears, and without much experience with UniFFI it might not be obvious what is meant by the term (eg, it might be easy to convince yourself that the term "custom" actually is referring to the builtin if that's the only term actually used)
When I read the other docs and the implementation though, I see:
// Defining `From<Handle> for i64` also gives us `Into<i64> for Handle`
impl From<Handle> for i64 { ... }
impl TryFrom<i64> for Handle { ... }
Which makes me think - why not change the new model to:
// Expose `http::HeaderMap` to Uniffi.
uniffi::custom_type!(HeaderMap, Vec<HttpHeader>, {
into_builtin: |obj| { ... }
try_from_builtin: |val| { ... }
}
The improvements here IMO are:
- The types become that little more obvious - what the "builtin" means seems more obvious.
- It better matches how Rust names the equivalent functionality, so is more likely to be familiar to the reader.
- It better matches the actual implementation and makes the docs that bit easier to read.
eg, the docs can say something like "into_builtin defaults to Into<Builtin>
and "try_from_builtin" defaults to TryFrom<Builtin>
.
Indeed, they can even encourage you to implement those builtin traits instead of manually specifying the "overrides" (which they kinda do in your patch, but having this consistency makes that story stronger)
It could be argued it makes the breakage that little bit more difficult to mechanically fix,
but it seems like it is actually a much better fit for how it actually works and seems far more intuitive for new UniFFI users.
It's an opportunity we will probably not get again.
(I'll also admit this came to me just last night, so I haven't thought it through in great detail, but though it worth throwing out there anyway!)
d62d864
to
d3140ef
Compare
This is a very good point. I'd love to get a very specific and easy to understand name here. The reason I didn't go with "builtin" is that type doesn't actually need to be a builtin type. You could convert the custom type into a user-defined record/enum/interface. Would this actually happen in practice? I wasn't really sure. Maybe this would be a good opportunity to introduce a term here. There were a lot of times when I was writing docs that I wanted a word for a type that had the FFI traits implemented, but couldn't find a good one. Sometimes I wanted to call it a "UniFFI type", but it's not so clear what that means from the words alone. I usually ended up saying something like "a type that can be used in the exported UniFFI API", which was pretty awkward. I think it could be good to pick a term and add a section in the manual that defined it. "UniFFI type" or "UniFFI-enabled type" are the best names that I could come up with. WDYT?
True, but isn't this also true for the current code? You could say " |
This is the http-header example right? The "custom type" is a remote type and the "builtin" type is a record? I agree "builtin" isn't a great name, but I think it's important the concept have a name.
My take is that:
|
or to put my last comment another way - I think the term "builtin" is more problematic than "custom", because it implies records etc can not be used - but having 2 distinct "not ideal" terms is somewhat better than just using a single "ideal" term. |
Using two terms for the two different types makes a lot of sense. What if we leaned into that more and used a syntax like this?
|
What I do like about this is what I liked about my idea above - ie, I think this is basically as good as simply
because it does use the 2 terms. In both cases though, the sticking point remains more "builtin" than "custom". Another bad option might be "existing"? |
I've been thinking about this one off and on for a bit and "existing" seems like the best option to me, should we just go with that one? |
That sgtm, at least as the next straw-man! |
342647a
to
83a2328
Compare
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.
This implementation and motivation is great, thanks for your patience while we catch up with your thinking.
I'm now a bit worried that my naming suggestions of before have actually made things worse. Maybe some small tweaks to the docs are all that's necessary, but I do think it's still worth thinking about before landing - that naming is almost the only part of this which will be visible by users so it's worth getting right, and I'm really sorry if I'm just making it worse or more confusing. Once I get over that I think this is ready to go.
CHANGELOG.md
Outdated
- The Rust side of the custom type system has changed and users will need to update their code. | ||
The `UniffiCustomTypeConverter` trait is no longer used, use the `custom_type!` macro instead. | ||
We did this to help fix some edge-cases with custom types wrapping types from other crates (eg, Url). | ||
See https://mozilla.github.io/uniffi-rs/0.29/Upgrading.html for help upgrading and https://mozilla.github.io/uniffi-rs/0.29/udl/custom_types.html for details. |
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.
gah - a bit of a side issue, but these links will be 404s until the next release, which doesn't seem ideal. 2 options we have seem to be:
- link to /next instead. This works in the short term, but is likely to go stale over the longer term. I'm not sure going stale matters in practice though in terms of old changelog entries.
- Our docs process is mildly tweaked so the presumed next version (0.29 in this case) is always created as soon as the last version is released.
The problem with that second option is that it assumes 0.29 will be the next version - while I see no reason to believe that's not true, you never know. Eg, if we manage to squeeze a number of other breaking improvments in, we might decide to skip 0.29 and move directly to 0.30
Regardless, this is less about this PR and more about our docs in general, so this doesn't need to block anything.
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.
Let's use next
. It works for now and I agree that going stale over the long term is not the biggest deal.
docs/manual/src/Upgrading.md
Outdated
``` | ||
|
||
The custom_type macro is more flexible than the old system. For example, the `try_from_existing` and | ||
`into_existing` can be omitted in many cases, and will use the `TryInto` and `From` traits by default. |
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.
Reading the docs below, it looks like we might also use Into
and TryFrom
depending on the direction/context. If so, could you please tweak this? In particular, this sentence almost seems like the reverse of what's sensible (ie, that we have a try_from
method, which uses From
if omitted, and we have an into
method which, if omitted, uses TryInto
appears wrong and inconsistent).
(If I'm not wrong about that, the changes make here could make the text quite vague, which I think is OK if the alternative makes things appear more restrictive than it is)
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.
Thinking about this some more, I think we should spell out the exact trait and always use the into version. So this should say "For example, the try_from_existing
and into_existing
can be omitted in many cases, and will use the TryInto<Custom>
and Into<Existing>
traits by default." The reason for this is that Into is auto-derived from From, but not the other way around.
Maybe we should rename try_from_existing
to try_into_custom
to match this?
docs/manual/src/Upgrading.md
Outdated
|
||
``` | ||
uniffi::custom_type!(MyType, UniFFIBuiltinType, { | ||
try_from_existing: |val| { ... }, |
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.
really nit-picking here, but I wonder if this line should say, eg, try_from_existing: |val| { Ok(...) },
to highlight the fact this is a Result<>
?
docs/manual/src/udl/custom_types.md
Outdated
* Represented by the `java.net.URL` type in Kotlin | ||
|
||
Lastly, the `Guid(String)` example shows another benefit of custom types: they can be used to | ||
support Rust types that are not currently supported by UniFFI like tuple-style structs. |
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.
tuple structs are supported by procmacros.
docs/manual/src/udl/custom_types.md
Outdated
|
||
- Values passed to the foreign code will be converted using `<SerializableStruct as Into<String>>` before being lowered as a `String`. | ||
- Values passed to the Rust code will be converted using `<String as TryInto<SerializableStruct>>` after lifted as a `String`. | ||
- The `TryInto::Error` type can be anything that implements `Into<anyhow::Error>`. |
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.
My brain hurts - following on from the last comment - have we got these names correct? We have into_existing
which doesn't return a result, but here talking about "TryInto" (which does) seems confusing and backwards.
I'm worried my last suggestion which gave us these new names actually made things more complicated?
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 I just got this backwards and I'll reverse them.
83a2328
to
12dbb13
Compare
Please nit-pick, I agree that the names are very important here. I just pushed some fixes/clarifications to the docs. I'm currently 50/50 on renaming |
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.
Think of these comments more as brainstorming than a concrete proposal :)
12dbb13
to
3ed223d
Compare
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.
Just a few thoughts. I think it's a bit confusing reading existing
, custom_newtype
and custom_type
.
I would like to define overall naming patterns somewhere.
We have:
- Built-in types from the Rust world
- User created types via
struct
andenum
- UniFFI types
- Types from external crates
How about:
primitive_type
custom_type
bridge_type
external_type
There is a lot of context that I am properly missing. But might be worth defining the terms we use first, and then adjust the conventions in the codebase.
3ed223d
to
366cba1
Compare
It's great to hear a take coming from someone who hasn't been so deep in the weeds on this one. I'm going to try to rephrase it slightly, please tell me if I'm missing anything:
On the Rust side, this would work exactly like a custom_newtype today. On the foreign side, we could define a At this point, I think we're talking about more than one PR worth of work. Maybe we could split this up into multiple ones:
|
Idk whether this question was directed at @gruberb or everybody, but personally I really like |
If others like bridge type, that works for me. |
We discussed this today, got some more clarity and came up with some decisions. I misunderstood part of Bastian's point. He's fine with "custom type" as a name, the issue was with the other type involved, which he wants to name "bridge type". The new plan is to:
|
01de4f7
to
1cccb70
Compare
Updated the language as describe above. I think this is maybe finally ready to land, tell me what you think. |
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.
This is great, thanks so much for your patience shepherding this through! I'd still love to get more eyes on this, so maybe give it a week or so for them to magically materialize :)
1cccb70
to
d797b8e
Compare
I'm really happy to see this work happen. Thank you so much Ben! Having read the discussion about names, I totally agree with you about My suggestion would be to keep the uniffi::custom_type!(HeaderMap, Vec<HttpHeader>
into_lowered: |obj| { ... },
try_from_lowered: |val| { ... },
); alternates, using verbs: uniffi::custom_type!(HeaderMap, Vec<HttpHeader>
lower: |obj| { ... },
try_lift: |val| { ... },
); My hunch would be that if you're at the stage of adding new types to cross the FFI, then you've already found the lifting and lowering concepts already prominent in the docs. |
This is a pretty interesting idea. Conceptually, it means that values would get lowered/lifted several times ( My initial feeling is that I like it, what do others think? |
d797b8e
to
57aae50
Compare
I like the suggestion of going with |
5da16bd
to
f70464f
Compare
f70464f
to
b7bdd25
Compare
We discussed this yesterday and there's general agreement to lean into the lower/lift terminology to describe this. I updated the PR to use I'd love to get this merged. I think @mhammond has some suggestions for the new doc page, does anyone else have suggestions/comments? |
Implemented a new custom type system described in the new docs. This system is easier to use and also allows us to do some extra tricks behind the scene. For example, I hope to add a flag that will toggle between implementing `FfiConverter` for the local tag vs a blanket impl. It's not possible to do that with the current system and adding support would be awkward. I wanted to keep backwards compatibility for a bit, but couldn't figure out a good way to do it. The main issue is that for the old system, if a custom type is declared in the UDL then we generate the `FfiConverter` implementation, while the new system expects the user to call `custom_type!` to create the impl. I tried to get things working by creating a blanket impl of `FfiConverter` for types that implemented `UniffiCustomTypeConverter`, but ran into issues -- the first blocker I found was that there's no way to generate `TYPE_ID_META` since we don't know the name of the custom type. Removed the nested-module-import fixture. The custom type code will no longer test it once we remove the old code, since it's not using `UniffiCustomTypeConverter`. I couldn't think of a way to make it work again.
Merged some great updates to the docs from Mark and updated the changelog. Once this passes CI I'm going to hit the merge button. |
ea03815
to
4de0a3a
Compare
PR notes: I split this out out from #2087 so that we can discuss the custom type changes separate from the remote type changes. I think the new syntax is pretty uncontroversial, but this does require a breaking change so I wanted to make room to discuss that separate from the main conversation. On the topic of breaking changes: I tried but failed on avoiding them. Maybe it's possible with some extra work, but I don't think it's worth it at this point. How does that sound to others?
Implemented a new custom type system described in the new docs. This system is easier to use and also allows us to do some extra tricks behind the scene. For example, I hope to add a flag that will toggle between implementing
FfiConverter
for the local tag vs a blanket impl. It's not possible to do that with the current system and adding support would be awkward.I wanted to keep backwards compatibility for a bit, but couldn't figure out a good way to do it. The main issue is that for the old system, if a custom type is declared in the UDL then we generate the
FfiConverter
implementation, while the new system expects the user to callcustom_type!
to create the impl. I tried to get things working by creating a blanket impl ofFfiConverter
for types that implementedUniffiCustomTypeConverter
, but ran into issues -- the first blocker I found was that there's no way to generateTYPE_ID_META
since we don't know the name of the custom type.Removed the nested-module-import fixture. The custom type code will no longer test it once we remove the old code, since it's not using
UniffiCustomTypeConverter
. I couldn't think of a way to make it work again.