Ensure that every item is in some module's children list

Previously, if an item's parent was not a module (eg a nested class definition
whose parent it the outer class definition) and the parent was not whitelisted
but the item was transitively whitelisted, then we could generate uses of the
item without emitting any definition for it. This could happen because we were
relying on the outer type calling for code generation on its inner types, but
that relies on us doing code generation for the outer type, which won't happen
if the outer type is not whitelisted.

This commit avoids this gotcha by ensuring that all items end up in a module's
children list, and so will be code generated even if their parent is not
whitelisted.

Fixes rust-lang#769

Author: fitzgen

src/codegen/mod.rs

@@ -435,6 +435,16 @@ impl CodeGenerator for Var {
         }
         result.saw_var(&canonical_name);
 
+        // We can't generate bindings to static variables of templates. The
+        // number of actual variables for a single declaration are open ended
+        // and we don't know what instantiations do or don't exist.
+        let type_params = item.all_template_params(ctx);
+        if let Some(params) = type_params {
+            if !params.is_empty() {
+                return;
+            }
+        }
+
         let ty = self.ty().to_rust_ty_or_opaque(ctx, &());
 
         if let Some(val) = self.val() {
@@ -759,8 +769,11 @@ impl CodeGenerator for TemplateInstantiation {
             let align = layout.align;
 
             let name = item.canonical_name(ctx);
-            let fn_name = format!("__bindgen_test_layout_{}_instantiation_{}",
-                                  name, item.exposed_id(ctx));
+            let mut fn_name = format!("__bindgen_test_layout_{}_instantiation", name);
+            let times_seen = result.overload_number(&fn_name);
+            if times_seen > 0 {
+                write!(&mut fn_name, "_{}", times_seen).unwrap();
+            }
 
             let fn_name = ctx.rust_ident_raw(&fn_name);
 

src/ir/context.rs

@@ -2,7 +2,7 @@
 
 use super::derive::{CanDeriveCopy, CanDeriveDebug, CanDeriveDefault};
 use super::int::IntKind;
-use super::item::{Item, ItemCanonicalPath, ItemSet};
+use super::item::{Item, ItemAncestors, ItemCanonicalPath, ItemSet};
 use super::item_kind::ItemKind;
 use super::module::{Module, ModuleKind};
 use super::named::{UsedTemplateParameters, analyze};
@@ -348,14 +348,29 @@ impl<'ctx> BindgenContext<'ctx> {
         let is_template_instantiation =
             is_type && item.expect_type().is_template_instantiation();
 
-        // Be sure to track all the generated children under namespace, even
-        // those generated after resolving typerefs, etc.
+        // Ensure that every item (other than the root module) is in a module's
+        // children list. This is to make sure that every whitelisted item get's
+        // codegen'd, even if its parent is not whitelisted. See issue #769 for
+        // details.
         if item.id() != item.parent_id() {
+            let mut added_as_child = false;
+
             if let Some(mut parent) = self.items.get_mut(&item.parent_id()) {
                 if let Some(mut module) = parent.as_module_mut() {
-                    module.children_mut().push(item.id());
+                    module.children_mut().insert(item.id());
+                    added_as_child = true;
                 }
             }
+
+            if !added_as_child {
+                self.items
+                    .get_mut(&self.current_module)
+                    .expect("Should always have an item for self.current_module")
+                    .as_module_mut()
+                    .expect("self.current_module should always be a module")
+                    .children_mut()
+                    .insert(item.id());
+            }
         }
 
         if is_type && item.expect_type().is_comp() {
@@ -620,33 +635,18 @@ impl<'ctx> BindgenContext<'ctx> {
                 item.parent_id()
             };
 
+            // Relocate the replacement item from where it was declared, to
+            // where the thing it is replacing was declared.
+            //
+            // First, we'll make sure that its parent id is correct.
 
-            // Reparent the item.
             let old_parent = self.resolve_item(replacement).parent_id();
-
             if new_parent == old_parent {
+                // Same parent and therefore also same containing
+                // module. Nothing to do here.
                 continue;
             }
 
-            if let Some(mut module) = self.items
-                .get_mut(&old_parent)
-                .unwrap()
-                .as_module_mut() {
-                // Deparent the replacement.
-                let position = module.children()
-                    .iter()
-                    .position(|id| *id == replacement)
-                    .unwrap();
-                module.children_mut().remove(position);
-            }
-
-            if let Some(mut module) = self.items
-                .get_mut(&new_parent)
-                .unwrap()
-                .as_module_mut() {
-                module.children_mut().push(replacement);
-            }
-
             self.items
                 .get_mut(&replacement)
                 .unwrap()
@@ -655,6 +655,49 @@ impl<'ctx> BindgenContext<'ctx> {
                 .get_mut(&id)
                 .unwrap()
                 .set_parent_for_replacement(old_parent);
+
+            // Second, make sure that it is in the correct module's children
+            // set.
+
+            let old_module = {
+                let immut_self = &*self;
+                old_parent.ancestors(immut_self)
+                    .chain(Some(immut_self.root_module))
+                    .find(|id| {
+                        let item = immut_self.resolve_item(*id);
+                        item.as_module().map_or(false, |m| m.children().contains(&replacement))
+                    })
+            };
+            let old_module = old_module.expect("Every replacement item should be in a module");
+
+            let new_module = {
+                let immut_self = &*self;
+                new_parent.ancestors(immut_self).find(|id| {
+                    immut_self.resolve_item(*id).is_module()
+                })
+            };
+            let new_module = new_module.unwrap_or(self.root_module);
+
+            if new_module == old_module {
+                // Already in the correct module.
+                continue;
+            }
+
+            self.items
+                .get_mut(&old_module)
+                .unwrap()
+                .as_module_mut()
+                .unwrap()
+                .children_mut()
+                .remove(&replacement);
+
+            self.items
+                .get_mut(&new_module)
+                .unwrap()
+                .as_module_mut()
+                .unwrap()
+                .children_mut()
+                .insert(replacement);
         }
     }
 

src/ir/module.rs

@@ -1,7 +1,8 @@
 //! Intermediate representation for modules (AKA C++ namespaces).
 
-use super::context::{BindgenContext, ItemId};
+use super::context::BindgenContext;
 use super::dot::DotAttributes;
+use super::item::ItemSet;
 use clang;
 use parse::{ClangSubItemParser, ParseError, ParseResult};
 use parse_one;
@@ -24,7 +25,7 @@ pub struct Module {
     /// The kind of module this is.
     kind: ModuleKind,
     /// The children of this module, just here for convenience.
-    children_ids: Vec<ItemId>,
+    children: ItemSet,
 }
 
 impl Module {
@@ -33,7 +34,7 @@ impl Module {
         Module {
             name: name,
             kind: kind,
-            children_ids: vec![],
+            children: ItemSet::new(),
         }
     }
 
@@ -43,13 +44,13 @@ impl Module {
     }
 
     /// Get a mutable reference to this module's children.
-    pub fn children_mut(&mut self) -> &mut Vec<ItemId> {
-        &mut self.children_ids
+    pub fn children_mut(&mut self) -> &mut ItemSet {
+        &mut self.children
     }
 
     /// Get this module's children.
-    pub fn children(&self) -> &[ItemId] {
-        &self.children_ids
+    pub fn children(&self) -> &ItemSet {
+        &self.children
     }
 
     /// Whether this namespace is inline.

tests/expectations/tests/anon_union.rs

@@ -80,7 +80,7 @@ impl Default for ErrorResult {
     fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }
 #[test]
-fn __bindgen_test_layout_TErrorResult_instantiation_1() {
+fn __bindgen_test_layout_TErrorResult_instantiation() {
     assert_eq!(::std::mem::size_of::<TErrorResult>() , 24usize , concat ! (
                "Size of template specialization: " , stringify ! (
                TErrorResult ) ));

tests/expectations/tests/class_nested.rs

@@ -78,7 +78,7 @@ extern "C" {
     pub static mut var: A_B;
 }
 #[test]
-fn __bindgen_test_layout_A_D_instantiation_1() {
+fn __bindgen_test_layout_A_D_instantiation() {
     assert_eq!(::std::mem::size_of::<A_D<::std::os::raw::c_int>>() , 4usize ,
                concat ! (
                "Size of template specialization: " , stringify ! (

tests/expectations/tests/class_with_dtor.rs

@@ -35,7 +35,7 @@ impl Default for WithoutDtor {
     fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }
 #[test]
-fn __bindgen_test_layout_HandleWithDtor_instantiation_1() {
+fn __bindgen_test_layout_HandleWithDtor_instantiation() {
     assert_eq!(::std::mem::size_of::<HandleWithDtor<::std::os::raw::c_int>>()
                , 8usize , concat ! (
                "Size of template specialization: " , stringify ! (

tests/expectations/tests/crtp.rs

@@ -51,15 +51,15 @@ impl Default for DerivedFromBaseWithDestructor {
     fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }
 #[test]
-fn __bindgen_test_layout_Base_instantiation_1() {
+fn __bindgen_test_layout_Base_instantiation() {
     assert_eq!(::std::mem::size_of::<Base>() , 1usize , concat ! (
                "Size of template specialization: " , stringify ! ( Base ) ));
     assert_eq!(::std::mem::align_of::<Base>() , 1usize , concat ! (
                "Alignment of template specialization: " , stringify ! ( Base )
                ));
 }
 #[test]
-fn __bindgen_test_layout_BaseWithDestructor_instantiation_2() {
+fn __bindgen_test_layout_BaseWithDestructor_instantiation() {
     assert_eq!(::std::mem::size_of::<BaseWithDestructor>() , 1usize , concat !
                (
                "Size of template specialization: " , stringify ! (

tests/expectations/tests/default-template-parameter.rs

@@ -16,7 +16,7 @@ impl <T, U> Default for Foo<T, U> {
     fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }
 #[test]
-fn __bindgen_test_layout_Foo_instantiation_1() {
+fn __bindgen_test_layout_Foo_instantiation() {
     assert_eq!(::std::mem::size_of::<Foo<bool, ::std::os::raw::c_int>>() ,
                8usize , concat ! (
                "Size of template specialization: " , stringify ! (

tests/expectations/tests/forward-declaration-autoptr.rs

@@ -41,3 +41,12 @@ impl Clone for Bar {
 impl Default for Bar {
     fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }
+#[test]
+fn __bindgen_test_layout_RefPtr_instantiation() {
+    assert_eq!(::std::mem::size_of::<RefPtr<Foo>>() , 8usize , concat ! (
+               "Size of template specialization: " , stringify ! ( RefPtr<Foo>
+               ) ));
+    assert_eq!(::std::mem::align_of::<RefPtr<Foo>>() , 8usize , concat ! (
+               "Alignment of template specialization: " , stringify ! (
+               RefPtr<Foo> ) ));
+}

tests/expectations/tests/gen-constructors-neg.rs

@@ -19,3 +19,7 @@ fn bindgen_test_layout_Foo() {
 impl Clone for Foo {
     fn clone(&self) -> Self { *self }
 }
+extern "C" {
+    #[link_name = "_ZN3FooC1Ei"]
+    pub fn Foo_Foo(this: *mut Foo, a: ::std::os::raw::c_int);
+}

tests/expectations/tests/gen-destructors-neg.rs

@@ -21,3 +21,7 @@ fn bindgen_test_layout_Foo() {
                 "Alignment of field: " , stringify ! ( Foo ) , "::" ,
                 stringify ! ( bar ) ));
 }
+extern "C" {
+    #[link_name = "_ZN3FooD1Ev"]
+    pub fn Foo_Foo_destructor(this: *mut Foo);
+}

tests/expectations/tests/inner_template_self.rs

@@ -36,3 +36,12 @@ impl Clone for InstantiateIt {
 impl Default for InstantiateIt {
     fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }
+#[test]
+fn __bindgen_test_layout_LinkedList_instantiation() {
+    assert_eq!(::std::mem::size_of::<LinkedList>() , 16usize , concat ! (
+               "Size of template specialization: " , stringify ! ( LinkedList
+               ) ));
+    assert_eq!(::std::mem::align_of::<LinkedList>() , 8usize , concat ! (
+               "Alignment of template specialization: " , stringify ! (
+               LinkedList ) ));
+}

tests/expectations/tests/issue-372.rs

@@ -101,4 +101,15 @@ pub mod root {
     impl Default for F {
         fn default() -> Self { unsafe { ::std::mem::zeroed() } }
     }
+    #[test]
+    fn __bindgen_test_layout_C_instantiation() {
+        assert_eq!(::std::mem::size_of::<[u64; 33usize]>() , 264usize , concat
+                   ! (
+                   "Size of template specialization: " , stringify ! (
+                   [u64; 33usize] ) ));
+        assert_eq!(::std::mem::align_of::<[u64; 33usize]>() , 8usize , concat
+                   ! (
+                   "Alignment of template specialization: " , stringify ! (
+                   [u64; 33usize] ) ));
+    }
 }

tests/expectations/tests/issue-569-non-type-template-params-causing-layout-test-failures.rs

@@ -32,7 +32,7 @@ impl Default for JS_AutoIdVector {
     fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }
 #[test]
-fn __bindgen_test_layout_JS_Base_instantiation_2() {
+fn __bindgen_test_layout_JS_Base_instantiation() {
     assert_eq!(::std::mem::size_of::<JS_Base>() , 1usize , concat ! (
                "Size of template specialization: " , stringify ! ( JS_Base )
                ));

tests/expectations/tests/issue-573-layout-test-failures.rs

@@ -28,3 +28,11 @@ fn bindgen_test_layout_AutoIdVector() {
 impl Default for AutoIdVector {
     fn default() -> Self { unsafe { ::std::mem::zeroed() } }
 }
+#[test]
+fn __bindgen_test_layout_Outer_instantiation() {
+    assert_eq!(::std::mem::size_of::<Outer>() , 1usize , concat ! (
+               "Size of template specialization: " , stringify ! ( Outer ) ));
+    assert_eq!(::std::mem::align_of::<Outer>() , 1usize , concat ! (
+               "Alignment of template specialization: " , stringify ! ( Outer
+               ) ));
+}

tests/expectations/tests/issue-574-assertion-failure-in-codegen.rs

@@ -36,3 +36,11 @@ extern "C" {
     #[link_name = "AutoIdVector"]
     pub static mut AutoIdVector: _bindgen_ty_1;
 }
+#[test]
+fn __bindgen_test_layout_a_instantiation() {
+    assert_eq!(::std::mem::size_of::<a>() , 1usize , concat ! (
+               "Size of template specialization: " , stringify ! ( a ) ));
+    assert_eq!(::std::mem::align_of::<a>() , 1usize , concat ! (
+               "Alignment of template specialization: " , stringify ! ( a )
+               ));
+}

tests/expectations/tests/issue-584-stylo-template-analysis-panic.rs

@@ -79,3 +79,11 @@ extern "C" {
     #[link_name = "_Z25Servo_Element_GetSnapshotv"]
     pub fn Servo_Element_GetSnapshot() -> A;
 }
+#[test]
+fn __bindgen_test_layout_f_instantiation() {
+    assert_eq!(::std::mem::size_of::<f>() , 1usize , concat ! (
+               "Size of template specialization: " , stringify ! ( f ) ));
+    assert_eq!(::std::mem::align_of::<f>() , 1usize , concat ! (
+               "Alignment of template specialization: " , stringify ! ( f )
+               ));
+}

tests/expectations/tests/issue-674-1.rs

@@ -43,4 +43,13 @@ pub mod root {
     impl Clone for CapturingContentInfo {
         fn clone(&self) -> Self { *self }
     }
+    #[test]
+    fn __bindgen_test_layout_Maybe_instantiation() {
+        assert_eq!(::std::mem::size_of::<u8>() , 1usize , concat ! (
+                   "Size of template specialization: " , stringify ! ( u8 )
+                   ));
+        assert_eq!(::std::mem::align_of::<u8>() , 1usize , concat ! (
+                   "Alignment of template specialization: " , stringify ! ( u8
+                   ) ));
+    }
 }