From 05778b158aab6d8b62b9847e316346e3b0633096 Mon Sep 17 00:00:00 2001 From: Mark Hammond Date: Tue, 31 Dec 2024 20:36:00 -0500 Subject: [PATCH] Remove a panic in add_type_definition and add error contexts so resulting errors make sense. (#2369) --- .../ui/trait_methods_unknown_trait.stderr | 2 +- .../uitests/tests/ui/typedef_extern.stderr | 2 +- uniffi_bindgen/src/interface/mod.rs | 41 ++++++++++++++----- uniffi_bindgen/src/interface/universe.rs | 26 ++++++++---- uniffi_bindgen/src/lib.rs | 8 ++-- uniffi_bindgen/src/library_mode.rs | 9 ++-- 6 files changed, 61 insertions(+), 27 deletions(-) diff --git a/fixtures/uitests/tests/ui/trait_methods_unknown_trait.stderr b/fixtures/uitests/tests/ui/trait_methods_unknown_trait.stderr index c16d65ac66..2f2a62561a 100644 --- a/fixtures/uitests/tests/ui/trait_methods_unknown_trait.stderr +++ b/fixtures/uitests/tests/ui/trait_methods_unknown_trait.stderr @@ -1,4 +1,4 @@ -error: Failed to generate scaffolding from UDL file at ../../../../fixtures/uitests/src/trait_methods_unknown_trait.udl: Invalid trait name: PartialEq +error: Failed to generate scaffolding from UDL file at ../../../../fixtures/uitests/src/trait_methods_unknown_trait.udl: parsing udl file ../../../../fixtures/uitests/src/trait_methods_unknown_trait.udl: Invalid trait name: PartialEq --> tests/ui/trait_methods_unknown_trait.rs:2:1 | 2 | uniffi_macros::generate_and_include_scaffolding!("../../../../fixtures/uitests/src/trait_methods_unknown_trait.udl"); diff --git a/fixtures/uitests/tests/ui/typedef_extern.stderr b/fixtures/uitests/tests/ui/typedef_extern.stderr index 2af3f4d1d3..9647a08f31 100644 --- a/fixtures/uitests/tests/ui/typedef_extern.stderr +++ b/fixtures/uitests/tests/ui/typedef_extern.stderr @@ -1,4 +1,4 @@ -error: Failed to generate scaffolding from UDL file at ../../../../fixtures/uitests/src/typedef_extern.udl: `typedef extern` is no longer supported. +error: Failed to generate scaffolding from UDL file at ../../../../fixtures/uitests/src/typedef_extern.udl: parsing udl file ../../../../fixtures/uitests/src/typedef_extern.udl: `typedef extern` is no longer supported. You must replace `extern` with the type of the object: * "enum" for Enums. * "record", "dictionary" or "struct" for Records. diff --git a/uniffi_bindgen/src/interface/mod.rs b/uniffi_bindgen/src/interface/mod.rs index 9fc0ed3bf8..d2d990b9ed 100644 --- a/uniffi_bindgen/src/interface/mod.rs +++ b/uniffi_bindgen/src/interface/mod.rs @@ -49,7 +49,7 @@ use std::{ iter, }; -use anyhow::{anyhow, bail, ensure, Result}; +use anyhow::{anyhow, bail, ensure, Context, Result}; pub mod universe; pub use uniffi_meta::{AsType, EnumShape, ExternalKind, ObjectImpl, Type}; @@ -822,7 +822,9 @@ impl ComponentInterface { if matches!(defn.shape, EnumShape::Error { .. }) { self.errors.insert(defn.name.clone()); } - self.types.add_known_types(defn.iter_types())?; + self.types + .add_known_types(defn.iter_types()) + .with_context(|| format!("adding enum {defn:?}"))?; v.insert(defn); } Entry::Occupied(o) => { @@ -845,7 +847,9 @@ impl ComponentInterface { pub(super) fn add_record_definition(&mut self, defn: Record) -> Result<()> { match self.records.entry(defn.name().to_owned()) { Entry::Vacant(v) => { - self.types.add_known_types(defn.iter_types())?; + self.types + .add_known_types(defn.iter_types()) + .with_context(|| format!("adding record {defn:?}"))?; v.insert(defn); } Entry::Occupied(o) => { @@ -874,7 +878,9 @@ impl ComponentInterface { if self.types.get_type_definition(defn.name()).is_some() { bail!("Conflicting type definition for \"{}\"", defn.name()); } - self.types.add_known_types(defn.iter_types())?; + self.types + .add_known_types(defn.iter_types()) + .with_context(|| format!("adding function {defn:?}"))?; defn.throws_name() .map(|n| self.errors.insert(n.to_string())); self.functions.push(defn); @@ -887,7 +893,9 @@ impl ComponentInterface { .ok_or_else(|| anyhow!("add_constructor_meta: object {} not found", &meta.self_name))?; let defn: Constructor = meta.into(); - self.types.add_known_types(defn.iter_types())?; + self.types + .add_known_types(defn.iter_types()) + .with_context(|| format!("adding constructor {defn:?}"))?; defn.throws_name() .map(|n| self.errors.insert(n.to_string())); object.constructors.push(defn); @@ -900,7 +908,9 @@ impl ComponentInterface { let object = get_object(&mut self.objects, &method.object_name) .ok_or_else(|| anyhow!("add_method_meta: object {} not found", &method.object_name))?; - self.types.add_known_types(method.iter_types())?; + self.types + .add_known_types(method.iter_types()) + .with_context(|| format!("adding method {method:?}"))?; method .throws_name() .map(|n| self.errors.insert(n.to_string())); @@ -913,7 +923,10 @@ impl ComponentInterface { let object = get_object(&mut self.objects, meta.self_name()) .ok_or_else(|| anyhow!("add_uniffitrait_meta: object not found"))?; let ut: UniffiTrait = meta.into(); - self.types.add_known_types(ut.iter_types())?; + self.types + .add_known_types(ut.iter_types()) + .with_context(|| format!("adding builtin trait {ut:?}"))?; + object.uniffi_traits.push(ut); Ok(()) } @@ -925,7 +938,10 @@ impl ComponentInterface { /// Called by `APIBuilder` impls to add a newly-parsed object definition to the `ComponentInterface`. fn add_object_definition(&mut self, defn: Object) -> Result<()> { self.types.add_known_type(&defn.as_type())?; - self.types.add_known_types(defn.iter_types())?; + self.types + .add_known_types(defn.iter_types()) + .with_context(|| format!("adding object {defn:?}'"))?; + self.objects.push(defn); Ok(()) } @@ -940,7 +956,10 @@ impl ComponentInterface { defn: CallbackInterface, ) -> Result<()> { self.types.add_known_type(&defn.as_type())?; - self.types.add_known_types(defn.iter_types())?; + self.types + .add_known_types(defn.iter_types()) + .with_context(|| format!("adding callback {defn:?}'"))?; + self.callback_interfaces.push(defn); Ok(()) } @@ -962,7 +981,9 @@ impl ComponentInterface { if let Some(error) = method.throws_type() { self.callback_interface_throws_types.insert(error.clone()); } - self.types.add_known_types(method.iter_types())?; + self.types + .add_known_types(method.iter_types()) + .with_context(|| format!("adding trait method {method:?}"))?; method .throws_name() .map(|n| self.errors.insert(n.to_string())); diff --git a/uniffi_bindgen/src/interface/universe.rs b/uniffi_bindgen/src/interface/universe.rs index 9a78b3bf34..ebb14ad338 100644 --- a/uniffi_bindgen/src/interface/universe.rs +++ b/uniffi_bindgen/src/interface/universe.rs @@ -6,7 +6,7 @@ //! which can be used by the bindings generator code to determine what type-related helper //! functions to emit for a given component. //! -use anyhow::Result; +use anyhow::{Context, Result}; use std::{collections::hash_map::Entry, collections::BTreeSet, collections::HashMap}; pub use uniffi_meta::{AsType, ExternalKind, NamespaceMetadata, ObjectImpl, Type, TypeIterator}; @@ -45,9 +45,12 @@ impl TypeUniverse { fn add_type_definition(&mut self, name: &str, type_: &Type) -> Result<()> { match self.type_definitions.entry(name.to_string()) { Entry::Occupied(o) => { - // all conflicts have been resolved by now in udl. - // I doubt procmacros could cause this? - assert_eq!(type_, o.get()); + // mismatched types are bad. + let cur = o.get(); + anyhow::ensure!( + type_ == cur, + "conflicting types:\ncur: {cur:?}\nnew: {type_:?}", + ); Ok(()) } Entry::Vacant(e) => { @@ -91,18 +94,22 @@ impl TypeUniverse { | Type::External { name, .. } => self.add_type_definition(name, type_)?, Type::Custom { name, builtin, .. } => { self.add_type_definition(name, type_)?; - self.add_known_type(builtin)?; + self.add_known_type(builtin) + .with_context(|| format!("adding custom builtin type {builtin:?}"))?; } // Structurally recursive types. Type::Optional { inner_type, .. } | Type::Sequence { inner_type, .. } => { - self.add_known_type(inner_type)?; + self.add_known_type(inner_type) + .with_context(|| format!("adding optional type {inner_type:?}"))?; } Type::Map { key_type, value_type, } => { - self.add_known_type(key_type)?; - self.add_known_type(value_type)?; + self.add_known_type(key_type) + .with_context(|| format!("adding map key type {key_type:?}"))?; + self.add_known_type(value_type) + .with_context(|| format!("adding map value type {value_type:?}"))?; } } Ok(()) @@ -111,7 +118,8 @@ impl TypeUniverse { /// Add many [`Type`]s... pub fn add_known_types(&mut self, types: TypeIterator<'_>) -> Result<()> { for t in types { - self.add_known_type(t)? + self.add_known_type(t) + .with_context(|| format!("adding type {t:?}"))? } Ok(()) } diff --git a/uniffi_bindgen/src/lib.rs b/uniffi_bindgen/src/lib.rs index ab5317a57d..e24f7b05d1 100644 --- a/uniffi_bindgen/src/lib.rs +++ b/uniffi_bindgen/src/lib.rs @@ -335,7 +335,8 @@ pub fn generate_component_scaffolding( out_dir_override: Option<&Utf8Path>, format_code: bool, ) -> Result<()> { - let component = parse_udl(udl_file, &crate_name_from_cargo_toml(udl_file)?)?; + let component = parse_udl(udl_file, &crate_name_from_cargo_toml(udl_file)?) + .with_context(|| format!("parsing udl file {udl_file}"))?; generate_component_scaffolding_inner(component, udl_file, out_dir_override, format_code) } @@ -348,7 +349,8 @@ pub fn generate_component_scaffolding_for_crate( out_dir_override: Option<&Utf8Path>, format_code: bool, ) -> Result<()> { - let component = parse_udl(udl_file, crate_name)?; + let component = + parse_udl(udl_file, crate_name).with_context(|| format!("parsing udl file {udl_file}"))?; generate_component_scaffolding_inner(component, udl_file, out_dir_override, format_code) } @@ -365,7 +367,7 @@ fn generate_component_scaffolding_inner( write!(f, "{}", RustScaffolding::new(&component, file_stem)) .context("Failed to write output file")?; if format_code { - format_code_with_rustfmt(&out_path)?; + format_code_with_rustfmt(&out_path).context("formatting generated Rust code")?; } Ok(()) } diff --git a/uniffi_bindgen/src/library_mode.rs b/uniffi_bindgen/src/library_mode.rs index 3a6a3f0133..806031c6c6 100644 --- a/uniffi_bindgen/src/library_mode.rs +++ b/uniffi_bindgen/src/library_mode.rs @@ -19,7 +19,7 @@ use crate::{ macro_metadata, overridden_config_value, BindgenCrateConfigSupplier, BindingGenerator, Component, ComponentInterface, GenerationSettings, Result, }; -use anyhow::bail; +use anyhow::{bail, Context}; use camino::Utf8Path; use std::{collections::HashMap, fs}; use toml::value::Table as TomlTable; @@ -43,11 +43,14 @@ pub fn generate_bindings( out_dir: &Utf8Path, try_format_code: bool, ) -> Result>> { - let mut components = find_components(library_path, config_supplier)? + let mut components = find_components(library_path, config_supplier) + .with_context(|| format!("finding components in '{library_path}'"))? .into_iter() .map(|Component { ci, config }| { let toml_value = overridden_config_value(config, config_file_override)?; - let config = binding_generator.new_config(&toml_value)?; + let config = binding_generator + .new_config(&toml_value) + .context("loading toml")?; Ok(Component { ci, config }) }) .collect::>>()?;