From 25979483eecbfd1074e18a8db8e6737df81de25a Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Fri, 27 Dec 2024 19:59:19 -0500 Subject: [PATCH] External custom types in UDL must use the [Custom] attribute Previously they were declared with the `External` attribute, 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. --- CHANGELOG.md | 13 +++-- docs/manual/src/Upgrading.md | 29 +++++++--- docs/manual/src/types/custom_types.md | 16 ++---- fixtures/ext-types/lib/src/ext-types-lib.udl | 12 ++-- .../uitests/tests/ui/typedef_extern.stderr | 9 ++- uniffi_udl/src/attributes.rs | 31 +++++++--- uniffi_udl/src/finder.rs | 57 +++++++++---------- 7 files changed, 99 insertions(+), 68 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index af4e5ec55..2837bcc65 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/docs/manual/src/Upgrading.md b/docs/manual/src/Upgrading.md index f8ca2952c..259a5ad4e 100644 --- a/docs/manual/src/Upgrading.md +++ b/docs/manual/src/Upgrading.md @@ -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: @@ -29,7 +28,7 @@ impl UniffiCustomTypeConverter for NewCustomType { After: -``` +```rust uniffi::custom_type!(NewCustomType, BridgeType, { try_lift: |val| { Ok(...) }, lower: |obj| { ... }, @@ -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: @@ -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. +``` + ## Remote Types The macros `ffi_converter_forward` and all `use_*` macros (eg, `use_udl_record!`, `use_udl_object!`, `use_udl_enum!` etc) @@ -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; ``` diff --git a/docs/manual/src/types/custom_types.md b/docs/manual/src/types/custom_types.md index 02af134e2..1b884a5e8 100644 --- a/docs/manual/src/types/custom_types.md +++ b/docs/manual/src/types/custom_types.md @@ -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. diff --git a/fixtures/ext-types/lib/src/ext-types-lib.udl b/fixtures/ext-types/lib/src/ext-types-lib.udl index cebc531f2..c315a8a36 100644 --- a/fixtures/ext-types/lib/src/ext-types-lib.udl +++ b/fixtures/ext-types/lib/src/ext-types-lib.udl @@ -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. diff --git a/fixtures/uitests/tests/ui/typedef_extern.stderr b/fixtures/uitests/tests/ui/typedef_extern.stderr index 2af3f4d1d..2226a5188 100644 --- a/fixtures/uitests/tests/ui/typedef_extern.stderr +++ b/fixtures/uitests/tests/ui/typedef_extern.stderr @@ -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"] @@ -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"); diff --git a/uniffi_udl/src/attributes.rs b/uniffi_udl/src/attributes.rs index 133dea648..dc617ab47 100644 --- a/uniffi_udl/src/attributes.rs +++ b/uniffi_udl/src/attributes.rs @@ -35,7 +35,7 @@ pub(super) enum Attribute { External { crate_name: String }, Remote, // Custom type on the scaffolding side - Custom, + Custom { crate_name: Option }, // The interface described is implemented as a trait. Trait, // Modifies `Trait` to enable foreign implementations (callback interfaces) @@ -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), @@ -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 @@ -547,6 +550,10 @@ impl TypedefAttributes { pub(super) fn get_crate_name(&self) -> Option { 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, }) } @@ -564,9 +571,12 @@ impl TryFrom<&weedle::attribute::ExtendedAttributeList<'_>> for TypedefAttribute weedle_attributes: &weedle::attribute::ExtendedAttributeList<'_>, ) -> Result { 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)) } } @@ -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(); @@ -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(); diff --git a/uniffi_udl/src/finder.rs b/uniffi_udl/src/finder.rs index 559992460..1bf4b54c9 100644 --- a/uniffi_udl/src/finder.rs +++ b/uniffi_udl/src/finder.rs @@ -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. @@ -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"] @@ -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. /// @@ -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( @@ -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, @@ -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, @@ -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) } } @@ -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(udl: &str, tester: F)