diff --git a/Cargo.lock b/Cargo.lock index aa27c7ac6..d8bf9c484 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -155,8 +155,7 @@ dependencies = [ [[package]] name = "autocxx-bindgen" version = "0.71.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a7d41cf081e31a74378456586b47a5bae2b75a98e5f7c248c9d9bf433e3637f4" +source = "git+https://github.com/adetaylor/rust-bindgen?branch=hacks-for-using-all-template-params#bc3b56e2b152466340000f6430399b79afacfbe3" dependencies = [ "bitflags 2.6.0", "cexpr", diff --git a/Cargo.toml b/Cargo.toml index dfbbaaf00..ac83bbf2d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -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" } diff --git a/engine/Cargo.toml b/engine/Cargo.toml index 039f3367e..e06da546f 100644 --- a/engine/Cargo.toml +++ b/engine/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. diff --git a/engine/src/conversion/analysis/replace_hopeless_typedef_targets.rs b/engine/src/conversion/analysis/replace_hopeless_typedef_targets.rs index 918fb8f3f..1ba3b7ed0 100644 --- a/engine/src/conversion/analysis/replace_hopeless_typedef_targets.rs +++ b/engine/src/conversion/analysis/replace_hopeless_typedef_targets.rs @@ -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, }) diff --git a/engine/src/conversion/analysis/type_converter.rs b/engine/src/conversion/analysis/type_converter.rs index 40bd081a5..843d886d7 100644 --- a/engine/src/conversion/analysis/type_converter.rs +++ b/engine/src/conversion/analysis/type_converter.rs @@ -233,37 +233,14 @@ impl<'a> TypeConverter<'a> { ctx: &TypeConversionContext, ) -> Result, 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,6 +360,38 @@ 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 { + 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), @@ -390,6 +399,48 @@ impl<'a> TypeConverter<'a> { } } + 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 { + 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> + } + _ => Box::new(std::iter::empty()) as Box>, + }) + .flatten() + .collect() + } + fn convert_punctuated

( &mut self, pun: Punctuated, @@ -525,6 +576,8 @@ impl<'a> TypeConverter<'a> { fn get_templated_typename( &mut self, rs_definition: &Type, + qn: &QualifiedName, + ns: &Namespace, ) -> Result<(QualifiedName, Option), 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(api: UnanalyzedApi) -> Api { 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"), diff --git a/engine/src/conversion/api.rs b/engine/src/conversion/api.rs index d5da41c52..105ad9e34 100644 --- a/engine/src/conversion/api.rs +++ b/engine/src/conversion/api.rs @@ -466,6 +466,7 @@ pub(crate) enum Api { /// 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 { name: ApiName, rs_definition: Option>, 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. diff --git a/engine/src/conversion/codegen_rs/mod.rs b/engine/src/conversion/codegen_rs/mod.rs index 98991c67a..fefb75915 100644 --- a/engine/src/conversion/codegen_rs/mod.rs +++ b/engine/src/conversion/codegen_rs/mod.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(), } } diff --git a/engine/src/conversion/error_reporter.rs b/engine/src/conversion/error_reporter.rs index c5555a475..2470ba73d 100644 --- a/engine/src/conversion/error_reporter.rs +++ b/engine/src/conversion/error_reporter.rs @@ -94,17 +94,22 @@ pub(crate) fn convert_apis( 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, diff --git a/engine/src/conversion/parse/bindgen_semantic_attributes.rs b/engine/src/conversion/parse/bindgen_semantic_attributes.rs index f2dd7d7d7..d29a0b3b6 100644 --- a/engine/src/conversion/parse/bindgen_semantic_attributes.rs +++ b/engine/src/conversion/parse/bindgen_semantic_attributes.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())), diff --git a/engine/src/conversion/parse/parse_bindgen.rs b/engine/src/conversion/parse/parse_bindgen.rs index f49914540..fa1466c7f 100644 --- a/engine/src/conversion/parse/parse_bindgen.rs +++ b/engine/src/conversion/parse/parse_bindgen.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 { diff --git a/integration-tests/tests/integration_test.rs b/integration-tests/tests/integration_test.rs index 70457f048..3dc0e4ec6 100644 --- a/integration-tests/tests/integration_test.rs +++ b/integration-tests/tests/integration_test.rs @@ -11656,7 +11656,7 @@ fn test_typedef_unsupported_type_pub() { } #[test] -fn test_typedef_unsupported_type_pri() { +fn test_typedef_unsupported_type_pri_prob() { let hdr = indoc! {" #include namespace NS{ @@ -11678,6 +11678,36 @@ fn test_typedef_unsupported_type_pri() { ); } +#[test] +fn test_typedef_unsupported_type_pri_self_contained() { + let hdr = indoc! {" + namespace fakestd { + struct less {}; + struct allocator {}; + template + struct set { + Key key; + }; + } + namespace NS{ + class cls{ + private: + typedef fakestd::set InnerType; + }; + } + "}; + + run_test_ex( + "", + hdr, + quote! {}, + quote! { generate_ns!("NS") }, + None, + None, + None, + ); +} + #[test] fn test_array_trouble1() { let hdr = indoc! {" @@ -12387,9 +12417,17 @@ fn test_cpp_union_pod() { } #[test] +#[ignore] // https://github.com/google/autocxx/issues/1393 fn test_cpp_static_template() { let hdr = indoc! {" #pragma once + + class Boobar { + public: + static int test2() { + return 1; + } + }; template class Toto @@ -12399,6 +12437,7 @@ fn test_cpp_static_template() { { return 1; } + T t; }; "}; @@ -12407,6 +12446,8 @@ fn test_cpp_static_template() { hdr, quote! {}, quote! { + generate!("Toto") + generate!("Boobar") concrete!("Toto", TotoInt) }, None,