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

External custom types in UDL must use the [Custom] attribute #2371

Merged
merged 1 commit into from
Jan 1, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,19 @@
We've made a number of breaking changes to fix long standing paper-cuts with UniFFI in a
multi-crate environment and to simplify your and our implementations.

While **no changes required to foreign code**, we apologize for the inconvenience!
[See the detailed upgrade notes](https://mozilla.github.io/uniffi-rs/next/Upgrading.html)

You are impacted if you use "Custom types", or use UDL with types from more than one crate.
We have [detailed upgrade notes](https://mozilla.github.io/uniffi-rs/next/Upgrading.html)
While **no changes are required to foreign code**, we apologize for the inconvenience!

- "Custom Types" have changed, all implementations will need to update their Rust code.
The `UniffiCustomTypeConverter` trait is no longer used, use the
You are impacted if you use `UniffiCustomTypeConverter` to implement "Custom types",
or use UDL with types from more than one crate.

- `UniffiCustomTypeConverter` has been removed, you must now use the
[`custom_type!` macro](https://mozilla.github.io/uniffi-rs/next/types/custom_types.html) instead.

- The [UDL syntax for external types](https://mozilla.github.io/uniffi-rs/next/udl/external_types.html) has changed.
`typedef extern MyEnum;` has been replaced
with `typedef enum MyEnum;`. Attributes other than `[External = "crate_name"]` have been removed.
with `typedef enum MyEnum;`. `[Custom]` and `[External]` are the only supported attributes for a `typedef`.

- "remote" types (where UDL can re-export a type defined in
a non-UniFFI crate - eg, `log::Level`) must now use a
Expand Down
29 changes: 21 additions & 8 deletions docs/manual/src/Upgrading.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,12 @@
We've made a number of breaking changes in this release, particularly
to:

* Custom types (both UDL and proc-macros impacted)
* External Types (UDL impacted)
* Custom types: UniffiCustomTypeConverter has been removed.
* External types: `extern` has been removed; you must describe the type.

## Custom types

Custom types are now implemented using a macro rather than implementing the `UniffiCustomTypeConverter` trait,
addressing some edge-cases with custom types wrapping types from other crates (eg, Url).
Custom types still implemented via the `UniffiCustomTypeConverter` trait must move to proc-macros.

Before:

Expand All @@ -29,7 +28,7 @@ impl UniffiCustomTypeConverter for NewCustomType {

After:

```
```rust
uniffi::custom_type!(NewCustomType, BridgeType, {
try_lift: |val| { Ok(...) },
lower: |obj| { ... },
Expand All @@ -46,12 +45,12 @@ External types can no longer be described in UDL via `extern` - instead, you mus
For example:
```
[External="crate_name"]
typedef extern MyEnum
typedef extern MyEnum;
```
is no longer accepted - you must use, eg:
```
[External="crate_name"]
typedef enum MyEnum
typedef enum MyEnum;
```

Edge-cases broken include:
Expand All @@ -61,6 +60,20 @@ Edge-cases broken include:

See [Remote and External Types](./types/remote_ext_types.md) for more detail.

## External Custom Types

Previously you could describe an external Custom Type `Url` in UDL as:
```
[External="crate_name"]
typedef extern Url;
```

But now you must use:
```
[Custom="crate_name"]
typedef string Url; // replace `string` with any appropriate type.
```
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


## Remote Types

The macros `ffi_converter_forward` and all `use_*` macros (eg, `use_udl_record!`, `use_udl_object!`, `use_udl_enum!` etc)
Expand All @@ -76,7 +89,7 @@ The `Rust` attribute has been removed - use the same typedef syntax described ab
[Rust="record"]
typedef extern One;
```
becomes
becomes a `typedef` with no attributes
```
typedef record One;
```
16 changes: 6 additions & 10 deletions docs/manual/src/types/custom_types.md
Original file line number Diff line number Diff line change
Expand Up @@ -87,22 +87,18 @@ followed by the custom type.
typedef i64 Handle;
```

**note**: you must still call the `custom_type!` or `custom_newtype!` macros in your Rust code, as described above.


#### Using custom types from other crates

To use custom types from other crates from UDL, use a typedef wrapped with the `[External]` attribute.

For example, if another crate wanted to use the examples here:
You can specify the crate name if the custom type implementation is external.

```idl
[External="crate_defining_handle_name"]
[Custom="crate_defining_handle_name"]
typedef i64 Handle;

[External="crate_defining_log_record_name"]
[Custom="crate_defining_log_record_name"]
typedef dictionary LogRecord;
```

**note**: you must still call the `custom_type!` or `custom_newtype!` macros in your Rust code, as described above.

## User-defined types

All examples so far in this section convert the custom type to a builtin type.
Expand Down
12 changes: 6 additions & 6 deletions fixtures/ext-types/lib/src/ext-types-lib.udl
Original file line number Diff line number Diff line change
Expand Up @@ -44,15 +44,15 @@ typedef trait UniffiOneUDLTrait;
typedef dictionary UniffiOneProcMacroType;

// A Custom (ie, "wrapped") type defined externally in `../../custom-types/src/lib.rs`,
[External="ext_types_custom"]
typedef custom Guid;
[Custom="ext_types_custom"]
typedef string Guid;

// And re-use the `custom-types` example - this exposes `Url` and `Handle`
[External="custom_types"]
typedef custom Url;
[Custom="custom_types"]
typedef string Url;

[External="custom_types"]
typedef custom Handle;
[Custom="custom_types"]
typedef i64 Handle;

// Here are some different kinds of remote types - the types are described
// in this UDL, but the types themselves are defined in a different crate.
Expand Down
9 changes: 8 additions & 1 deletion fixtures/uitests/tests/ui/typedef_extern.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ error: Failed to generate scaffolding from UDL file at ../../../../fixtures/uite
* "record", "dictionary" or "struct" for Records.
* "object", "impl" or "interface" for objects.
* "trait", "callback" or "trait_with_foreign" for traits.
* "custom" for Custom Types.

For example:
[External="crate_name"]
Expand All @@ -15,6 +14,14 @@ error: Failed to generate scaffolding from UDL file at ../../../../fixtures/uite
typedef enum ExternalEnum;

See https://mozilla.github.io/uniffi-rs/next/types/remote_ext_types.html for more.

External Custom Types must be declared in the same way, but with
[Custom="crate_name"] instead of [Extern="crate_name"]

[Custom="crate_name"]
typedef string Url;

See https://mozilla.github.io/uniffi-rs/next/types/custom_types.html for more.
--> tests/ui/typedef_extern.rs:2:1
|
2 | uniffi_macros::generate_and_include_scaffolding!("../../../../fixtures/uitests/src/typedef_extern.udl");
Expand Down
31 changes: 23 additions & 8 deletions uniffi_udl/src/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ pub(super) enum Attribute {
External { crate_name: String },
Remote,
// Custom type on the scaffolding side
Custom,
Custom { crate_name: Option<String> },
// The interface described is implemented as a trait.
Trait,
// Modifies `Trait` to enable foreign implementations (callback interfaces)
Expand Down Expand Up @@ -66,7 +66,7 @@ impl TryFrom<&weedle::attribute::ExtendedAttribute<'_>> for Attribute {
"ByRef" => Ok(Attribute::ByRef),
"Enum" => Ok(Attribute::Enum),
"Error" => Ok(Attribute::Error),
"Custom" => Ok(Attribute::Custom),
"Custom" => Ok(Attribute::Custom { crate_name: None }),
"Trait" => Ok(Attribute::Trait),
"WithForeign" => Ok(Attribute::WithForeign),
"Async" => Ok(Attribute::Async),
Expand All @@ -83,6 +83,9 @@ impl TryFrom<&weedle::attribute::ExtendedAttribute<'_>> for Attribute {
"External" => Ok(Attribute::External {
crate_name: name_from_id_or_string(&identity.rhs),
}),
"Custom" => Ok(Attribute::Custom {
crate_name: Some(name_from_id_or_string(&identity.rhs)),
}),
_ => anyhow::bail!(
"Attribute identity Identifier not supported: {:?}",
identity.lhs_identifier.0
Expand Down Expand Up @@ -547,6 +550,10 @@ impl TypedefAttributes {
pub(super) fn get_crate_name(&self) -> Option<String> {
self.0.iter().find_map(|attr| match attr {
Attribute::External { crate_name, .. } => Some(crate_name.clone()),
Attribute::Custom {
crate_name: Some(crate_name),
..
} => Some(crate_name.clone()),
_ => None,
})
}
Expand All @@ -564,9 +571,12 @@ impl TryFrom<&weedle::attribute::ExtendedAttributeList<'_>> for TypedefAttribute
weedle_attributes: &weedle::attribute::ExtendedAttributeList<'_>,
) -> Result<Self, Self::Error> {
let attrs = parse_attributes(weedle_attributes, |attr| match attr {
Attribute::External { .. } | Attribute::Custom => Ok(()),
Attribute::External { .. } | Attribute::Custom { .. } => Ok(()),
_ => bail!(format!("{attr:?} not supported for typedefs")),
})?;
if attrs.len() > 1 {
bail!("Can't be [Custom] and [External]");
}
Ok(Self(attrs))
}
}
Expand Down Expand Up @@ -933,6 +943,13 @@ mod test {
let (_, node) = weedle::attribute::ExtendedAttributeList::parse("[Custom]").unwrap();
let attrs = TypedefAttributes::try_from(&node).unwrap();
assert!(attrs.is_custom());
assert!(attrs.get_crate_name().is_none());

let (_, node) =
weedle::attribute::ExtendedAttributeList::parse("[Custom=\"crate_name\"]").unwrap();
let attrs = TypedefAttributes::try_from(&node).unwrap();
assert!(attrs.is_custom());
assert_eq!(attrs.get_crate_name().unwrap(), "crate_name");

let (_, node) =
weedle::attribute::ExtendedAttributeList::parse("[External=crate_name]").unwrap();
Expand All @@ -943,12 +960,10 @@ mod test {

#[test]
fn test_typedef_attributes_malformed() {
let (_, node) = weedle::attribute::ExtendedAttributeList::parse("[Custom=foo]").unwrap();
let (_, node) =
weedle::attribute::ExtendedAttributeList::parse("[External=foo, Custom]").unwrap();
let err = TypedefAttributes::try_from(&node).unwrap_err();
assert_eq!(
err.to_string(),
"Attribute identity Identifier not supported: \"Custom\""
);
assert_eq!(err.to_string(), "Can't be [Custom] and [External]");

let (_, node) = weedle::attribute::ExtendedAttributeList::parse("[External]").unwrap();
let err = TypedefAttributes::try_from(&node).unwrap_err();
Expand Down
57 changes: 28 additions & 29 deletions uniffi_udl/src/finder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ use anyhow::{bail, Result};

use super::TypeCollector;
use crate::attributes::{InterfaceAttributes, TypedefAttributes};
use uniffi_meta::{ExternalKind, ObjectImpl, Type};
use uniffi_meta::{ObjectImpl, Type};

// We broke this, so try and be as helpful as possible.
static ERR_TYPEDEF_EXTERN: &str = r#"`typedef extern` is no longer supported.
Expand All @@ -32,7 +32,6 @@ You must replace `extern` with the type of the object:
* "record", "dictionary" or "struct" for Records.
* "object", "impl" or "interface" for objects.
* "trait", "callback" or "trait_with_foreign" for traits.
* "custom" for Custom Types.

For example:
[External="crate_name"]
Expand All @@ -42,7 +41,15 @@ Would be replaced with:
[External="crate_name"]
typedef enum ExternalEnum;

See https://mozilla.github.io/uniffi-rs/next/types/remote_ext_types.html for more."#;
See https://mozilla.github.io/uniffi-rs/next/types/remote_ext_types.html for more.

External Custom Types must be declared in the same way, but with
[Custom="crate_name"] instead of [Extern="crate_name"]

[Custom="crate_name"]
typedef string Url;

See https://mozilla.github.io/uniffi-rs/next/types/custom_types.html for more."#;

/// Trait to help with an early "type discovery" phase when processing the UDL.
///
Expand Down Expand Up @@ -131,18 +138,18 @@ impl TypeFinder for weedle::EnumDefinition<'_> {
impl TypeFinder for weedle::TypedefDefinition<'_> {
fn add_type_definitions_to(&self, types: &mut TypeCollector) -> Result<()> {
let attrs = TypedefAttributes::try_from(self.attributes.as_ref())?;
if attrs.is_custom() {
// A local type which wraps a builtin and for which we will generate an
// `FfiConverter` implementation.
let ty = if attrs.is_custom() {
// A type which wraps a builtin with an `FfiConverter` implementation.
// If local we will generate that implementation, if external we will reference it.
let builtin = types.resolve_type_expression(&self.type_)?;
types.add_type_definition(
self.identifier.0,
Type::Custom {
module_path: types.module_path(),
name: self.identifier.0.to_string(),
builtin: builtin.into(),
},
)
let module_path = attrs
.get_crate_name()
.unwrap_or_else(|| types.module_path());
Type::Custom {
module_path,
name: self.identifier.0.to_string(),
builtin: builtin.into(),
}
} else {
let typedef_type = match &self.type_.type_ {
weedle::types::Type::Single(weedle::types::SingleType::NonAny(
Expand All @@ -157,7 +164,7 @@ impl TypeFinder for weedle::TypedefDefinition<'_> {
let module_path = attrs
.get_crate_name()
.unwrap_or_else(|| types.module_path());
let ty = match typedef_type {
match typedef_type {
"dictionary" | "record" | "struct" => Type::Record {
module_path,
name,
Expand All @@ -166,13 +173,6 @@ impl TypeFinder for weedle::TypedefDefinition<'_> {
module_path,
name,
},
// last Type::External holdback - we don't know the builtin!
"custom" => Type::External {
namespace: "".to_string(), // we don't know this yet
module_path,
name,
kind: ExternalKind::DataClass,
},
"interface" | "impl" => Type::Object {
module_path,
name,
Expand All @@ -191,12 +191,11 @@ impl TypeFinder for weedle::TypedefDefinition<'_> {
// "extern" gets a special error to help upgrading
"extern" => bail!(ERR_TYPEDEF_EXTERN),
_ => bail!("Can't work out the type - no attributes and unknown extern type '{typedef_type}'"),
};
// mangle external types - Type::External must die.
let ty =
uniffi_meta::convert_external_type(ty, &types.module_path(), &|s| s.to_string());
types.add_type_definition(self.identifier.0, ty)
}
}
};
// mangle external types - Type::External must die.
let ty = uniffi_meta::convert_external_type(ty, &types.module_path(), &|s| s.to_string());
types.add_type_definition(self.identifier.0, ty)
}
}

Expand All @@ -219,7 +218,7 @@ impl TypeFinder for weedle::CallbackInterfaceDefinition<'_> {
#[cfg(test)]
mod test {
use super::*;
use uniffi_meta::ObjectImpl;
use uniffi_meta::{ExternalKind, ObjectImpl};

// A helper to take valid UDL and a closure to check what's in it.
fn test_a_finding<F>(udl: &str, tester: F)
Expand Down