External custom types in UDL must use the [Custom] attribute #2371
Conversation
Previously they were declared with the `External` attribute, but this mean UniFFI could not know the underlying type, leading to issues like mozilla#2025. This PR does not however fix any such issues, it instead allows UniFFI metadata to carry enough information to fix in the future. This is primarily being done now to include this breaking change in with all other breaking changes we are making in the next release.
| ``` | ||
| [Custom="crate_name"] | ||
| typedef string Url; // replace `string` with any appropriate type. | ||
| ``` |
There was a problem hiding this comment.
I haven't looked over everything, but I think the main question is how this should be spelled out in the UDL. The main alternative in my mind is something like:
[External="crate_name"]
typedef custom Url;
The upside is that this looks like other external type definitions, the downside is that it doesn't define the bridged type. Maybe that's okay though. I think we can get that info from the metadata when --library is used.
The other case is when --library isn't used. In that case, maybe users can specify the bridged type with another attribute:
[External="crate_name", BridgeType="string"]
typedef custom Url;
This feels worse than the proposed syntax, but IMO bindgen without library mode is outdated and we should start deprecating it and/or making it somewhat of a second-class citizen.
There was a problem hiding this comment.
The upside is that this looks like other external type definitions, the downside is that it doesn't define the bridged type. Maybe that's okay though. I think we can get that info from the metadata when --library is used.
I don't think this is true - I actually started thinking it was, and started making the builtin elt of Custom an Option - the problem is that this falls apart in the Rust scaffolding - it needs to know the type - even for external types - to know how to lower etc it.
This feels worse than the proposed syntax, but IMO bindgen without library mode is outdated
Agree entirely - I really would prefer to have found a way to not specify the type, but failed (a few times actually :)
There was a problem hiding this comment.
the problem is that this falls apart in the Rust scaffolding
oops, I should have clarified - we work around that in the rust scaffolding today by assuming a RustBuffer, but that falls apart in #2025.
There was a problem hiding this comment.
I tried for a while to make this work. I made some things work, but I hit a fundamental roadblock: if the user is not using library mode, then we need to generate the bindings directly from the UDL. In that case, there needs to be a way in the UDL to specify the builtin-type, since that's going to determine the FFI type. This proposal seems as good as any other to me.
| ``` | ||
| [Custom="crate_name"] | ||
| typedef string Url; // replace `string` with any appropriate type. | ||
| ``` |
There was a problem hiding this comment.
I tried for a while to make this work. I made some things work, but I hit a fundamental roadblock: if the user is not using library mode, then we need to generate the bindings directly from the UDL. In that case, there needs to be a way in the UDL to specify the builtin-type, since that's going to determine the FFI type. This proposal seems as good as any other to me.
Previously they were declared with the
Externalattribute, but this mean UniFFI could not know the underlying type, leading to issues like #2025.This PR does not however fix any such issues, it instead allows UniFFI metadata to carry enough information to fix in the future. This is primarily being done now to include this breaking change in with all other breaking changes we are making in the next release.