Merge pull request #1187 from google/recursive-struct-fields

Fix problem with field circular dependencies

Author: adetaylor

engine/src/conversion/analysis/abstract_types.rs

@@ -63,6 +63,7 @@ pub(crate) fn mark_types_abstract(mut apis: ApiVec<FnPrePhase2>) -> ApiVec<FnPre
                                         kind: TypeKind::Pod | TypeKind::NonPod,
                                         castable_bases,
                                         field_deps,
+                                        field_definition_deps,
                                         field_info,
                                         is_generic,
                                         in_anonymous_namespace,
@@ -84,6 +85,7 @@ pub(crate) fn mark_types_abstract(mut apis: ApiVec<FnPrePhase2>) -> ApiVec<FnPre
                                     kind: TypeKind::Abstract,
                                     castable_bases,
                                     field_deps,
+                                    field_definition_deps,
                                     field_info,
                                     is_generic,
                                     in_anonymous_namespace,

engine/src/conversion/analysis/depth_first.rs

@@ -14,29 +14,42 @@ use itertools::Itertools;
 
 use crate::types::QualifiedName;
 
-use super::deps::HasDependencies;
+/// A little like `HasDependencies` but accounts for only direct fiele
+/// and bases.
+pub(crate) trait HasFieldsAndBases {
+    fn name(&self) -> &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<A>`, this should only return `std::unique_ptr`.
+    fn field_and_base_deps(&self) -> Box<dyn Iterator<Item = &QualifiedName> + '_>;
+}
 
-/// 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<Item = &'a T> + 'a,
 ) -> impl Iterator<Item = &'a T> {
     let queue: VecDeque<_> = inputs.collect();
     let yet_to_do = queue.iter().map(|api| api.name()).collect();
     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<Self::Item> {
         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<QualifiedName>);
 
-    impl HasDependencies for Thing {
+    impl HasFieldsAndBases for Thing {
         fn name(&self) -> &QualifiedName {
             &self.0
         }
 
-        fn deps(&self) -> Box<dyn Iterator<Item = &QualifiedName> + '_> {
+        fn field_and_base_deps(&self) -> Box<dyn Iterator<Item = &QualifiedName> + '_> {
             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"));

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<QualifiedName, ItemsFound> = HashMap::new();
 
-    for api in depth_first(apis.iter()) {
+    for api in fields_and_bases_first(apis.iter()) {
         if let Api::Struct {
             name,
             analysis:

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<FnPrePhase1> {
+    fn name(&self) -> &QualifiedName {
+        self.name()
+    }
+
+    fn field_and_base_deps(&self) -> Box<dyn Iterator<Item = &QualifiedName> + '_> {
+        match self {
+            Api::Struct {
+                analysis:
+                    PodAnalysis {
+                        field_definition_deps,
+                        bases,
+                        ..
+                    },
+                ..
+            } => Box::new(field_definition_deps.iter().chain(bases.iter())),
+            _ => Box::new(std::iter::empty()),
+        }
+    }
+}

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<QualifiedName>,
+    /// All field types. e.g. for std::unique_ptr<A>, this would include
+    /// both std::unique_ptr and A
     pub(crate) field_deps: HashSet<QualifiedName>,
+    /// Types within fields where we need a definition, e.g. for
+    /// std::unique_ptr<A> it would just be std::unique_ptr.
+    pub(crate) field_definition_deps: HashSet<QualifiedName>,
     pub(crate) field_info: Vec<FieldInfo>,
     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<QualifiedName>,
+    field_definition_deps: &mut HashSet<QualifiedName>,
     field_info: &mut Vec<FieldInfo>,
     extra_apis: &mut ApiVec<NullPhase>,
 ) -> Vec<ConvertErrorFromCpp> {
@@ -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,

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 <memory>
+        struct A {
+            std::unique_ptr<A> a;
+        };
+    "};
+    run_test("", hdr, quote! {}, &["A"], &[]);
+}
+
+#[test]
+fn test_recursive_field_indirect() {
+    let hdr = indoc! {"
+        #include <memory>
+        struct B;
+        struct A {
+            std::unique_ptr<B> a;
+        };
+        struct B {
+            std::unique_ptr<A> a1;
+            A a2;
+        };
+    "};
+    run_test("", hdr, quote! {}, &["A", "B"], &[]);
+}
+
 #[test]
 fn test_typedef_unsupported_type_pub() {
     let hdr = indoc! {"