Use TraitMethodMetadata for callback interface methods

This brings the concepts closer together and should make it easier to
allow trait interfaces to be created from foreign objects (mozilla#1578).

- Refactored `get_or_insert_object()` to just be `get_object()`.
  Ordered the metadata enum so that object definitions always come
  before methods.
- Removed the `Result` return from `add_known_type()`, since it couldn't
  fail anyways.

fixtures/metadata/src/tests.rs

@@ -481,7 +481,6 @@ mod test_function_metadata {
             MethodMetadata {
                 module_path: "uniffi_fixture_metadata".into(),
                 self_name: "Calculator".into(),
-                self_is_trait: false,
                 name: "add".into(),
                 is_async: false,
                 inputs: vec![
@@ -560,7 +559,6 @@ mod test_function_metadata {
             MethodMetadata {
                 module_path: "uniffi_fixture_metadata".into(),
                 self_name: "Calculator".into(),
-                self_is_trait: false,
                 name: "async_sub".into(),
                 is_async: true,
                 inputs: vec![
@@ -588,7 +586,6 @@ mod test_function_metadata {
             MethodMetadata {
                 module_path: "uniffi_fixture_metadata".into(),
                 self_name: "Calculator".into(),
-                self_is_trait: false,
                 name: "get_display".into(),
                 is_async: false,
                 inputs: vec![],
@@ -607,10 +604,10 @@ mod test_function_metadata {
     fn test_trait_method() {
         check_metadata(
             &UNIFFI_META_UNIFFI_FIXTURE_METADATA_METHOD_CALCULATORDISPLAY_DISPLAY_RESULT,
-            MethodMetadata {
+            TraitMethodMetadata {
                 module_path: "uniffi_fixture_metadata".into(),
-                self_name: "CalculatorDisplay".into(),
-                self_is_trait: true,
+                trait_name: "CalculatorDisplay".into(),
+                index: 0,
                 name: "display_result".into(),
                 is_async: false,
                 inputs: vec![
@@ -637,21 +634,20 @@ mod test_function_metadata {
             },
         );
         check_metadata(
-            &UNIFFI_META_UNIFFI_FIXTURE_METADATA_CALLBACK_INTERFACE_METHOD_LOGGER_LOG,
-            CallbackInterfaceMethodMetadata {
+            &UNIFFI_META_UNIFFI_FIXTURE_METADATA_METHOD_LOGGER_LOG,
+            TraitMethodMetadata {
                 module_path: "uniffi_fixture_metadata".into(),
                 trait_name: "Logger".into(),
                 index: 0,
                 name: "log".into(),
+                is_async: false,
                 inputs: vec![FnParamMetadata {
                     name: "message".into(),
                     ty: Type::String,
                 }],
                 return_type: None,
                 throws: None,
-                checksum:
-                    UNIFFI_META_CONST_UNIFFI_FIXTURE_METADATA_CALLBACK_INTERFACE_METHOD_LOGGER_LOG
-                        .checksum(),
+                checksum: UNIFFI_META_CONST_UNIFFI_FIXTURE_METADATA_METHOD_LOGGER_LOG.checksum(),
             },
         );
     }

uniffi_bindgen/src/interface/mod.rs

@@ -50,7 +50,7 @@ use std::{
     iter,
 };
 
-use anyhow::{bail, ensure, Result};
+use anyhow::{anyhow, bail, ensure, Result};
 
 pub mod types;
 pub use types::{AsType, ExternalKind, ObjectImpl, Type};
@@ -74,9 +74,7 @@ pub use record::{Field, Record};
 
 pub mod ffi;
 pub use ffi::{FfiArgument, FfiFunction, FfiType};
-use uniffi_meta::{
-    CallbackInterfaceMethodMetadata, ConstructorMetadata, MethodMetadata, ObjectMetadata,
-};
+use uniffi_meta::{ConstructorMetadata, ObjectMetadata, TraitMethodMetadata};
 
 // This needs to match the minor version of the `uniffi` crate.  See
 // `docs/uniffi-versioning.md` for details.
@@ -123,7 +121,7 @@ impl ComponentInterface {
             bail!("parse error");
         }
         // Unconditionally add the String type, which is used by the panic handling
-        ci.types.add_known_type(&Type::String)?;
+        ci.types.add_known_type(&Type::String);
         // We process the WebIDL definitions in two passes.
         // First, go through and look for all the named types.
         ci.types.add_type_definitions_from(defns.as_slice())?;
@@ -605,7 +603,7 @@ impl ComponentInterface {
             Entry::Vacant(v) => {
                 for variant in defn.variants() {
                     for field in variant.fields() {
-                        self.types.add_known_type(&field.as_type())?;
+                        self.types.add_known_type(&field.as_type());
                     }
                 }
                 v.insert(defn);
@@ -631,7 +629,7 @@ impl ComponentInterface {
         match self.records.entry(defn.name().to_owned()) {
             Entry::Vacant(v) => {
                 for field in defn.fields() {
-                    self.types.add_known_type(&field.as_type())?;
+                    self.types.add_known_type(&field.as_type());
                 }
                 v.insert(defn);
             }
@@ -654,10 +652,10 @@ impl ComponentInterface {
     /// Called by `APIBuilder` impls to add a newly-parsed function definition to the `ComponentInterface`.
     pub(super) fn add_function_definition(&mut self, defn: Function) -> Result<()> {
         for arg in &defn.arguments {
-            self.types.add_known_type(&arg.type_)?;
+            self.types.add_known_type(&arg.type_);
         }
         if let Some(ty) = &defn.return_type {
-            self.types.add_known_type(ty)?;
+            self.types.add_known_type(ty);
         }
 
         // Since functions are not a first-class type, we have to check for duplicates here
@@ -670,54 +668,52 @@ impl ComponentInterface {
         }
         if defn.is_async() {
             // Async functions depend on the foreign executor
-            self.types.add_known_type(&Type::ForeignExecutor)?;
+            self.types.add_known_type(&Type::ForeignExecutor);
         }
         self.functions.push(defn);
 
         Ok(())
     }
 
     pub(super) fn add_constructor_meta(&mut self, meta: ConstructorMetadata) -> Result<()> {
-        let object = get_or_insert_object(&mut self.objects, &meta.self_name, ObjectImpl::Struct);
+        let object = get_object(&mut self.objects, &meta.self_name)
+            .ok_or_else(|| anyhow!("add_constructor_meta: object {} not found", &meta.self_name))?;
         let defn: Constructor = meta.into();
 
         for arg in &defn.arguments {
-            self.types.add_known_type(&arg.type_)?;
+            self.types.add_known_type(&arg.type_);
         }
         object.constructors.push(defn);
 
         Ok(())
     }
 
-    pub(super) fn add_method_meta(&mut self, meta: MethodMetadata) -> Result<()> {
-        let imp = ObjectImpl::from_is_trait(meta.self_is_trait);
-        let object = get_or_insert_object(&mut self.objects, &meta.self_name, imp);
+    pub(super) fn add_method_meta(&mut self, meta: impl Into<Method>) -> Result<()> {
         let defn: Method = meta.into();
+        let object = get_object(&mut self.objects, &defn.object_name)
+            .ok_or_else(|| anyhow!("add_method_meta: object {} not found", &defn.object_name))?;
 
         for arg in &defn.arguments {
-            self.types.add_known_type(&arg.type_)?;
+            self.types.add_known_type(&arg.type_);
         }
         if let Some(ty) = &defn.return_type {
-            self.types.add_known_type(ty)?;
+            self.types.add_known_type(ty);
         }
         if defn.is_async() {
             // Async functions depend on the foreign executor
-            self.types.add_known_type(&Type::ForeignExecutor)?;
+            self.types.add_known_type(&Type::ForeignExecutor);
         }
         object.methods.push(defn);
 
         Ok(())
     }
 
-    pub(super) fn add_object_free_fn(&mut self, meta: ObjectMetadata) -> Result<()> {
-        let imp = ObjectImpl::from_is_trait(meta.is_trait);
-        self.types.add_known_type(&Type::Object {
-            name: meta.name.clone(),
-            imp,
-        })?;
-        let object = get_or_insert_object(&mut self.objects, &meta.name, imp);
-        object.ffi_func_free.name = meta.free_ffi_symbol_name();
-        Ok(())
+    pub(super) fn add_object_meta(&mut self, meta: ObjectMetadata) {
+        let free_name = meta.free_ffi_symbol_name();
+        let mut obj = Object::new(meta.name, ObjectImpl::from_is_trait(meta.is_trait));
+        obj.ffi_func_free.name = free_name;
+        self.types.add_known_type(&obj.as_type());
+        self.add_object_definition(obj);
     }
 
     /// Called by `APIBuilder` impls to add a newly-parsed object definition to the `ComponentInterface`.
@@ -745,26 +741,23 @@ impl ComponentInterface {
         self.callback_interfaces.push(defn);
     }
 
-    pub(super) fn add_callback_interface_method_meta(
-        &mut self,
-        meta: CallbackInterfaceMethodMetadata,
-    ) -> Result<()> {
-        let cb = match get_callback_interface(&mut self.callback_interfaces, &meta.trait_name) {
-            Some(cb) => cb,
-            None => bail!("Callback interface not found: {}", meta.trait_name),
-        };
-        // uniffi_meta should ensure that we process callback interface methods in order, double
-        // check that here
-        if cb.methods.len() != meta.index as usize {
-            bail!(
-                "UniFFI internal error: callback interface method index mismatch for {}::{} (expected {}, saw {})",
-                meta.trait_name,
-                meta.name,
-                cb.methods.len(),
-                meta.index,
-            );
+    pub(super) fn add_trait_method_meta(&mut self, meta: TraitMethodMetadata) -> Result<()> {
+        if let Some(cbi) = get_callback_interface(&mut self.callback_interfaces, &meta.trait_name) {
+            // uniffi_meta should ensure that we process callback interface methods in order, double
+            // check that here
+            if cbi.methods.len() != meta.index as usize {
+                bail!(
+                    "UniFFI internal error: callback interface method index mismatch for {}::{} (expected {}, saw {})",
+                    meta.trait_name,
+                    meta.name,
+                    cbi.methods.len(),
+                    meta.index,
+                );
+            }
+            cbi.methods.push(meta.into());
+        } else {
+            self.add_method_meta(meta)?;
         }
-        cb.methods.push(meta.into());
         Ok(())
     }
 
@@ -840,36 +833,15 @@ impl ComponentInterface {
     }
 }
 
-fn get_or_insert_object<'a>(
-    objects: &'a mut Vec<Object>,
-    name: &str,
-    imp: ObjectImpl,
-) -> &'a mut Object {
-    // The find-based way of writing this currently runs into a borrow checker
-    // error, so we use position
-    match objects.iter_mut().position(|o| o.name == name) {
-        Some(idx) => {
-            // what we created before must match the implementation.
-            assert_eq!(objects[idx].imp, imp);
-            &mut objects[idx]
-        }
-        None => {
-            objects.push(Object::new(name.to_owned(), imp));
-            objects.last_mut().unwrap()
-        }
-    }
+fn get_object<'a>(objects: &'a mut[Object], name: &str) -> Option<&'a mut Object> {
+    objects.iter_mut().find(|o| o.name == name)
 }
 
 fn get_callback_interface<'a>(
     callback_interfaces: &'a mut [CallbackInterface],
     name: &str,
 ) -> Option<&'a mut CallbackInterface> {
-    // The find-based way of writing this currently runs into a borrow checker
-    // error, so we use position
-    callback_interfaces
-        .iter_mut()
-        .position(|o| o.name == name)
-        .map(|idx| &mut callback_interfaces[idx])
+    callback_interfaces.iter_mut().find(|o| o.name == name)
 }
 
 /// Stateful iterator for yielding all types contained in a given type.

uniffi_bindgen/src/interface/object.rs

@@ -272,10 +272,10 @@ impl APIConverter<Object> for weedle::InterfaceDefinition<'_> {
                 // need to add known types as they aren't explicitly referenced in
                 // the UDL
                 if let Some(ref return_type) = return_type {
-                    ci.types.add_known_type(return_type)?;
+                    ci.types.add_known_type(return_type);
                 }
                 for arg in &arguments {
-                    ci.types.add_known_type(&arg.type_)?;
+                    ci.types.add_known_type(&arg.type_);
                 }
                 Ok(Method {
                     // The name is used to create the ffi function for the method.
@@ -653,8 +653,8 @@ impl From<uniffi_meta::MethodMetadata> for Method {
     }
 }
 
-impl From<uniffi_meta::CallbackInterfaceMethodMetadata> for Method {
-    fn from(meta: uniffi_meta::CallbackInterfaceMethodMetadata) -> Self {
+impl From<uniffi_meta::TraitMethodMetadata> for Method {
+    fn from(meta: uniffi_meta::TraitMethodMetadata) -> Self {
         let checksum_fn_name = meta.checksum_symbol_name();
         let return_type = meta.return_type.map(Into::into);
         let arguments = meta.inputs.into_iter().map(Into::into).collect();

uniffi_bindgen/src/interface/types/mod.rs

@@ -307,7 +307,7 @@ impl TypeUniverse {
         if resolve_builtin_type(name).is_some() {
             bail!("please don't shadow builtin types ({name}, {:?})", type_,);
         }
-        self.add_known_type(&type_)?;
+        self.add_known_type(&type_);
         match self.type_definitions.entry(name.to_string()) {
             Entry::Occupied(o) => {
                 let existing_def = o.get();
@@ -345,7 +345,7 @@ impl TypeUniverse {
     }
 
     /// Add a [Type] to the set of all types seen in the component interface.
-    pub fn add_known_type(&mut self, type_: &Type) -> Result<()> {
+    pub fn add_known_type(&mut self, type_: &Type) {
         // Types are more likely to already be known than not, so avoid unnecessary cloning.
         if !self.all_known_types.contains(type_) {
             self.all_known_types.insert(type_.to_owned());
@@ -355,17 +355,15 @@ impl TypeUniverse {
             // this is important if the inner type isn't ever mentioned outside one of these
             // generic builtin types.
             match type_ {
-                Type::Optional(t) => self.add_known_type(t)?,
-                Type::Sequence(t) => self.add_known_type(t)?,
+                Type::Optional(t) => self.add_known_type(t),
+                Type::Sequence(t) => self.add_known_type(t),
                 Type::Map(k, v) => {
-                    self.add_known_type(k)?;
-                    self.add_known_type(v)?;
+                    self.add_known_type(k);
+                    self.add_known_type(v);
                 }
                 _ => {}
             }
         }
-
-        Ok(())
     }
 
     /// Check if a [Type] is present