From e574c6cb3d406c9bc3731f697c110b6ece2e2a8b Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Thu, 11 Apr 2024 21:50:07 -0400 Subject: [PATCH] Python: Fix custom types generating invalid code when there are forward references. Also includes many other custom-type tests, all of which work, and clarifies in the docs about which types can be used with custom types. Fixes #2067 --- CHANGELOG.md | 4 ++ docs/manual/src/udl/custom_types.md | 19 ++++-- .../custom-types/src/custom_types.udl | 4 ++ fixtures/ext-types/custom-types/src/lib.rs | 64 +++++++++++++++++++ .../custom-types/tests/bindings/test_guid.py | 3 + fixtures/ext-types/lib/src/lib.rs | 48 +++++++++++++- .../tests/bindings/test_imported_types.kts | 6 ++ .../lib/tests/bindings/test_imported_types.py | 7 ++ .../tests/bindings/test_imported_types.swift | 7 ++ .../src/bindings/python/gen_python/mod.rs | 25 ++++++++ .../bindings/python/templates/CustomType.py | 6 -- .../src/bindings/python/templates/Types.py | 8 +++ 12 files changed, 190 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index bae49e5bec..41dde16004 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ ## [[UnreleasedUniFFIVersion]] (backend crates: [[UnreleasedBackendVersion]]) - (_[[ReleaseDate]]_) +### What's fixed? +- Python: Fix custom types generating invalid code when there are forward references. + ([#2067](https://github.com/mozilla/uniffi-rs/issues/2067)) + ### What's changed? - The async runtime can be specified for constructors/methods, this will override the runtime specified at the impl block level. diff --git a/docs/manual/src/udl/custom_types.md b/docs/manual/src/udl/custom_types.md index a9eb153ce8..57e93936ad 100644 --- a/docs/manual/src/udl/custom_types.md +++ b/docs/manual/src/udl/custom_types.md @@ -1,8 +1,8 @@ # Custom types Custom types allow you to extend the UniFFI type system to support types from your Rust crate or 3rd -party libraries. This relies on a [builtin](./builtin_types.md) UDL type move data across the -FFI, followed by a conversion to your custom type. +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 in the scaffolding code @@ -12,7 +12,13 @@ Consider the following trivial Rust abstraction for a "handle" which wraps an in pub struct Handle(i64); ``` -You can use this type in your udl by declaring it via a `typedef` with a `Custom` attribute, +In this trivial example, the simplest way to expose this is with a macro. + +``` +uniffi::custom_newtype!(Handle, i64); +``` + +Or you can define this in UDL via a `typedef` with a `Custom` attribute, defining the builtin type that it's based on. ```idl @@ -21,7 +27,12 @@ typedef i64 Handle; ``` For this to work, your Rust code must also implement a special trait named -`UniffiCustomTypeConverter`. This trait is generated by UniFFI and can be found in the generated +`UniffiCustomTypeConverter`. + +An implementation is provided if you used the `uniffi::custom_newtype!()` macro. +But if you use UDL or otherwise need to implement your own: + +This trait is generated by UniFFI and can be found in the generated Rust scaffolding - it is defined as: ```Rust diff --git a/fixtures/ext-types/custom-types/src/custom_types.udl b/fixtures/ext-types/custom-types/src/custom_types.udl index 6bbe476d62..c088229a27 100644 --- a/fixtures/ext-types/custom-types/src/custom_types.udl +++ b/fixtures/ext-types/custom-types/src/custom_types.udl @@ -1,6 +1,10 @@ [Custom] typedef string Guid; +// Wrapping another custom type. +[Custom] +typedef Guid ANestedGuid; + [Error] enum GuidError { "TooShort" diff --git a/fixtures/ext-types/custom-types/src/lib.rs b/fixtures/ext-types/custom-types/src/lib.rs index 19fff7aa93..db89bb68e4 100644 --- a/fixtures/ext-types/custom-types/src/lib.rs +++ b/fixtures/ext-types/custom-types/src/lib.rs @@ -105,4 +105,68 @@ impl UniffiCustomTypeConverter for Guid { } } +pub struct ANestedGuid(pub Guid); + +impl UniffiCustomTypeConverter for ANestedGuid { + type Builtin = Guid; + + // This is a "fixture" rather than an "example", so we are free to do things that don't really + // make sense for real apps. + fn into_custom(val: Self::Builtin) -> uniffi::Result { + Ok(ANestedGuid(val)) + } + + fn from_custom(obj: Self) -> Self::Builtin { + obj.0 + } +} + +#[uniffi::export] +fn get_nested_guid(nguid: Option) -> ANestedGuid { + nguid.unwrap_or_else(|| ANestedGuid(Guid("ANestedGuid".to_string()))) +} + +// Dependent types might come in the "wrong" order - #2067 triggered +// a bug here because of the alphabetical order of these types. +pub struct ANestedOuid(pub Ouid); +uniffi::custom_newtype!(ANestedOuid, Ouid); + +#[uniffi::export] +fn get_nested_ouid(nouid: Option) -> ANestedOuid { + nouid.unwrap_or_else(|| ANestedOuid(Ouid("ANestedOuid".to_string()))) +} + +// And custom types around other objects. +#[derive(uniffi::Object)] +pub struct InnerObject; + +#[uniffi::export] +impl InnerObject { + #[uniffi::constructor] + fn new() -> Self { + Self + } +} + +pub struct NestedObject(pub std::sync::Arc); +uniffi::custom_newtype!(NestedObject, std::sync::Arc); + +#[uniffi::export] +pub fn get_nested_object(n: NestedObject) -> NestedObject { + n +} + +#[derive(uniffi::Record)] +pub struct InnerRecord { + i: i32, +} + +pub struct NestedRecord(pub InnerRecord); +uniffi::custom_newtype!(NestedRecord, InnerRecord); + +#[uniffi::export] +pub fn get_nested_record(n: NestedRecord) -> NestedRecord { + n +} + uniffi::include_scaffolding!("custom_types"); diff --git a/fixtures/ext-types/custom-types/tests/bindings/test_guid.py b/fixtures/ext-types/custom-types/tests/bindings/test_guid.py index 20aed22d08..0a4b5171fb 100644 --- a/fixtures/ext-types/custom-types/tests/bindings/test_guid.py +++ b/fixtures/ext-types/custom-types/tests/bindings/test_guid.py @@ -55,5 +55,8 @@ def test_guid_callback(self): self.assertEqual(guid, "callback-test-payload") self.assertEqual(test_callback.saw_guid, "callback-test-payload") + def test_custom(self): + get_nested_object(InnerObject()) + if __name__=='__main__': unittest.main() diff --git a/fixtures/ext-types/lib/src/lib.rs b/fixtures/ext-types/lib/src/lib.rs index 6849d230e6..63fdac77ae 100644 --- a/fixtures/ext-types/lib/src/lib.rs +++ b/fixtures/ext-types/lib/src/lib.rs @@ -1,5 +1,5 @@ use custom_types::Handle; -use ext_types_custom::Guid; +use ext_types_custom::{ANestedGuid, Guid, Ouid}; use ext_types_external_crate::{ ExternalCrateDictionary, ExternalCrateInterface, ExternalCrateNonExhaustiveEnum, }; @@ -11,6 +11,18 @@ use uniffi_one::{ use uniffi_sublib::SubLibType; use url::Url; +// #1988 +uniffi::ffi_converter_forward!( + ext_types_custom::Ouid, + ext_types_custom::UniFfiTag, + crate::UniFfiTag +); +uniffi::ffi_converter_forward!( + ext_types_custom::ANestedGuid, + ext_types_custom::UniFfiTag, + crate::UniFfiTag +); + pub struct CombinedType { pub uoe: UniffiOneEnum, pub uot: UniffiOneType, @@ -100,6 +112,40 @@ fn get_maybe_urls(urls: Vec>) -> Vec> { urls } +// XXX - #1854 +// fn get_imported_guid(guid: Guid) -> Guid { + +#[uniffi::export] +fn get_imported_ouid(ouid: Ouid) -> Ouid { + ouid +} + +// external custom types wrapping external custom types. +#[uniffi::export] +fn get_imported_nested_guid(guid: Option) -> ANestedGuid { + guid.unwrap_or_else(|| ANestedGuid(Guid("nested".to_string()))) +} + +#[uniffi::export] +fn get_imported_nested_ouid(guid: Option) -> ANestedGuid { + guid.unwrap_or_else(|| ANestedGuid(Guid("nested".to_string()))) +} + +// A local custom type wrapping an external imported UDL type +// XXX - #1854 +// pub struct NestedExternalGuid(pub Guid); +// ... +// fn get_nested_external_guid(nguid: Option) -> NestedExternalGuid { + +// A local custom type wrapping an external imported procmacro type +pub struct NestedExternalOuid(pub Ouid); +uniffi::custom_newtype!(NestedExternalOuid, Ouid); + +#[uniffi::export] +fn get_nested_external_ouid(ouid: Option) -> NestedExternalOuid { + ouid.unwrap_or_else(|| NestedExternalOuid(Ouid("nested-external-ouid".to_string()))) +} + // A struct fn get_uniffi_one_type(t: UniffiOneType) -> UniffiOneType { t diff --git a/fixtures/ext-types/lib/tests/bindings/test_imported_types.kts b/fixtures/ext-types/lib/tests/bindings/test_imported_types.kts index 87e1dd04f2..2a66371408 100644 --- a/fixtures/ext-types/lib/tests/bindings/test_imported_types.kts +++ b/fixtures/ext-types/lib/tests/bindings/test_imported_types.kts @@ -5,6 +5,7 @@ import uniffi.imported_types_lib.* import uniffi.imported_types_sublib.* import uniffi.uniffi_one_ns.* +import uniffi.ext_types_custom.* val ct = getCombinedType(null) assert(ct.uot.sval == "hello") @@ -28,6 +29,11 @@ assert(getMaybeUrl(null) == null) assert(getUrls(listOf(url)) == listOf(url)) assert(getMaybeUrls(listOf(url, null)) == listOf(url, null)) +assert(getGuid("guid") == "guid") +assert(getUuid("uuid") == "uuid") +//assert(getImportedGuid("guid") == "guid") +assert(getImportedUuid("uuid") == "uuid") + val uot = UniffiOneType("hello") assert(getUniffiOneType(uot) == uot) assert(getMaybeUniffiOneType(uot)!! == uot) diff --git a/fixtures/ext-types/lib/tests/bindings/test_imported_types.py b/fixtures/ext-types/lib/tests/bindings/test_imported_types.py index 4c50ff39d2..2fab8f2d16 100644 --- a/fixtures/ext-types/lib/tests/bindings/test_imported_types.py +++ b/fixtures/ext-types/lib/tests/bindings/test_imported_types.py @@ -7,6 +7,7 @@ from imported_types_lib import * from imported_types_sublib import * from uniffi_one_ns import * +from ext_types_custom import * class TestIt(unittest.TestCase): def test_it(self): @@ -40,6 +41,12 @@ def test_get_url(self): self.assertEqual(get_maybe_url(None), None) self.assertEqual(get_maybe_urls([url, None]), [url, None]) + def test_custom_types(self): + self.assertEqual(get_guid("guid"), "guid") + self.assertEqual(get_ouid("ouid"), "ouid") + self.assertEqual(get_ouid("uuid"), "uuid") + self.assertEqual(get_nested_guid("uuid"), "uuid") + def test_get_uniffi_one_type(self): t1 = UniffiOneType(sval="hello") self.assertEqual(t1, get_uniffi_one_type(t1)) diff --git a/fixtures/ext-types/lib/tests/bindings/test_imported_types.swift b/fixtures/ext-types/lib/tests/bindings/test_imported_types.swift index 4acbf83025..ab5de91c8a 100644 --- a/fixtures/ext-types/lib/tests/bindings/test_imported_types.swift +++ b/fixtures/ext-types/lib/tests/bindings/test_imported_types.swift @@ -30,6 +30,13 @@ assert(getMaybeUrl(url: nil) == nil) assert(getUrls(urls: [url]) == [url]) assert(getMaybeUrls(urls: [url, nil]) == [url, nil]) +assert(getGuid(value: "guid") == "guid") +assert(getOuid(ouid: "ouid") == "ouid") +assert(getImportedOuid(ouid: "ouid") == "ouid") +assert(getNestedOuid(nouid: "ouid") == "ouid") +assert(getImportedNestedGuid(guid: nil) == "nested") +assert(getNestedExternalOuid(ouid: nil) == "nested-external-ouid") + assert(getUniffiOneType(t: UniffiOneType(sval: "hello")).sval == "hello") assert(getMaybeUniffiOneType(t: UniffiOneType(sval: "hello"))!.sval == "hello") assert(getMaybeUniffiOneType(t: nil) == nil) diff --git a/uniffi_bindgen/src/bindings/python/gen_python/mod.rs b/uniffi_bindgen/src/bindings/python/gen_python/mod.rs index 6a10a38e7f..679641e2d7 100644 --- a/uniffi_bindgen/src/bindings/python/gen_python/mod.rs +++ b/uniffi_bindgen/src/bindings/python/gen_python/mod.rs @@ -298,6 +298,31 @@ impl<'a> TypeRenderer<'a> { }); "" } + + // An inefficient algo to return type aliases needed for custom types + // in an order such that dependencies are in the correct order. + // Eg, if there's a custom type `Guid` -> `str` and another `GuidWrapper` -> `Guid`, + // it's important the type alias for `Guid` appears first. + fn get_custom_type_aliases(&self) -> Vec<(String, &Type)> { + let mut ordered = vec![]; + for type_ in self.ci.iter_types() { + if let Type::Custom { name, builtin, .. } = type_ { + match ordered + .iter() + .position(|x: &(&str, &Type)| *name == x.1.as_codetype().type_label()) + { + // This 'name' appears as a builtin, so we must insert our type first. + Some(pos) => ordered.insert(pos, (name, builtin)), + // Otherwise at the end. + None => ordered.push((name, builtin)), + } + } + } + ordered + .into_iter() + .map(|(n, t)| (PythonCodeOracle.class_name(n), t)) + .collect() + } } #[derive(Template)] diff --git a/uniffi_bindgen/src/bindings/python/templates/CustomType.py b/uniffi_bindgen/src/bindings/python/templates/CustomType.py index f75a85dc27..898ecb28f2 100644 --- a/uniffi_bindgen/src/bindings/python/templates/CustomType.py +++ b/uniffi_bindgen/src/bindings/python/templates/CustomType.py @@ -1,9 +1,6 @@ {%- match python_config.custom_types.get(name.as_str()) %} {% when None %} {#- No custom type config, just forward all methods to our builtin type #} -# Type alias -{{ name }} = {{ builtin|type_name }} - class _UniffiConverterType{{ name }}: @staticmethod def write(value, buf): @@ -35,9 +32,6 @@ def lower(value): {%- else %} {%- endmatch %} -# Type alias -{{ name }} = {{ builtin|type_name }} - {#- Custom type config supplied, use it to convert the builtin type #} class _UniffiConverterType{{ name }}: @staticmethod diff --git a/uniffi_bindgen/src/bindings/python/templates/Types.py b/uniffi_bindgen/src/bindings/python/templates/Types.py index 4aaed253e0..29f66fe886 100644 --- a/uniffi_bindgen/src/bindings/python/templates/Types.py +++ b/uniffi_bindgen/src/bindings/python/templates/Types.py @@ -97,3 +97,11 @@ {%- else %} {%- endmatch %} {%- endfor %} + +{#- +Setup type aliases for our custom types, has complications due to +forward type references, #2067 +-#} +{%- for (name, ty) in self.get_custom_type_aliases() %} +{{ name }} = {{ ty|type_name }} +{%- endfor %}