Fix proc_macro defined errors when they aren't used as errors. (#2119)

When a `#[derive(uniffi::Error)]` didn't appear in any function signatures,
the FfiConverter for the item was written to assume an error, but the
bindings FfiConverter treated the item as a plain-old Enum. This caused
runtime errors when trying to unpack the rustbuffers due to the
disagreement about the buffer format.

As part of fixing this, the `Option<bool>` to try and represent the
flatness of Enums/Errors was replaced with an `EnumShape` enum to
make these states clearer and so the ComponentInterface can better
understand the layout.

Fixes #2108

CHANGELOG.md

@@ -15,6 +15,9 @@
 - Procmacros support tuple-errors (ie, enums used as errors can be tuple-enums.)
 
 ### What's fixed?
+- Fixed a problem with procmacro defined errors when the error was not used as an `Err` result
+  in the namespace ([#2108](https://github.com/mozilla/uniffi-rs/issues/2108))
+
 - Custom Type names are now treated as type names by all bindings. This means if they will work if they happen to be
   keywords in the language. There's a very small risk of this being a breaking change if you used a type name which
   did not already start with a capital letter, but this changes makes all type naming consistent.

fixtures/error-types/src/lib.rs

@@ -149,6 +149,31 @@ fn return_proc_error(e: String) -> Arc<ProcErrorInterface> {
     Arc::new(ProcErrorInterface { e })
 }
 
+#[derive(thiserror::Error, Debug)]
+#[error("NonUniffiTypeValue: {v}")]
+pub struct NonUniffiType {
+    v: String,
+}
+
+// Note: It's important for this test that this error
+// *not* be used directly as the `Err` for any functions etc.
+#[derive(thiserror::Error, uniffi::Error, Debug)]
+pub enum Inner {
+    #[error("{0}")]
+    CaseA(String),
+}
+
+// Note: It's important for this test that this error
+// *not* be used directly as the `Err` for any functions etc.
+#[derive(thiserror::Error, uniffi::Error, Debug)]
+#[uniffi(flat_error)]
+pub enum FlatInner {
+    #[error("{0}")]
+    CaseA(String),
+    #[error("{0}")]
+    CaseB(NonUniffiType),
+}
+
 // Enums have good coverage elsewhere, but simple coverage here is good.
 #[derive(thiserror::Error, uniffi::Error, Debug)]
 pub enum Error {
@@ -158,6 +183,13 @@ pub enum Error {
     Value { value: String },
     #[error("IntValue: {value}")]
     IntValue { value: u16 },
+    #[error(transparent)]
+    FlatInnerError {
+        #[from]
+        error: FlatInner,
+    },
+    #[error(transparent)]
+    InnerError { error: Inner },
 }
 
 #[uniffi::export]
@@ -170,6 +202,20 @@ fn oops_enum(i: u16) -> Result<(), Error> {
         })
     } else if i == 2 {
         Err(Error::IntValue { value: i })
+    } else if i == 3 {
+        Err(Error::FlatInnerError {
+            error: FlatInner::CaseA("inner".to_string()),
+        })
+    } else if i == 4 {
+        Err(Error::FlatInnerError {
+            error: FlatInner::CaseB(NonUniffiType {
+                v: "value".to_string(),
+            }),
+        })
+    } else if i == 5 {
+        Err(Error::InnerError {
+            error: Inner::CaseA("inner".to_string()),
+        })
     } else {
         panic!("unknown variant {i}")
     }

fixtures/error-types/tests/bindings/test.kts

@@ -68,6 +68,27 @@ try {
     assert(e.toString() == "uniffi.error_types.Exception${'$'}IntValue: value=2")
 }
 
+try {
+    oopsEnum(3u)
+    throw RuntimeException("Should have failed")
+} catch (e: Exception.FlatInnerException) {
+    assert(e.toString() == "uniffi.error_types.Exception\$FlatInnerException: error=uniffi.error_types.FlatInner${'$'}CaseA: inner")
+}
+
+try {
+    oopsEnum(4u)
+    throw RuntimeException("Should have failed")
+} catch (e: Exception.FlatInnerException) {
+    assert(e.toString() == "uniffi.error_types.Exception\$FlatInnerException: error=uniffi.error_types.FlatInner${'$'}CaseB: NonUniffiTypeValue: value")
+}
+
+try {
+    oopsEnum(5u)
+    throw RuntimeException("Should have failed")
+} catch (e: Exception.InnerException) {
+    assert(e.toString() == "uniffi.error_types.Exception\$InnerException: error=uniffi.error_types.Inner${'$'}CaseA: v1=inner")
+}
+
 try {
     oopsTuple(0u)
     throw RuntimeException("Should have failed")

fixtures/error-types/tests/bindings/test.py

@@ -99,6 +99,23 @@ def test_enum_error_int_value(self):
         self.assertEqual(str(cm.exception), "value=2")
         self.assertEqual(repr(cm.exception), "Error.IntValue(value=2)")
 
+    def test_enum_flat_inner(self):
+        with self.assertRaises(Error.FlatInnerError) as cm:
+            oops_enum(3)
+        # XXX - can't compare Python errors.
+        self.assertEqual(str(cm.exception.error), "inner")
+
+        with self.assertRaises(Error.FlatInnerError) as cm:
+            oops_enum(4)
+        # XXX - can't compare Python errors.
+        self.assertEqual(str(cm.exception.error), "NonUniffiTypeValue: value")
+
+    def test_enum_inner(self):
+        with self.assertRaises(Error.InnerError) as cm:
+            oops_enum(5)
+        # XXX - can't compare Python errors.
+        self.assertEqual(cm.exception.error[0], "inner")
+
     def test_tuple_error(self):
         r = get_tuple()
         self.assertEqual(repr(r), "TupleError.Oops('oops')")

fixtures/error-types/tests/bindings/test.swift

@@ -53,6 +53,29 @@ do {
     assert(error.localizedDescription == "error_types.Error.IntValue(value: 2)")
 }
 
+do {
+    try oopsEnum(i: 3)
+    fatalError("Should have thrown")
+} catch let e as Error {
+    assert(String(describing: e) == "FlatInnerError(error: error_types.FlatInner.CaseA(message: \"inner\"))")
+    assert(String(reflecting: e) == "error_types.Error.FlatInnerError(error: error_types.FlatInner.CaseA(message: \"inner\"))")
+}
+
+do {
+    try oopsEnum(i: 4)
+    fatalError("Should have thrown")
+} catch let e as Error {
+    assert(String(describing: e) == "FlatInnerError(error: error_types.FlatInner.CaseB(message: \"NonUniffiTypeValue: value\"))")
+    assert(String(reflecting: e) == "error_types.Error.FlatInnerError(error: error_types.FlatInner.CaseB(message: \"NonUniffiTypeValue: value\"))")
+}
+
+do {
+    try oopsEnum(i: 5)
+    fatalError("Should have thrown")
+} catch let e as Error {
+    assert(String(describing: e) == "InnerError(error: error_types.Inner.CaseA(\"inner\"))")
+}
+
 do {
     try oopsTuple(i: 0)
     fatalError("Should have thrown")

fixtures/metadata/src/tests.rs

@@ -207,7 +207,7 @@ mod test_metadata {
             EnumMetadata {
                 module_path: "uniffi_fixture_metadata".into(),
                 name: "Weapon".into(),
-                forced_flatness: None,
+                shape: EnumShape::Enum,
                 discr_type: None,
                 variants: vec![
                     VariantMetadata {
@@ -242,7 +242,7 @@ mod test_metadata {
             EnumMetadata {
                 module_path: "uniffi_fixture_metadata".into(),
                 name: "State".into(),
-                forced_flatness: None,
+                shape: EnumShape::Enum,
                 discr_type: None,
                 variants: vec![
                     VariantMetadata {
@@ -290,7 +290,7 @@ mod test_metadata {
             EnumMetadata {
                 module_path: "uniffi_fixture_metadata".into(),
                 name: "ReprU8".into(),
-                forced_flatness: None,
+                shape: EnumShape::Enum,
                 discr_type: Some(Type::UInt8),
                 variants: vec![
                     VariantMetadata {
@@ -325,7 +325,7 @@ mod test_metadata {
             EnumMetadata {
                 module_path: "uniffi_fixture_metadata".into(),
                 name: "NoRepr".into(),
-                forced_flatness: None,
+                shape: EnumShape::Enum,
                 discr_type: None,
                 variants: vec![VariantMetadata {
                     name: "One".into(),
@@ -346,7 +346,7 @@ mod test_metadata {
             EnumMetadata {
                 module_path: "uniffi_fixture_metadata".into(),
                 name: "FlatError".into(),
-                forced_flatness: Some(true),
+                shape: EnumShape::Error { flat: true },
                 discr_type: None,
                 variants: vec![
                     VariantMetadata {
@@ -375,7 +375,7 @@ mod test_metadata {
             EnumMetadata {
                 module_path: "uniffi_fixture_metadata".into(),
                 name: "ComplexError".into(),
-                forced_flatness: Some(false),
+                shape: EnumShape::Error { flat: false },
                 discr_type: None,
                 variants: vec![
                     VariantMetadata {

uniffi_bindgen/src/interface/enum_.rs

@@ -160,7 +160,7 @@
 //! ```
 
 use anyhow::Result;
-use uniffi_meta::Checksum;
+use uniffi_meta::{Checksum, EnumShape};
 
 use super::record::Field;
 use super::{AsType, Literal, Type, TypeIterator};
@@ -176,24 +176,7 @@ pub struct Enum {
     pub(super) module_path: String,
     pub(super) discr_type: Option<Type>,
     pub(super) variants: Vec<Variant>,
-    // NOTE: `flat` is a misleading name and to make matters worse, has 2 different
-    // meanings depending on the context :(
-    // * When used as part of Rust scaffolding generation, it means "is this enum
-    //   used with an Error, and that error should we lowered to foreign bindings
-    //   by converting each variant to a string and lowering the variant with that
-    //   string?". In that context, it should probably be called `lowered_as_string` or
-    //   similar.
-    // * When used as part of bindings generation, it means "does this enum have only
-    //   variants with no associated data"? The foreign binding generators are likely
-    //   to generate significantly different versions of the enum based on that value.
-    //
-    // The reason it is described as "has 2 different meanings" by way of example:
-    // * For an Enum described as being a flat error, but the enum itself has variants with data,
-    //   `flat` will be `true` for the Enum when generating scaffolding and `false` when
-    //   generating bindings.
-    // * For an Enum not used as an error but which has no variants with data, `flat` will be
-    //   false when generating the scaffolding but `true` when generating bindings.
-    pub(super) flat: bool,
+    pub(super) shape: EnumShape,
     pub(super) non_exhaustive: bool,
     #[checksum_ignore]
     pub(super) docstring: Option<String>,
@@ -248,7 +231,10 @@ impl Enum {
     }
 
     pub fn is_flat(&self) -> bool {
-        self.flat
+        match self.shape {
+            EnumShape::Error { flat } => flat,
+            EnumShape::Enum => self.variants.iter().all(|v| v.fields.is_empty()),
+        }
     }
 
     pub fn is_non_exhaustive(&self) -> bool {
@@ -262,14 +248,12 @@ impl Enum {
     pub fn docstring(&self) -> Option<&str> {
         self.docstring.as_deref()
     }
+}
+
+impl TryFrom<uniffi_meta::EnumMetadata> for Enum {
+    type Error = anyhow::Error;
 
-    // Sadly can't use TryFrom due to the 'is_flat' complication.
-    pub fn try_from_meta(meta: uniffi_meta::EnumMetadata, flat: bool) -> Result<Self> {
-        // This is messy - error enums are considered "flat" if the user
-        // opted in via a special attribute, regardless of whether the enum
-        // is actually flat.
-        // Real enums are considered flat iff they are actually flat.
-        // We don't have that context here, so this is handled by our caller.
+    fn try_from(meta: uniffi_meta::EnumMetadata) -> Result<Self> {
         Ok(Self {
             name: meta.name,
             module_path: meta.module_path,
@@ -279,7 +263,7 @@ impl Enum {
                 .into_iter()
                 .map(TryInto::try_into)
                 .collect::<Result<_>>()?,
-            flat,
+            shape: meta.shape,
             non_exhaustive: meta.non_exhaustive,
             docstring: meta.docstring.clone(),
         })
@@ -414,6 +398,7 @@ mod test {
         // The enum with associated data.
         let ed = ci.get_enum_definition("TestEnumWithData").unwrap();
         assert!(!ed.is_flat());
+        assert_eq!(ed.shape, EnumShape::Enum);
         assert_eq!(ed.variants().len(), 3);
         assert_eq!(
             ed.variants().iter().map(|v| v.name()).collect::<Vec<_>>(),
@@ -462,6 +447,8 @@ mod test {
         );
         assert_eq!(ewd.variants()[0].fields().len(), 0);
         assert_eq!(ewd.variants()[1].fields().len(), 0);
+        assert!(ewd.is_flat());
+        assert_eq!(ewd.shape, EnumShape::Enum);
 
         // Flat enums pass over the FFI as bytebuffers.
         // (It might be nice to optimize these to pass as plain integers, but that's
@@ -669,7 +656,7 @@ mod test {
             name: "test".to_string(),
             discr_type: None,
             variants: vec![],
-            flat: false,
+            shape: EnumShape::Enum,
             non_exhaustive: false,
             docstring: None,
         };

uniffi_bindgen/src/interface/mod.rs

@@ -52,7 +52,7 @@ use std::{
 use anyhow::{anyhow, bail, ensure, Result};
 
 pub mod universe;
-pub use uniffi_meta::{AsType, ExternalKind, ObjectImpl, Type};
+pub use uniffi_meta::{AsType, EnumShape, ExternalKind, ObjectImpl, Type};
 use universe::{TypeIterator, TypeUniverse};
 
 mod callbacks;
@@ -815,6 +815,9 @@ impl ComponentInterface {
     pub(super) fn add_enum_definition(&mut self, defn: Enum) -> Result<()> {
         match self.enums.entry(defn.name().to_owned()) {
             Entry::Vacant(v) => {
+                if matches!(defn.shape, EnumShape::Error { .. }) {
+                    self.errors.insert(defn.name.clone());
+                }
                 self.types.add_known_types(defn.iter_types())?;
                 v.insert(defn);
             }
@@ -1179,7 +1182,7 @@ existing definition: Enum {
             docstring: None,
         },
     ],
-    flat: true,
+    shape: Enum,
     non_exhaustive: false,
     docstring: None,
 },
@@ -1201,7 +1204,9 @@ new definition: Enum {
             docstring: None,
         },
     ],
-    flat: true,
+    shape: Error {
+        flat: true,
+    },
     non_exhaustive: false,
     docstring: None,
 }",

uniffi_bindgen/src/macro_metadata/ci.rs

@@ -2,7 +2,7 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-use crate::interface::{CallbackInterface, ComponentInterface, Enum, Record, Type};
+use crate::interface::{CallbackInterface, ComponentInterface, Record, Type};
 use anyhow::{bail, Context};
 use uniffi_meta::{create_metadata_groups, group_metadata, EnumMetadata, Metadata, MetadataGroup};
 
@@ -57,19 +57,13 @@ pub fn add_group_to_ci(iface: &mut ComponentInterface, group: MetadataGroup) ->
     Ok(())
 }
 
-fn add_enum_to_ci(
-    iface: &mut ComponentInterface,
-    meta: EnumMetadata,
-    is_flat: bool,
-) -> anyhow::Result<()> {
+fn add_enum_to_ci(iface: &mut ComponentInterface, meta: EnumMetadata) -> anyhow::Result<()> {
     let ty = Type::Enum {
         name: meta.name.clone(),
         module_path: meta.module_path.clone(),
     };
     iface.types.add_known_type(&ty)?;
-
-    let enum_ = Enum::try_from_meta(meta, is_flat)?;
-    iface.add_enum_definition(enum_)?;
+    iface.add_enum_definition(meta.try_into()?)?;
     Ok(())
 }
 
@@ -96,10 +90,7 @@ fn add_item_to_ci(iface: &mut ComponentInterface, item: Metadata) -> anyhow::Res
             iface.add_record_definition(record)?;
         }
         Metadata::Enum(meta) => {
-            let flat = meta
-                .forced_flatness
-                .unwrap_or_else(|| meta.variants.iter().all(|v| v.fields.is_empty()));
-            add_enum_to_ci(iface, meta, flat)?;
+            add_enum_to_ci(iface, meta)?;
         }
         Metadata::Object(meta) => {
             iface.types.add_known_type(&Type::Object {