Fix namespaced destructors and reject unions

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"

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<QualifiedName>,
     ignored_types: HashSet<QualifiedName>,
     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)),

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<String, ConvertErrorFromCpp> {
-        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 {

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

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<QualifiedName, String>;
+/// 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<QualifiedName, String>);
 
-pub(crate) fn original_name_map_from_apis<T: AnalysisPhase>(apis: &ApiVec<T>) -> 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<T: AnalysisPhase>(apis: &ApiVec<T>) -> 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<String, ConvertErrorFromCpp> {
-    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<String, ConvertErrorFromCpp> {
+        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<Vec<_>, _> = 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<Vec<_>, _> = 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)
     }
 }