Avoid 'unused template param' problems

bindgen can handle a large fraction of C++ code, even with templates, and
autocxx can in turn handle a very large fraction of bindgen output. Pretty much
the only things autocxx can't handle are:

* bindgen opaque types passed by value
* Types with unused template parameters

This change, with its associated bindgen change, tries to make progress to
removing this second category of unhandled thing.

It alters bindgen such that it always uses all template parameters, adding a
PhantomData field where necessary.

It's not quite right yet.

Author: adetaylor

Cargo.lock

@@ -38,8 +38,8 @@ moveit = { version = "0.6", features = [ "cxx" ] }
 members = ["parser", "engine", "gen/cmd", "gen/build", "macro", "demo", "tools/reduce", "tools/mdbook-preprocessor", "integration-tests"]
 exclude = ["examples/s2", "examples/steam-mini", "examples/subclass", "examples/chromium-fake-render-frame-host", "examples/pod", "examples/non-trivial-type-on-stack", "examples/llvm", "examples/reference-wrappers", "examples/cpp_calling_rust", "tools/stress-test"]
 
-#[patch.crates-io]
+[patch.crates-io]
 #cxx = { path="../cxx" }
 #cxx-gen = { path="../cxx/gen/lib" }
-#autocxx-bindgen = { path="../bindgen" }
+#autocxx-bindgen = { path="../bindgen/bindgen" }
 #moveit = { path="../moveit" }

Cargo.toml

@@ -30,8 +30,8 @@ log = "0.4"
 proc-macro2 = "1.0.11"
 quote = "1.0"
 indoc = "1.0"
-autocxx-bindgen = { version = "=0.71.1", default-features = false, features = ["logging", "which-rustfmt"] }
-#autocxx-bindgen = { git = "https://github.com/adetaylor/rust-bindgen", branch = "merge-latest-upstream", default-features = false, features = ["logging", "which-rustfmt"] }
+#autocxx-bindgen = { version = "=0.71.1", default-features = false, features = ["logging", "which-rustfmt"] }
+autocxx-bindgen = { git = "https://github.com/adetaylor/rust-bindgen", branch = "hacks-for-using-all-template-params", default-features = false, features = ["logging", "which-rustfmt"] }
 itertools = "0.10.3"
 cc = { version = "1.0", optional = true }
 # Note: Keep the patch-level version of cxx-gen and cxx in sync.

engine/Cargo.toml

@@ -93,6 +93,7 @@ pub(crate) fn replace_hopeless_typedef_targets(
             Api::ForwardDeclaration {
                 name,
                 err: Some(ConvertErrorWithContext(err, ctx)),
+                ..
             } => Api::IgnoredItem { name, err, ctx },
             _ => api,
         })

engine/src/conversion/analysis/replace_hopeless_typedef_targets.rs

