Python: Fix custom types generating invalid code when there are forward references

CHANGELOG.md

@@ -12,6 +12,9 @@
   did not already start with a capital letter, but this changes makes all type naming consistent.
   ([#2073](https://github.com/mozilla/uniffi-rs/issues/2073))
 
+- 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.
 

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

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"

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<Self> {
+        Ok(ANestedGuid(val))
+    }
+
+    fn from_custom(obj: Self) -> Self::Builtin {
+        obj.0
+    }
+}
+
+#[uniffi::export]
+fn get_nested_guid(nguid: Option<ANestedGuid>) -> 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>) -> 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<InnerObject>);
+uniffi::custom_newtype!(NestedObject, std::sync::Arc<InnerObject>);
+
+#[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");

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

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<Option<Url>>) -> Vec<Option<Url>> {
     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>) -> ANestedGuid {
+    guid.unwrap_or_else(|| ANestedGuid(Guid("nested".to_string())))
+}
+
+#[uniffi::export]
+fn get_imported_nested_ouid(guid: Option<ANestedGuid>) -> 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>) -> 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>) -> NestedExternalOuid {
+    ouid.unwrap_or_else(|| NestedExternalOuid(Ouid("nested-external-ouid".to_string())))
+}
+
 // A struct
 fn get_uniffi_one_type(t: UniffiOneType) -> UniffiOneType {
     t

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(getOuid("ouid") == "ouid")
+//assert(getImportedGuid("guid") == "guid")
+assert(getImportedOuid("ouid") == "ouid")
+
 val uot = UniffiOneType("hello")
 assert(getUniffiOneType(uot) == uot)
 assert(getMaybeUniffiOneType(uot)!! == uot)

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

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)

uniffi_bindgen/src/bindings/python/gen_python/mod.rs

@@ -298,6 +298,33 @@ 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. Fails to handle
+    // another level of indirection (eg, `A { builtin: C}, B { }, C { builtin: B })`)
+    // but that's pathological :)
+    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)]

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
-{{ type_name }} = {{ builtin|type_name }}
-
 class _UniffiConverterType{{ name }}:
     @staticmethod
     def write(value, buf):
@@ -35,9 +32,6 @@ def lower(value):
 {%- else %}
 {%- endmatch %}
 
-# Type alias
-{{ type_name }} = {{ builtin|type_name }}
-
 {#- Custom type config supplied, use it to convert the builtin type #}
 class _UniffiConverterType{{ name }}:
     @staticmethod

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 %}