Auto merge of #770 - fitzgen:issue-769-bad-instantiation-test, r=emilio

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.

This does have the downside of changing the relative order of some of the emitted code, and so this has a big diff (as will the next bindgen update for downstream dependencies) but I actually think the newer order makes more sense, for what that is worth.

Fixes #769

r? @emilio

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() {
@@ -752,15 +762,25 @@ impl CodeGenerator for TemplateInstantiation {
             return
         }
 
+        // If there are any unbound type parameters, then we can't generate a
+        // layout test because we aren't dealing with a concrete type with a
+        // concrete size and alignment.
+        if ctx.uses_any_template_parameters(item.id()) {
+            return;
+        }
+
         let layout = item.kind().expect_type().layout(ctx);
 
         if let Some(layout) = layout {
             let size = layout.size;
             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);
 
@@ -2935,6 +2955,17 @@ impl CodeGenerator for Function {
                    item: &Item) {
         debug!("<Function as CodeGenerator>::codegen: item = {:?}", item);
 
+        // Similar to static member variables in a class template, we can't
+        // generate bindings to template functions, because the set of
+        // instantiations is open ended and we have no way of knowing which
+        // monomorphizations actually exist.
+        let type_params = item.all_template_params(ctx);
+        if let Some(params) = type_params {
+            if !params.is_empty() {
+                return;
+            }
+        }
+
         let name = self.name();
         let mut canonical_name = item.canonical_name(ctx);
         let mangled_name = self.mangled_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,8 @@ 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.
-        if item.id() != item.parent_id() {
-            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());
-                }
-            }
+        if item.id() != self.root_module {
+            self.add_item_to_module(&item);
         }
 
         if is_type && item.expect_type().is_comp() {
@@ -407,6 +401,38 @@ impl<'ctx> BindgenContext<'ctx> {
         }
     }
 