@@ -233,37 +233,14 @@ impl<'a> TypeConverter<'a> {
         ctx: &TypeConversionContext,
     ) -> Result<Annotated<Type>, ConvertErrorFromCpp> {
         // First, qualify any unqualified paths.
-        if typ.path.segments.iter().next().unwrap().ident != "root" {
-            let ty = QualifiedName::from_type_path(&typ);
-            // If the type looks like it is unqualified, check we know it
-            // already, and if not, qualify it according to the current
-            // namespace. This is a bit of a shortcut compared to having a full
-            // resolution pass which can search all known namespaces.
-            if !known_types().is_known_type(&ty) {
-                let num_segments = typ.path.segments.len();
-                if num_segments > 1 {
-                    return Err(ConvertErrorFromCpp::UnsupportedBuiltInType(ty));
-                }
-                if !self.types_found.contains(&ty) {
-                    typ.path.segments = std::iter::once(&"root".to_string())
-                        .chain(ns.iter())
-                        .map(|s| {
-                            let i = make_ident(s);
-                            parse_quote! { #i }
-                        })
-                        .chain(typ.path.segments)
-                        .collect();
-                }
-            }
-        }
-
-        let original_tn = QualifiedName::from_type_path(&typ);
+        let original_tn = self.qualified_name_from_bindgen_output_type_path(&mut typ, ns)?;
         original_tn
             .validate_ok_for_cxx()
             .map_err(ConvertErrorFromCpp::InvalidIdent)?;
         if self.config.is_on_blocklist(&original_tn.to_cpp_name()) {
             return Err(ConvertErrorFromCpp::Blocked(original_tn));
         }
+
         let mut deps = HashSet::new();
 
         // Now convert this type itself.
@@ -370,7 +347,7 @@ impl<'a> TypeConverter<'a> {
                 if self.ignored_types.contains(&qn) {
                     return Err(ConvertErrorFromCpp::ConcreteVersionOfIgnoredTemplate);
                 }
-                let (new_tn, api) = self.get_templated_typename(&Type::Path(typ))?;
+                let (new_tn, api) = self.get_templated_typename(&Type::Path(typ), &qn, ns)?;
                 extra_apis.extend(api.into_iter());
                 // Although it's tempting to remove the dep on the original type,
                 // this means we wouldn't spot cases where the original type can't
@@ -383,13 +360,87 @@ impl<'a> TypeConverter<'a> {
         Ok(Annotated::new(Type::Path(typ), deps, extra_apis, kind))
     }
 
+    fn qualified_name_from_bindgen_output_type_path(
+        &self,
+        typ: &mut TypePath,
+        ns: &Namespace,
+    ) -> Result<QualifiedName, ConvertErrorFromCpp> {
+        if typ.path.segments.iter().next().unwrap().ident != "root" {
+            let ty = QualifiedName::from_type_path(&typ);
+            // If the type looks like it is unqualified, check we know it
+            // already, and if not, qualify it according to the current
+            // namespace. This is a bit of a shortcut compared to having a full
+            // resolution pass which can search all known namespaces.
+            if !known_types().is_known_type(&ty) {
+                let num_segments = typ.path.segments.len();
+                if num_segments > 1 {
+                    return Err(ConvertErrorFromCpp::UnsupportedBuiltInType(ty));
+                }
+                if !self.types_found.contains(&ty) {
+                    typ.path.segments = std::iter::once(&"root".to_string())
+                        .chain(ns.iter())
+                        .map(|s| {
+                            let i = make_ident(s);
+                            parse_quote! { #i }
+                        })
+                        .chain(typ.path.segments.clone())
+                        .collect();
+                }
+            }
+        }
+
+        Ok(QualifiedName::from_type_path(&typ))
+    }
+
     fn get_generic_args(typ: &mut TypePath) -> Option<&mut PathSegment> {
         match typ.path.segments.last_mut() {
             Some(s) if !s.arguments.is_empty() => Some(s),
             _ => None,
         }
     }
 
+    fn type_params_depend_on_forward_declarations(&self, ty: &Type, ns: &Namespace ) -> bool {
+        if let Type::Path(typ) = ty {
+            let type_params = self.get_type_params(&mut typ.clone(), ns);
+            !type_params.is_disjoint(&self.forward_declarations)    
+        } else {
+            false // FIXME do better
+        }
+    }
+
+    /// Get the names of any type parameters in this type path.
+    fn get_type_params(&self, typ: &mut TypePath, ns: &Namespace) -> HashSet<QualifiedName> {
+        typ.path
+            .segments
+            .iter_mut()
+            .map(|seg| match seg.arguments {
+                PathArguments::AngleBracketed(ref mut angle_bracketed_generic_arguments) => {
+                    Box::new(
+                        angle_bracketed_generic_arguments
+                            .args
+                            .iter_mut()
+                            .filter_map(|arg| match arg {
+                                GenericArgument::Type(Type::Path(ref mut inner_typ)) => {
+                                    // Disregard any errors during this process, for now.
+                                    // We are looking for types which we have previously
+                                    // recorded as forward declarations. If we can't
+                                    // figure out the name of this type now, we probably
+                                    // couldn't have done so in the past when we
+                                    // recorded it as a forward declaration, so it wouldn't
+                                    // have matched anyway.
+                                    self.qualified_name_from_bindgen_output_type_path(inner_typ, ns)
+                                        .ok()
+                                }
+                                _ => None,
+                            }),
+                    ) as Box<dyn Iterator<Item = QualifiedName>>
+                }
+                _ => Box::new(std::iter::empty()) as Box<dyn Iterator<Item = QualifiedName>>,
+            })
+            .flatten()
+            .collect()
+    }
+
     fn convert_punctuated<P>(
         &mut self,
         pun: Punctuated<GenericArgument, P>,
@@ -525,6 +576,8 @@ impl<'a> TypeConverter<'a> {
     fn get_templated_typename(
         &mut self,
         rs_definition: &Type,
+        qn: &QualifiedName,
+        ns: &Namespace,
     ) -> Result<(QualifiedName, Option<UnanalyzedApi>), ConvertErrorFromCpp> {
         let count = self.concrete_templates.len();
         // We just use this as a hash key, essentially.
@@ -556,10 +609,13 @@ impl<'a> TypeConverter<'a> {
                     None => synthetic_ident,
                     Some(_) => format!("AutocxxConcrete{count}"),
                 };
+
+                let depends_on_forward_declaration = self.forward_declarations.contains(qn) || self.type_params_depend_on_forward_declarations(&rs_definition, ns);
                 let api = UnanalyzedApi::ConcreteType {
                     name: ApiName::new_in_root_namespace(make_ident(synthetic_ident)),
                     cpp_definition: cpp_definition.clone(),
                     rs_definition: Some(Box::new(rs_definition.clone().into())),
+                    depends_on_forward_declaration,
                 };
                 self.concrete_templates
                     .insert(cpp_definition, api.name().clone());
@@ -670,6 +726,10 @@ impl<'a> TypeConverter<'a> {
                 | Api::OpaqueTypedef {
                     forward_declaration: true,
                     ..
+                }
+                | Api::ConcreteType {
+                    depends_on_forward_declaration: true,
+                    ..
                 } => Some(api.name()),
                 _ => None,
             })
@@ -699,10 +759,12 @@ pub(crate) fn add_analysis<A: AnalysisPhase>(api: UnanalyzedApi) -> Api<A> {
             name,
             rs_definition,
             cpp_definition,
+            depends_on_forward_declaration,
         } => Api::ConcreteType {
             name,
             rs_definition,
             cpp_definition,
+            depends_on_forward_declaration,
         },
         Api::IgnoredItem { name, err, ctx } => Api::IgnoredItem { name, err, ctx },
         _ => panic!("Function analysis created an unexpected type of extra API"),

engine/src/conversion/analysis/type_converter.rs

@@ -466,6 +466,7 @@ pub(crate) enum Api<T: AnalysisPhase> {
     /// A forward declaration, which we mustn't store in a UniquePtr.
     ForwardDeclaration {
         name: ApiName,
+        is_templated: bool,
         /// If we found a problem parsing this forward declaration, we'll
         /// ephemerally store the error here, as opposed to immediately
         /// converting it to an `IgnoredItem`. That's because the
@@ -490,6 +491,7 @@ pub(crate) enum Api<T: AnalysisPhase> {
         name: ApiName,
         rs_definition: Option<Box<Type>>,
         cpp_definition: String,
+        depends_on_forward_declaration: bool,
     },
     /// A simple note that we want to make a constructor for
     /// a `std::string` on the heap.

engine/src/conversion/api.rs

@@ -550,23 +550,31 @@ impl<'a> RsCodeGenerator<'a> {
                     false,
                 )
             }
-            Api::ConcreteType { .. } => self.generate_type(
+            Api::ForwardDeclaration {
+                is_templated: false,
+                ..
+            }
+            | Api::OpaqueTypedef { .. }
+            | Api::ConcreteType {
+                depends_on_forward_declaration: true,
+                ..
+            } => self.generate_type(
                 &name,
                 id,
                 TypeKind::Abstract,
-                false, // assume for now that these types can't be kept in a Vector
-                true,  // assume for now that these types can be put in a smart pointer
+                false, // these types can't be kept in a Vector
+                false, // these types can't be put in a smart pointer
                 || None,
                 associated_methods,
                 None,
                 false,
             ),
-            Api::ForwardDeclaration { .. } | Api::OpaqueTypedef { .. } => self.generate_type(
+            Api::ConcreteType { .. } => self.generate_type(
                 &name,
                 id,
                 TypeKind::Abstract,
-                false, // these types can't be kept in a Vector
-                false, // these types can't be put in a smart pointer
+                false, // assume for now that these types can't be kept in a Vector
+                true,  // assume for now that these types can be put in a smart pointer
                 || None,
                 associated_methods,
                 None,
@@ -643,7 +651,11 @@ impl<'a> RsCodeGenerator<'a> {
                 ctx: Some(ctx),
                 ..
             } => Self::generate_error_entry(err, ctx),
-            Api::IgnoredItem { .. } | Api::SubclassTraitItem { .. } => RsCodegenResult::default(),
+            Api::IgnoredItem { .. }
+            | Api::SubclassTraitItem { .. }
+            | Api::ForwardDeclaration {
+                is_templated: true, ..
+            } => RsCodegenResult::default(),
         }
     }
 

engine/src/conversion/codegen_rs/mod.rs

@@ -94,17 +94,22 @@ pub(crate) fn convert_apis<FF, SF, EF, TF, A, B>(
                 name,
                 rs_definition,
                 cpp_definition,
+                depends_on_forward_declaration,
             } => Ok(Box::new(std::iter::once(Api::ConcreteType {
                 name,
                 rs_definition,
                 cpp_definition,
+                depends_on_forward_declaration,
+            }))),
+            Api::ForwardDeclaration {
+                name,
+                is_templated,
+                err,
+            } => Ok(Box::new(std::iter::once(Api::ForwardDeclaration {
+                name,
+                is_templated,
+                err,
             }))),
-            Api::ForwardDeclaration { name, err } => {
-                Ok(Box::new(std::iter::once(Api::ForwardDeclaration {
-                    name,
-                    err,
-                })))
-            }
             Api::OpaqueTypedef {
                 name,
                 forward_declaration,

engine/src/conversion/error_reporter.rs

@@ -56,12 +56,7 @@ impl BindgenSemanticAttributes {
         &self,
         id_for_context: &Ident,
     ) -> Result<(), ConvertErrorWithContext> {
-        if self.has_attr("unused_template_param") {
-            Err(ConvertErrorWithContext(
-                ConvertErrorFromCpp::UnusedTemplateParam,
-                Some(ErrorContext::new_for_item(id_for_context.clone().into())),
-            ))
-        } else if self.get_cpp_visibility() != CppVisibility::Public {
+        if self.get_cpp_visibility() != CppVisibility::Public {
             Err(ConvertErrorWithContext(
                 ConvertErrorFromCpp::NonPublicNestedType,
                 Some(ErrorContext::new_for_item(id_for_context.clone().into())),

engine/src/conversion/parse/bindgen_semantic_attributes.rs

@@ -132,6 +132,7 @@ impl<'a> ParseBindgen<'a> {
                         name,
                         cpp_definition: cpp_definition.clone(),
                         rs_definition: None,
+                        depends_on_forward_declaration: false,
                     }
                 }),
         );
@@ -221,15 +222,9 @@ impl<'a> ParseBindgen<'a> {
                 let mut err = annotations.check_for_fatal_attrs(&s.ident).err();
                 let api = if ns.is_empty() && self.config.is_rust_type(&s.ident) {
                     None
-                } else if Self::spot_forward_declaration(&s.fields)
-                    || (Self::spot_zero_length_struct(&s.fields) && err.is_some())
-                {
+                } else if Self::spot_forward_declaration(&s.fields) {
                     // Forward declarations are recorded especially because we can't
                     // store them in UniquePtr or similar.
-                    // Templated forward declarations don't appear with an _unused field (which is what
-                    // we spot in the previous clause) but instead with an _address field.
-                    // So, solely in the case where we're storing up an error about such
-                    // a templated type, we'll also treat such cases as forward declarations.
                     //
                     // We'll also at this point check for one specific problem with
                     // forward declarations.
@@ -239,7 +234,12 @@ impl<'a> ParseBindgen<'a> {
                             Some(ErrorContext::new_for_item(s.ident.into())),
                         ));
                     }
-                    Some(UnanalyzedApi::ForwardDeclaration { name, err })
+                    let is_templated = Self::spot_field(&s.fields, "_phantom_0");
+                    Some(UnanalyzedApi::ForwardDeclaration {
+                        name,
+                        err,
+                        is_templated,
+                    })
                 } else {
                     let has_rvalue_reference_fields = s.fields.iter().any(|f| {
                         BindgenSemanticAttributes::new(&f.attrs).has_attr("rvalue_reference")
@@ -391,11 +391,10 @@ impl<'a> ParseBindgen<'a> {
     }
 
     fn spot_forward_declaration(s: &Fields) -> bool {
-        Self::spot_field(s, "_unused")
-    }
-
-    fn spot_zero_length_struct(s: &Fields) -> bool {
-        Self::spot_field(s, "_address")
+        // Non-templated forward declaration
+        Self::spot_field(s, "_unused") ||
+        // Templated forward declaration
+        (Self::spot_field(s, "_address") && Self::spot_field(s, "_phantom_0"))
     }
 
     fn spot_field(s: &Fields, desired_id: &str) -> bool {