From a009d0f866f0fa61bf76651b0f2679f8dbb8abc6 Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Tue, 8 Nov 2022 20:50:37 +0000 Subject: [PATCH 1/2] Add tests for circular dependencies. Test for #1186. --- integration-tests/tests/integration_test.rs | 27 +++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/integration-tests/tests/integration_test.rs b/integration-tests/tests/integration_test.rs index 245f1d08e..237cd0f2c 100644 --- a/integration-tests/tests/integration_test.rs +++ b/integration-tests/tests/integration_test.rs @@ -11309,6 +11309,33 @@ fn test_enum_in_ns() { run_test("", hdr, quote! {}, &["a::b"], &[]); } +#[test] +fn test_recursive_field() { + let hdr = indoc! {" + #include + struct A { + std::unique_ptr a; + }; + "}; + run_test("", hdr, quote! {}, &["A"], &[]); +} + +#[test] +fn test_recursive_field_indirect() { + let hdr = indoc! {" + #include + struct B; + struct A { + std::unique_ptr a; + }; + struct B { + std::unique_ptr a1; + A a2; + }; + "}; + run_test("", hdr, quote! {}, &["A", "B"], &[]); +} + #[test] fn test_typedef_unsupported_type_pub() { let hdr = indoc! {" From f68e08d2150ba14123743185736a063dce36175f Mon Sep 17 00:00:00 2001 From: Adrian Taylor Date: Tue, 8 Nov 2022 21:20:43 +0000 Subject: [PATCH 2/2] Denote only dependencies of real field types. --- .../src/conversion/analysis/abstract_types.rs | 2 + engine/src/conversion/analysis/depth_first.rs | 39 +++++++++++++------ .../analysis/fun/implicit_constructors.rs | 6 ++- engine/src/conversion/analysis/fun/mod.rs | 22 +++++++++++ engine/src/conversion/analysis/pod/mod.rs | 17 ++++++++ 5 files changed, 73 insertions(+), 13 deletions(-) diff --git a/engine/src/conversion/analysis/abstract_types.rs b/engine/src/conversion/analysis/abstract_types.rs index 4a120380b..2056697c8 100644 --- a/engine/src/conversion/analysis/abstract_types.rs +++ b/engine/src/conversion/analysis/abstract_types.rs @@ -63,6 +63,7 @@ pub(crate) fn mark_types_abstract(mut apis: ApiVec) -> ApiVec) -> ApiVec &QualifiedName; + /// Return field and base class dependencies of this item. + /// This should only include those items where a definition is required, + /// not merely a declaration. So if the field type is + /// `std::unique_ptr`, this should only return `std::unique_ptr`. + fn field_and_base_deps(&self) -> Box + '_>; +} -/// Return APIs in a depth-first order, i.e. those with no dependencies first. -pub(super) fn depth_first<'a, T: HasDependencies + Debug + 'a>( +/// Iterate through APIs in an order such that later APIs have no fields or bases +/// other than those whose types have already been processed. +pub(super) fn fields_and_bases_first<'a, T: HasFieldsAndBases + Debug + 'a>( inputs: impl Iterator + 'a, ) -> impl Iterator { let queue: VecDeque<_> = inputs.collect(); @@ -25,18 +35,21 @@ pub(super) fn depth_first<'a, T: HasDependencies + Debug + 'a>( DepthFirstIter { queue, yet_to_do } } -struct DepthFirstIter<'a, T: HasDependencies + Debug> { +struct DepthFirstIter<'a, T: HasFieldsAndBases + Debug> { queue: VecDeque<&'a T>, yet_to_do: HashSet<&'a QualifiedName>, } -impl<'a, T: HasDependencies + Debug> Iterator for DepthFirstIter<'a, T> { +impl<'a, T: HasFieldsAndBases + Debug> Iterator for DepthFirstIter<'a, T> { type Item = &'a T; fn next(&mut self) -> Option { let first_candidate = self.queue.get(0).map(|api| api.name()); while let Some(candidate) = self.queue.pop_front() { - if !candidate.deps().any(|d| self.yet_to_do.contains(&d)) { + if !candidate + .field_and_base_deps() + .any(|d| self.yet_to_do.contains(&d)) + { self.yet_to_do.remove(candidate.name()); return Some(candidate); } @@ -46,7 +59,11 @@ impl<'a, T: HasDependencies + Debug> Iterator for DepthFirstIter<'a, T> { "Failed to find a candidate; there must be a circular dependency. Queue is {}", self.queue .iter() - .map(|item| format!("{}: {}", item.name(), item.deps().join(","))) + .map(|item| format!( + "{}: {}", + item.name(), + item.field_and_base_deps().join(",") + )) .join("\n") ); } @@ -59,17 +76,17 @@ impl<'a, T: HasDependencies + Debug> Iterator for DepthFirstIter<'a, T> { mod test { use crate::types::QualifiedName; - use super::{depth_first, HasDependencies}; + use super::{fields_and_bases_first, HasFieldsAndBases}; #[derive(Debug)] struct Thing(QualifiedName, Vec); - impl HasDependencies for Thing { + impl HasFieldsAndBases for Thing { fn name(&self) -> &QualifiedName { &self.0 } - fn deps(&self) -> Box + '_> { + fn field_and_base_deps(&self) -> Box + '_> { Box::new(self.1.iter()) } } @@ -89,7 +106,7 @@ mod test { vec![QualifiedName::new_from_cpp_name("a")], ); let api_list = vec![a, b, c]; - let mut it = depth_first(api_list.iter()); + let mut it = fields_and_bases_first(api_list.iter()); assert_eq!(it.next().unwrap().0, QualifiedName::new_from_cpp_name("a")); assert_eq!(it.next().unwrap().0, QualifiedName::new_from_cpp_name("c")); assert_eq!(it.next().unwrap().0, QualifiedName::new_from_cpp_name("b")); diff --git a/engine/src/conversion/analysis/fun/implicit_constructors.rs b/engine/src/conversion/analysis/fun/implicit_constructors.rs index 8213b8070..7e19d81ba 100644 --- a/engine/src/conversion/analysis/fun/implicit_constructors.rs +++ b/engine/src/conversion/analysis/fun/implicit_constructors.rs @@ -14,7 +14,9 @@ use syn::{Type, TypeArray}; use crate::conversion::api::DeletedOrDefaulted; use crate::{ conversion::{ - analysis::{depth_first::depth_first, pod::PodAnalysis, type_converter::TypeKind}, + analysis::{ + depth_first::fields_and_bases_first, pod::PodAnalysis, type_converter::TypeKind, + }, api::{Api, ApiName, CppVisibility, FuncToConvert, SpecialMemberKind}, apivec::ApiVec, convert_error::ConvertErrorWithContext, @@ -197,7 +199,7 @@ pub(super) fn find_constructors_present( // These analyses include all bases and members of each class. let mut all_items_found: HashMap = HashMap::new(); - for api in depth_first(apis.iter()) { + for api in fields_and_bases_first(apis.iter()) { if let Api::Struct { name, analysis: diff --git a/engine/src/conversion/analysis/fun/mod.rs b/engine/src/conversion/analysis/fun/mod.rs index 5030572e4..99d48c531 100644 --- a/engine/src/conversion/analysis/fun/mod.rs +++ b/engine/src/conversion/analysis/fun/mod.rs @@ -64,6 +64,7 @@ use self::{ }; use super::{ + depth_first::HasFieldsAndBases, doc_label::make_doc_attrs, pod::{PodAnalysis, PodPhase}, tdef::TypedefAnalysis, @@ -2226,3 +2227,24 @@ fn extract_type_from_pinned_mut_ref(ty: &TypePath) -> Type { _ => panic!("did not find angle bracketed args"), } } + +impl HasFieldsAndBases for Api { + fn name(&self) -> &QualifiedName { + self.name() + } + + fn field_and_base_deps(&self) -> Box + '_> { + match self { + Api::Struct { + analysis: + PodAnalysis { + field_definition_deps, + bases, + .. + }, + .. + } => Box::new(field_definition_deps.iter().chain(bases.iter())), + _ => Box::new(std::iter::empty()), + } + } +} diff --git a/engine/src/conversion/analysis/pod/mod.rs b/engine/src/conversion/analysis/pod/mod.rs index 7aa74e85c..983a6398d 100644 --- a/engine/src/conversion/analysis/pod/mod.rs +++ b/engine/src/conversion/analysis/pod/mod.rs @@ -43,7 +43,12 @@ pub(crate) struct PodAnalysis { /// because otherwise we don't know whether they're /// abstract or not. pub(crate) castable_bases: HashSet, + /// All field types. e.g. for std::unique_ptr, this would include + /// both std::unique_ptr and A pub(crate) field_deps: HashSet, + /// Types within fields where we need a definition, e.g. for + /// std::unique_ptr it would just be std::unique_ptr. + pub(crate) field_definition_deps: HashSet, pub(crate) field_info: Vec, pub(crate) is_generic: bool, pub(crate) in_anonymous_namespace: bool, @@ -138,12 +143,14 @@ fn analyze_struct( metadata.check_for_fatal_attrs(&id)?; let bases = get_bases(&details.item); let mut field_deps = HashSet::new(); + let mut field_definition_deps = HashSet::new(); let mut field_info = Vec::new(); let field_conversion_errors = get_struct_field_types( type_converter, name.name.get_namespace(), &details.item, &mut field_deps, + &mut field_definition_deps, &mut field_info, extra_apis, ); @@ -186,6 +193,7 @@ fn analyze_struct( bases: bases.into_keys().collect(), castable_bases, field_deps, + field_definition_deps, field_info, is_generic, in_anonymous_namespace, @@ -198,6 +206,7 @@ fn get_struct_field_types( ns: &Namespace, s: &ItemStruct, field_deps: &mut HashSet, + field_definition_deps: &mut HashSet, field_info: &mut Vec, extra_apis: &mut ApiVec, ) -> Vec { @@ -225,6 +234,14 @@ fn get_struct_field_types( .unwrap_or(false) { field_deps.extend(r.types_encountered); + if let Type::Path(typ) = &r.ty { + // Later analyses need to know about the field + // types where we need full definitions, as opposed + // to just declarations. That means just the outermost + // type path. + // TODO: consider arrays. + field_definition_deps.insert(QualifiedName::from_type_path(typ)); + } field_info.push(FieldInfo { ty: r.ty, type_kind: r.kind,