+    /// 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.
+    fn add_item_to_module(&mut self, item: &Item) {
+        assert!(item.id() != self.root_module);
+        assert!(!self.items.contains_key(&item.id()));
+
+        if let Some(mut parent) = self.items.get_mut(&item.parent_id()) {
+            if let Some(mut module) = parent.as_module_mut() {
+                debug!("add_item_to_module: adding {:?} as child of parent module {:?}",
+                       item.id(),
+                       item.parent_id());
+
+                module.children_mut().insert(item.id());
+                return;
+            }
+        }
+
+        debug!("add_item_to_module: adding {:?} as child of current module {:?}",
+               item.id(),
+               self.current_module);
+
+        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());
+    }
+
     /// Add a new named template type parameter to this context's item set.
     pub fn add_named_type(&mut self, item: Item, definition: clang::Cursor) {
         debug!("BindgenContext::add_named_type: item = {:?}; definition = {:?}",
@@ -418,6 +444,8 @@ impl<'ctx> BindgenContext<'ctx> {
         assert_eq!(definition.kind(),
                    clang_sys::CXCursor_TemplateTypeParameter);
 
+        self.add_item_to_module(&item);
+
         let id = item.id();
         let old_item = self.items.insert(id, item);
         assert!(old_item.is_none(),
@@ -620,41 +648,65 @@ 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)
+            self.items
+                .get_mut(&replacement)
                 .unwrap()
-                .as_module_mut() {
-                // Deparent the replacement.
-                let position = module.children()
-                    .iter()
-                    .position(|id| *id == replacement)
-                    .unwrap();
-                module.children_mut().remove(position);
-            }
+                .set_parent_for_replacement(new_parent);
 
-            if let Some(mut module) = self.items
-                .get_mut(&new_parent)
-                .unwrap()
-                .as_module_mut() {
-                module.children_mut().push(replacement);
+            // 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(&replacement)
+                .get_mut(&old_module)
                 .unwrap()
-                .set_parent_for_replacement(new_parent);
+                .as_module_mut()
+                .unwrap()
+                .children_mut()
+                .remove(&replacement);
+
             self.items
-                .get_mut(&id)
+                .get_mut(&new_module)
                 .unwrap()
-                .set_parent_for_replacement(old_parent);
+                .as_module_mut()
+                .unwrap()
+                .children_mut()
+                .insert(replacement);
         }
     }
 
@@ -697,6 +749,11 @@ impl<'ctx> BindgenContext<'ctx> {
             self.process_replacements();
         }
 
+        // Make sure to do this after processing replacements, since that messes
+        // with the parentage and module children, and we want to assert that it
+        // messes with them correctly.
+        self.assert_every_item_in_a_module();
+
         self.find_used_template_parameters();
 
         let ret = cb(self);
@@ -727,6 +784,42 @@ impl<'ctx> BindgenContext<'ctx> {
                                                        traversal::all_edges)
     }
 
+    /// When the `testing_only_extra_assertions` feature is enabled, walk over
+    /// every item and ensure that it is in the children set of one of its
+    /// module ancestors.
+    fn assert_every_item_in_a_module(&self) {
+        if cfg!(feature = "testing_only_extra_assertions") {
+            assert!(self.in_codegen_phase());
+            assert!(self.current_module == self.root_module);
+
+            for (&id, _item) in self.items() {
+                if id == self.root_module {
+                    continue;
+                }
+
+                assert!(
+                    {
+                        let id = id.into_resolver()
+                            .through_type_refs()
+                            .through_type_aliases()
+                            .resolve(self)
+                            .id();
+                        id.ancestors(self)
+                            .chain(Some(self.root_module))
+                            .any(|ancestor| {
+                                debug!("Checking if {:?} is a child of {:?}", id, ancestor);
+                                self.resolve_item(ancestor)
+                                    .as_module()
+                                    .map_or(false, |m| m.children().contains(&id))
+                            })
+                    },
+                    "{:?} should be in some ancestor module's children set",
+                    id
+                );
+            }
+        }
+    }
+
     fn find_used_template_parameters(&mut self) {
         if self.options.whitelist_recursively {
             let used_params = analyze::<UsedTemplateParameters>(self);
@@ -783,6 +876,21 @@ impl<'ctx> BindgenContext<'ctx> {
             .map_or(false, |items_used_params| items_used_params.contains(&template_param))
     }
 
+    /// Return `true` if `item` uses any unbound, generic template parameters,
+    /// `false` otherwise.
+    ///
+    /// Has the same restrictions that `uses_template_parameter` has.
+    pub fn uses_any_template_parameters(&self, item: ItemId) -> bool {
+        assert!(self.in_codegen_phase(),
+                "We only compute template parameter usage as we enter codegen");
+
+        self.used_template_parameters
+            .as_ref()
+            .expect("should have template parameter usage info in codegen phase")
+            .get(&item)
+            .map_or(false, |used| !used.is_empty())
+    }
+
     // This deserves a comment. Builtin types don't get a valid declaration, so
     // we can't add it to the cursor->type map.
     //
@@ -794,6 +902,7 @@ impl<'ctx> BindgenContext<'ctx> {
     fn add_builtin_item(&mut self, item: Item) {
         debug!("add_builtin_item: item = {:?}", item);
         debug_assert!(item.kind().is_type());
+        self.add_item_to_module(&item);
         let id = item.id();
         let old_item = self.items.insert(id, item);
         assert!(old_item.is_none(), "Inserted type twice?");
@@ -929,10 +1038,14 @@ impl<'ctx> BindgenContext<'ctx> {
     ///   Incomplete<U> bar;
     /// };
     /// ```
+    ///
+    /// Finally, template instantiations are always children of the current
+    /// module. They use their template's definition for their name, so the
+    /// parent is only useful for ensuring that their layout tests get
+    /// codegen'd.
     fn instantiate_template(&mut self,
                             with_id: ItemId,
                             template: ItemId,
-                            parent_id: ItemId,
                             ty: &clang::Type,
                             location: clang::Cursor)
                             -> Option<ItemId> {
@@ -1038,13 +1151,14 @@ impl<'ctx> BindgenContext<'ctx> {
                         let sub_item = Item::new(sub_id,
                                                  None,
                                                  None,
-                                                 template_decl_id,
+                                                 self.current_module,
                                                  ItemKind::Type(sub_ty));
 
                         // Bypass all the validations in add_item explicitly.
                         debug!("instantiate_template: inserting nested \
                                 instantiation item: {:?}",
                                sub_item);
+                        self.add_item_to_module(&sub_item);
                         debug_assert!(sub_id == sub_item.id());
                         self.items.insert(sub_id, sub_item);
                         args.push(sub_id);
@@ -1086,10 +1200,11 @@ impl<'ctx> BindgenContext<'ctx> {
                            type_kind,
                            ty.is_const());
         let item =
-            Item::new(with_id, None, None, parent_id, ItemKind::Type(ty));
+            Item::new(with_id, None, None, self.current_module, ItemKind::Type(ty));
 
         // Bypass all the validations in add_item explicitly.
         debug!("instantiate_template: inserting item: {:?}", item);
+        self.add_item_to_module(&item);
         debug_assert!(with_id == item.id());
         self.items.insert(with_id, item);
         Some(with_id)
@@ -1143,11 +1258,6 @@ impl<'ctx> BindgenContext<'ctx> {
                    location.is_some() {
                     let location = location.unwrap();
 
-                    // It is always safe to hang instantiations off of the root
-                    // module. They use their template definition for naming,
-                    // and don't need the parent for anything else.
-                    let parent_id = self.root_module();
-
                     // For specialized type aliases, there's no way to get the
                     // template parameters as of this writing (for a struct
                     // specialization we wouldn't be in this branch anyway).
@@ -1166,7 +1276,6 @@ impl<'ctx> BindgenContext<'ctx> {
 
                     return self.instantiate_template(with_id,
                                                      id,
-                                                     parent_id,
                                                      ty,
                                                      location)
                         .or_else(|| Some(id));

src/ir/dot.rs

@@ -55,6 +55,15 @@ pub fn write_dot_file<P>(ctx: &BindgenContext, path: P) -> io::Result<()>
         if let Some(err) = err {
             return err;
         }
+
+        if let Some(module) = item.as_module() {
+            for child in module.children() {
+                try!(writeln!(&mut dot_file,
+                              "{} -> {} [style=dotted]",
+                              item.id().as_usize(),
+                              child.as_usize()));
+            }
+        }
     }
 
     try!(writeln!(&mut dot_file, "}}"));