diff --git a/engine/src/conversion/analysis/pod/byvalue_checker.rs b/engine/src/conversion/analysis/pod/byvalue_checker.rs index b65724c9d..f75094b1a 100644 --- a/engine/src/conversion/analysis/pod/byvalue_checker.rs +++ b/engine/src/conversion/analysis/pod/byvalue_checker.rs @@ -152,6 +152,12 @@ impl ByValueChecker { let fieldlist = Self::get_field_types(def); for ty_id in &fieldlist { match self.results.get(ty_id) { + None if ty_id.get_final_item() == "__BindgenUnionField" => { + field_safety_problem = PodState::UnsafeToBePod(format!( + "Type {tyname} could not be POD because it is a union" + )); + break; + } None => { field_safety_problem = PodState::UnsafeToBePod(format!( "Type {tyname} could not be POD because its dependent type {ty_id} isn't known" diff --git a/engine/src/conversion/analysis/type_converter.rs b/engine/src/conversion/analysis/type_converter.rs index ada512be7..5aec8fcb8 100644 --- a/engine/src/conversion/analysis/type_converter.rs +++ b/engine/src/conversion/analysis/type_converter.rs @@ -10,7 +10,7 @@ use crate::{ conversion::{ api::{AnalysisPhase, Api, ApiName, NullPhase, TypedefKind, UnanalyzedApi}, apivec::ApiVec, - codegen_cpp::type_to_cpp::type_to_cpp, + codegen_cpp::type_to_cpp::CppNameMap, ConvertErrorFromCpp, }, known_types::{known_types, CxxGenericType}, @@ -130,6 +130,7 @@ pub(crate) struct TypeConverter<'a> { forward_declarations: HashSet, ignored_types: HashSet, config: &'a IncludeCppConfig, + original_name_map: CppNameMap, } impl<'a> TypeConverter<'a> { @@ -144,6 +145,7 @@ impl<'a> TypeConverter<'a> { forward_declarations: Self::find_incomplete_types(apis), ignored_types: Self::find_ignored_types(apis), config, + original_name_map: CppNameMap::new_from_apis(apis), } } @@ -521,7 +523,7 @@ impl<'a> TypeConverter<'a> { // We just use this as a hash key, essentially. // TODO: Once we've completed the TypeConverter refactoring (see #220), // pass in an actual original_name_map here. - let cpp_definition = type_to_cpp(rs_definition, &HashMap::new())?; + let cpp_definition = self.original_name_map.type_to_cpp(rs_definition)?; let e = self.concrete_templates.get(&cpp_definition); match e { Some(tn) => Ok((tn.clone(), None)), diff --git a/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs b/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs index 9e0aff9a1..661a5b3a3 100644 --- a/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs +++ b/engine/src/conversion/codegen_cpp/function_wrapper_cpp.rs @@ -14,7 +14,7 @@ use crate::conversion::{ ConvertErrorFromCpp, }; -use super::type_to_cpp::{type_to_cpp, CppNameMap}; +use super::type_to_cpp::CppNameMap; impl TypeConversionPolicy { pub(super) fn unconverted_type( @@ -49,7 +49,7 @@ impl TypeConversionPolicy { Ok(format!( "{}{}*", const_string, - type_to_cpp(ty, cpp_name_map)? + cpp_name_map.type_to_cpp(ty)? )) } _ => self.unwrapped_type_as_string(cpp_name_map), @@ -60,7 +60,7 @@ impl TypeConversionPolicy { &self, cpp_name_map: &CppNameMap, ) -> Result { - type_to_cpp(self.cxxbridge_type(), cpp_name_map) + cpp_name_map.type_to_cpp(self.cxxbridge_type()) } pub(crate) fn is_a_pointer(&self) -> Pointerness { diff --git a/engine/src/conversion/codegen_cpp/mod.rs b/engine/src/conversion/codegen_cpp/mod.rs index 35cc97825..bb341a86f 100644 --- a/engine/src/conversion/codegen_cpp/mod.rs +++ b/engine/src/conversion/codegen_cpp/mod.rs @@ -20,11 +20,7 @@ use indexmap::map::IndexMap as HashMap; use indexmap::set::IndexSet as HashSet; use itertools::Itertools; use std::borrow::Cow; -use type_to_cpp::{original_name_map_from_apis, type_to_cpp, CppNameMap}; - -use self::type_to_cpp::{ - final_ident_using_original_name_map, namespaced_name_using_original_name_map, -}; +use type_to_cpp::CppNameMap; use super::{ analysis::{ @@ -124,7 +120,7 @@ impl<'a> CppCodeGenerator<'a> { let mut gen = CppCodeGenerator { additional_functions: Vec::new(), inclusions, - original_name_map: original_name_map_from_apis(apis), + original_name_map: CppNameMap::new_from_apis(apis), config, cpp_codegen_options, cxxgen_header_name, @@ -172,7 +168,7 @@ impl<'a> CppCodeGenerator<'a> { } => { let effective_cpp_definition = match rs_definition { Some(rs_definition) => { - Cow::Owned(type_to_cpp(rs_definition, &self.original_name_map)?) + Cow::Owned(self.original_name_map.type_to_cpp(rs_definition)?) } None => Cow::Borrowed(cpp_definition), }; @@ -486,9 +482,34 @@ impl<'a> CppCodeGenerator<'a> { ) } CppFunctionBody::Destructor(ns, id) => { - let ty_id = QualifiedName::new(ns, id.clone()); - let ty_id = final_ident_using_original_name_map(&ty_id, &self.original_name_map); - (format!("{arg_list}->~{ty_id}()"), "".to_string(), false) + let full_name = QualifiedName::new(ns, id.clone()); + let ty_id = self.original_name_map.get_final_item(&full_name); + let is_a_nested_struct = self.original_name_map.get(&full_name).is_some(); + // This is all super duper fiddly. + // All we want to do is call a destructor. Constraints: + // * an unnamed struct, e.g. typedef struct { .. } A, does not + // have any way of fully qualifying its destructor name. + // We have to use a 'using' statement. + // * we don't get enough information from bindgen to distinguish + // typedef struct { .. } A // unnamed struct + // from + // struct A { .. } // named struct + // * we can only do 'using A::B::C' if 'B' is a namespace, + // as opposed to a type with an inner type. + // * we can always do 'using C = A::B::C' but then SOME C++ + // compilers complain that it's unused, iff it's a named struct. + let destructor_call = format!("{arg_list}->{ty_id}::~{ty_id}()"); + let destructor_call = if ns.is_empty() { + destructor_call + } else { + let path = self.original_name_map.map(&full_name); + if is_a_nested_struct { + format!("{{ using {ty_id} = {path}; {destructor_call}; {ty_id}* pointless; (void)pointless; }}") + } else { + format!("{{ using {path}; {destructor_call}; }}") + } + }; + (destructor_call, "".to_string(), false) } CppFunctionBody::FunctionCall(ns, id) => match receiver { Some(receiver) => ( @@ -554,7 +575,7 @@ impl<'a> CppCodeGenerator<'a> { underlying_function_call = match placement_param { Some(placement_param) => { - let tyname = type_to_cpp(ret.cxxbridge_type(), &self.original_name_map)?; + let tyname = self.original_name_map.type_to_cpp(ret.cxxbridge_type())?; format!("new({placement_param}) {tyname}({call_itself})") } None => format!("return {call_itself}"), @@ -600,7 +621,7 @@ impl<'a> CppCodeGenerator<'a> { } fn namespaced_name(&self, name: &QualifiedName) -> String { - namespaced_name_using_original_name_map(name, &self.original_name_map) + self.original_name_map.map(name) } fn generate_ctype_typedef(&mut self, tn: &QualifiedName) { diff --git a/engine/src/conversion/codegen_cpp/type_to_cpp.rs b/engine/src/conversion/codegen_cpp/type_to_cpp.rs index 070d400d4..b2da44b58 100644 --- a/engine/src/conversion/codegen_cpp/type_to_cpp.rs +++ b/engine/src/conversion/codegen_cpp/type_to_cpp.rs @@ -19,121 +19,129 @@ use syn::{Token, Type}; /// Map from QualifiedName to original C++ name. Original C++ name does not /// include the namespace; this can be assumed to be the same as the namespace /// in the QualifiedName. -pub(crate) type CppNameMap = HashMap; +/// The "original C++ name" is mostly relevant in the case of nested types, +/// where the typename might be A::B within a namespace C::D. +pub(crate) struct CppNameMap(HashMap); -pub(crate) fn original_name_map_from_apis(apis: &ApiVec) -> CppNameMap { - apis.iter() - .filter_map(|api| { - api.cpp_name() - .as_ref() - .map(|cpp_name| (api.name().clone(), cpp_name.clone())) - }) - .collect() -} +impl CppNameMap { + /// Look through the APIs we've found to assemble the original name + /// map. + pub(crate) fn new_from_apis(apis: &ApiVec) -> Self { + Self( + apis.iter() + .filter_map(|api| { + api.cpp_name() + .as_ref() + .map(|cpp_name| (api.name().clone(), cpp_name.clone())) + }) + .collect(), + ) + } -pub(crate) fn namespaced_name_using_original_name_map( - qual_name: &QualifiedName, - original_name_map: &CppNameMap, -) -> String { - if let Some(cpp_name) = original_name_map.get(qual_name) { - qual_name - .get_namespace() - .iter() - .chain(once(cpp_name)) - .join("::") - } else { - qual_name.to_cpp_name() + /// Imagine a nested struct in namespace::outer::inner + /// This function converts from the bindgen name, namespace::outer_inner, + /// to namespace::outer::inner. + pub(crate) fn map(&self, qual_name: &QualifiedName) -> String { + if let Some(cpp_name) = self.0.get(qual_name) { + qual_name + .get_namespace() + .iter() + .chain(once(cpp_name)) + .join("::") + } else { + qual_name.to_cpp_name() + } } -} -pub(crate) fn final_ident_using_original_name_map( - qual_name: &QualifiedName, - original_name_map: &CppNameMap, -) -> String { - match original_name_map.get(qual_name) { - Some(original_name) => { - // If we have an original name, this may be a nested struct - // (e.g. A::B). The final ident here is just 'B' so... - original_name - .rsplit_once("::") - .map_or(original_name.clone(), |(_, original_name)| { - original_name.to_string() - }) + /// Get a stringified version of the last ident in the name. + /// e.g. for namespace::outer_inner this will return inner. + /// This is useful for doing things such as calling constructors + /// such as inner() or destructors such as ~inner() + pub(crate) fn get_final_item<'b>(&'b self, qual_name: &'b QualifiedName) -> &'b str { + match self.get(qual_name) { + Some(n) => match n.rsplit_once("::") { + Some((_, suffix)) => suffix, + None => qual_name.get_final_item(), + }, + None => qual_name.get_final_item(), } - None => qual_name.get_final_cpp_item(), } -} -pub(crate) fn type_to_cpp( - ty: &Type, - cpp_name_map: &CppNameMap, -) -> Result { - match ty { - Type::Path(typ) => { - // If this is a std::unique_ptr we do need to pass - // its argument through. - let qual_name = QualifiedName::from_type_path(typ); - let root = namespaced_name_using_original_name_map(&qual_name, cpp_name_map); - if root == "Pin" { - // Strip all Pins from type names when describing them in C++. - let inner_type = &typ.path.segments.last().unwrap().arguments; - if let syn::PathArguments::AngleBracketed(ab) = inner_type { - let inner_type = ab.args.iter().next().unwrap(); - if let syn::GenericArgument::Type(gat) = inner_type { - return type_to_cpp(gat, cpp_name_map); + /// Convert a type to its C++ spelling. + pub(crate) fn type_to_cpp(&self, ty: &Type) -> Result { + match ty { + Type::Path(typ) => { + // If this is a std::unique_ptr we do need to pass + // its argument through. + let qual_name = QualifiedName::from_type_path(typ); + let root = self.map(&qual_name); + if root == "Pin" { + // Strip all Pins from type names when describing them in C++. + let inner_type = &typ.path.segments.last().unwrap().arguments; + if let syn::PathArguments::AngleBracketed(ab) = inner_type { + let inner_type = ab.args.iter().next().unwrap(); + if let syn::GenericArgument::Type(gat) = inner_type { + return self.type_to_cpp(gat); + } } + panic!("Pin<...> didn't contain the inner types we expected"); } - panic!("Pin<...> didn't contain the inner types we expected"); - } - let suffix = match &typ.path.segments.last().unwrap().arguments { - syn::PathArguments::AngleBracketed(ab) => { - let results: Result, _> = ab - .args - .iter() - .map(|x| match x { - syn::GenericArgument::Type(gat) => type_to_cpp(gat, cpp_name_map), - _ => Ok("".to_string()), - }) - .collect(); - Some(results?.join(", ")) + let suffix = match &typ.path.segments.last().unwrap().arguments { + syn::PathArguments::AngleBracketed(ab) => { + let results: Result, _> = ab + .args + .iter() + .map(|x| match x { + syn::GenericArgument::Type(gat) => self.type_to_cpp(gat), + _ => Ok("".to_string()), + }) + .collect(); + Some(results?.join(", ")) + } + syn::PathArguments::None | syn::PathArguments::Parenthesized(_) => None, + }; + match suffix { + None => Ok(root), + Some(suffix) => Ok(format!("{root}<{suffix}>")), } - syn::PathArguments::None | syn::PathArguments::Parenthesized(_) => None, - }; - match suffix { - None => Ok(root), - Some(suffix) => Ok(format!("{root}<{suffix}>")), } - } - Type::Reference(typr) => match &*typr.elem { - Type::Path(typ) if typ.path.is_ident("str") => Ok("rust::Str".into()), - _ => Ok(format!( - "{}{}&", - get_mut_string(&typr.mutability), - type_to_cpp(typr.elem.as_ref(), cpp_name_map)? + Type::Reference(typr) => match &*typr.elem { + Type::Path(typ) if typ.path.is_ident("str") => Ok("rust::Str".into()), + _ => Ok(format!( + "{}{}&", + get_mut_string(&typr.mutability), + self.type_to_cpp(typr.elem.as_ref())? + )), + }, + Type::Ptr(typp) => Ok(format!( + "{}{}*", + get_mut_string(&typp.mutability), + self.type_to_cpp(typp.elem.as_ref())? + )), + Type::Array(_) + | Type::BareFn(_) + | Type::Group(_) + | Type::ImplTrait(_) + | Type::Infer(_) + | Type::Macro(_) + | Type::Never(_) + | Type::Paren(_) + | Type::Slice(_) + | Type::TraitObject(_) + | Type::Tuple(_) + | Type::Verbatim(_) => Err(ConvertErrorFromCpp::UnsupportedType( + ty.to_token_stream().to_string(), + )), + _ => Err(ConvertErrorFromCpp::UnknownType( + ty.to_token_stream().to_string(), )), - }, - Type::Ptr(typp) => Ok(format!( - "{}{}*", - get_mut_string(&typp.mutability), - type_to_cpp(typp.elem.as_ref(), cpp_name_map)? - )), - Type::Array(_) - | Type::BareFn(_) - | Type::Group(_) - | Type::ImplTrait(_) - | Type::Infer(_) - | Type::Macro(_) - | Type::Never(_) - | Type::Paren(_) - | Type::Slice(_) - | Type::TraitObject(_) - | Type::Tuple(_) - | Type::Verbatim(_) => Err(ConvertErrorFromCpp::UnsupportedType( - ty.to_token_stream().to_string(), - )), - _ => Err(ConvertErrorFromCpp::UnknownType( - ty.to_token_stream().to_string(), - )), + } + } + + /// Check an individual item in the name map. Returns a thing if + /// it's an inner type, otherwise returns none. + pub(crate) fn get(&self, name: &QualifiedName) -> Option<&String> { + self.0.get(name) } } diff --git a/engine/src/conversion/codegen_rs/mod.rs b/engine/src/conversion/codegen_rs/mod.rs index 37aba4fa3..1dbc637ee 100644 --- a/engine/src/conversion/codegen_rs/mod.rs +++ b/engine/src/conversion/codegen_rs/mod.rs @@ -55,9 +55,7 @@ use super::{ use super::{ api::{Layout, Provenance, RustSubclassFnDetails, SuperclassMethod, TraitImplSignature}, apivec::ApiVec, - codegen_cpp::type_to_cpp::{ - namespaced_name_using_original_name_map, original_name_map_from_apis, CppNameMap, - }, + codegen_cpp::type_to_cpp::CppNameMap, }; use super::{convert_error::ErrorContext, ConvertErrorFromCpp}; use quote::quote; @@ -163,7 +161,7 @@ impl<'a> RsCodeGenerator<'a> { unsafe_policy, include_list, bindgen_mod, - original_name_map: original_name_map_from_apis(&all_apis), + original_name_map: CppNameMap::new_from_apis(&all_apis), config, header_name, }; @@ -1137,7 +1135,7 @@ impl<'a> RsCodeGenerator<'a> { } fn generate_extern_type_impl(&self, type_kind: TypeKind, tyname: &QualifiedName) -> Vec { - let tynamestring = namespaced_name_using_original_name_map(tyname, &self.original_name_map); + let tynamestring = self.original_name_map.map(tyname); let fulltypath = tyname.get_bindgen_path_idents(); let kind_item = match type_kind { TypeKind::Pod => "Trivial", diff --git a/engine/src/types.rs b/engine/src/types.rs index 0239eff7d..deb3525fb 100644 --- a/engine/src/types.rs +++ b/engine/src/types.rs @@ -53,11 +53,15 @@ impl Namespace { pub(crate) fn depth(&self) -> usize { self.0.len() } + + pub(crate) fn to_cpp_path(&self) -> String { + self.0.join("::") + } } impl Display for Namespace { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - f.write_str(&self.0.join("::")) + f.write_str(&self.to_cpp_path()) } } @@ -173,14 +177,6 @@ impl QualifiedName { } } - pub(crate) fn get_final_cpp_item(&self) -> String { - let special_cpp_name = known_types().special_cpp_name(self); - match special_cpp_name { - Some(name) => Self::new_from_cpp_name(&name).1, - None => self.1.to_string(), - } - } - pub(crate) fn to_type_path(&self) -> TypePath { if let Some(known_type_path) = known_types().known_type_type_path(self) { known_type_path diff --git a/integration-tests/tests/integration_test.rs b/integration-tests/tests/integration_test.rs index 3954e35c7..4807d2a4a 100644 --- a/integration-tests/tests/integration_test.rs +++ b/integration-tests/tests/integration_test.rs @@ -4981,6 +4981,77 @@ fn test_union_ignored() { run_test("", hdr, rs, &["B"], &[]); } +#[test] +fn test_union_nonpod() { + let hdr = indoc! {" + #include + union A { + uint32_t a; + float b; + }; + "}; + let rs = quote! {}; + run_test("", hdr, rs, &["A"], &[]); +} + +#[test] +fn test_union_pod() { + let hdr = indoc! {" + #include + union A { + uint32_t a; + float b; + }; + "}; + let rs = quote! {}; + run_test_expect_fail("", hdr, rs, &[], &["A"]); +} + +#[test] +fn test_type_aliased_anonymous_union_ignored() { + let hdr = indoc! {" + #include + namespace test { + typedef union { + int a; + } Union; + }; + "}; + let rs = quote! {}; + run_test("", hdr, rs, &["test::Union"], &[]); +} + +#[test] +fn test_type_aliased_anonymous_struct_ignored() { + let hdr = indoc! {" + #include + namespace test { + typedef struct { + int a; + } Struct; + }; + "}; + let rs = quote! {}; + run_test("", hdr, rs, &["test::Struct"], &[]); +} + +#[test] +fn test_type_aliased_anonymous_nested_struct_ignored() { + let hdr = indoc! {" + #include + namespace test { + struct Outer { + typedef struct { + int a; + } Struct; + int b; + }; + }; + "}; + let rs = quote! {}; + run_test("", hdr, rs, &["test::Outer_Struct"], &[]); +} + #[ignore] // https://github.com/google/autocxx/issues/1251 #[test] fn test_double_underscores_ignored() {