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

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.
mozilla · Jan 1, 2025 · af9bd4c · af9bd4c
1 parent 05778b1
commit af9bd4c
Show file tree

Hide file tree

Showing 7 changed files with 99 additions and 68 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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)
@@ -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<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,
         })
     }
@@ -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))
     }
 }
@@ -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
@@ -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<F>(udl: &str, tester: F)