Correctly handle blacklisted items in the template analysis

src/ir/context.rs

@@ -631,6 +631,14 @@ impl<'ctx> BindgenContext<'ctx> {
     /// This method may only be called during the codegen phase, because the
     /// template usage information is only computed as we enter the codegen
     /// phase.
+    ///
+    /// If the item is blacklisted, then we say that it always uses the template
+    /// parameter. This is a little subtle. The template parameter usage
+    /// analysis only considers whitelisted items, and if any blacklisted item
+    /// shows up in the generated bindings, it is the user's responsibility to
+    /// manually provide a definition for them. To give them the most
+    /// flexibility when doing that, we assume that they use every template
+    /// parameter and always pass template arguments through in instantiations.
     pub fn uses_template_parameter(&self,
                                    item: ItemId,
                                    template_param: ItemId)
@@ -643,7 +651,7 @@ impl<'ctx> BindgenContext<'ctx> {
             .expect("should have found template parameter usage if we're in codegen")
             .get(&item)
             .map(|items_used_params| items_used_params.contains(&template_param))
-            .unwrap_or(false)
+            .unwrap_or_else(|| self.resolve_item(item).is_hidden(self))
     }
 
     // This deserves a comment. Builtin types don't get a valid declaration, so

src/ir/named.rs

@@ -131,7 +131,7 @@ use super::item::ItemSet;
 use super::template::AsNamed;
 use super::traversal::{EdgeKind, Trace};
 use super::ty::{TemplateDeclaration, TypeKind};
-use std::collections::HashMap;
+use std::collections::{HashMap, HashSet};
 use std::fmt;
 
 /// An analysis in the monotone framework.
@@ -263,6 +263,8 @@ pub struct UsedTemplateParameters<'ctx, 'gen>
     used: HashMap<ItemId, Option<ItemSet>>,
 
     dependencies: HashMap<ItemId, Vec<ItemId>>,
+
+    whitelisted_items: HashSet<ItemId>,
 }
 
 impl<'ctx, 'gen> UsedTemplateParameters<'ctx, 'gen> {
@@ -316,21 +318,30 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> {
            -> UsedTemplateParameters<'ctx, 'gen> {
         let mut used = HashMap::new();
         let mut dependencies = HashMap::new();
+        let whitelisted_items: HashSet<_> = ctx.whitelisted_items().collect();
 
-        for item in ctx.whitelisted_items() {
+        for item in whitelisted_items.iter().cloned() {
             dependencies.entry(item).or_insert(vec![]);
             used.insert(item, Some(ItemSet::new()));
 
             {
                 // We reverse our natural IR graph edges to find dependencies
                 // between nodes.
-                item.trace(ctx,
-                           &mut |sub_item, _| {
-                               dependencies.entry(sub_item)
-                                   .or_insert(vec![])
-                                   .push(item);
-                           },
-                           &());
+                item.trace(ctx, &mut |sub_item, _| {
+                    // We won't be generating code for items that aren't
+                    // whitelisted, so don't bother keeping track of their
+                    // template parameters. But isn't whitelisting the
+                    // transitive closure of reachable items from the explicitly
+                    // whitelisted items? Usually! The exception is explicitly
+                    // blacklisted items.
+                    if !whitelisted_items.contains(&sub_item) {
+                        return;
+                    }
+
+                    dependencies.entry(sub_item)
+                        .or_insert(vec![])
+                        .push(item);
+                }, &());
             }
 
             // Additionally, whether a template instantiation's template
@@ -361,6 +372,7 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> {
             ctx: ctx,
             used: used,
             dependencies: dependencies,
+            whitelisted_items: whitelisted_items,
         }
     }
 
@@ -395,6 +407,19 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> {
                 used_by_this_id.insert(id);
             }
 
+            // We say that blacklisted items use all of their template
+            // parameters. The blacklisted type is most likely implemented
+            // explicitly by the user, since it won't be in the generated
+            // bindings, and we don't know exactly what they'll to with template
+            // parameters, but we can push the issue down the line to them.
+            Some(&TypeKind::TemplateInstantiation(ref inst))
+                if !self.whitelisted_items.contains(&inst.template_definition()) => {
+                    let args = inst.template_arguments()
+                        .iter()
+                        .filter_map(|a| a.as_named(self.ctx, &()));
+                    used_by_this_id.extend(args);
+                }
+
             // A template instantiation's concrete template argument is
             // only used if the template declaration uses the
             // corresponding template parameter.
@@ -404,12 +429,12 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> {
 
                 let params = decl.self_template_params(self.ctx)
                     .unwrap_or(vec![]);
+
                 for (arg, param) in args.iter().zip(params.iter()) {
-                    let used_by_definition = self.used
-                                                 [&inst.template_definition()]
+                    let used_by_def = self.used[&inst.template_definition()]
                         .as_ref()
                         .unwrap();
-                    if used_by_definition.contains(param) {
+                    if used_by_def.contains(param) {
                         if let Some(named) = arg.as_named(self.ctx, &()) {
                             used_by_this_id.insert(named);
                         }
@@ -421,7 +446,12 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> {
             // parameter usage.
             _ => {
                 item.trace(self.ctx, &mut |sub_id, edge_kind| {
-                    if sub_id == id || !Self::consider_edge(edge_kind) {
+                    // Ignore ourselves, since union with ourself is a
+                    // no-op. Ignore edges that aren't relevant to the
+                    // analysis. Ignore edges to blacklisted items.
+                    if sub_id == id ||
+                       !Self::consider_edge(edge_kind) ||
+                       !self.whitelisted_items.contains(&sub_id) {
                         return;
                     }
 

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

@@ -0,0 +1,80 @@
+/* automatically generated by rust-bindgen */
+
+
+#![allow(non_snake_case)]
+
+pub type RefPtr<T> = T;
+
+#[repr(C)]
+#[derive(Debug, Copy)]
+pub struct b {
+    pub _base: g,
+}
+#[test]
+fn bindgen_test_layout_b() {
+    assert_eq!(::std::mem::size_of::<b>() , 1usize , concat ! (
+               "Size of: " , stringify ! ( b ) ));
+    assert_eq! (::std::mem::align_of::<b>() , 1usize , concat ! (
+                "Alignment of " , stringify ! ( b ) ));
+}
+impl Clone for b {
+    fn clone(&self) -> Self { *self }
+}
+impl Default for b {
+    fn default() -> Self { unsafe { ::std::mem::zeroed() } }
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy)]
+pub struct A {
+    pub _address: u8,
+}
+pub type A_a = b;
+#[test]
+fn bindgen_test_layout_A() {
+    assert_eq!(::std::mem::size_of::<A>() , 1usize , concat ! (
+               "Size of: " , stringify ! ( A ) ));
+    assert_eq! (::std::mem::align_of::<A>() , 1usize , concat ! (
+                "Alignment of " , stringify ! ( A ) ));
+}
+impl Clone for A {
+    fn clone(&self) -> Self { *self }
+}
+#[repr(C)]
+#[derive(Debug, Copy, Clone)]
+pub struct e<c> {
+    pub d: RefPtr<c>,
+}
+impl <c> Default for e<c> {
+    fn default() -> Self { unsafe { ::std::mem::zeroed() } }
+}
+#[repr(C)]
+#[derive(Debug, Default, Copy, Clone)]
+pub struct f {
+    pub _address: u8,
+}
+#[repr(C)]
+#[derive(Debug, Copy)]
+pub struct g {
+    pub h: f,
+}
+#[test]
+fn bindgen_test_layout_g() {
+    assert_eq!(::std::mem::size_of::<g>() , 1usize , concat ! (
+               "Size of: " , stringify ! ( g ) ));
+    assert_eq! (::std::mem::align_of::<g>() , 1usize , concat ! (
+                "Alignment of " , stringify ! ( g ) ));
+    assert_eq! (unsafe { & ( * ( 0 as * const g ) ) . h as * const _ as usize
+                } , 0usize , concat ! (
+                "Alignment of field: " , stringify ! ( g ) , "::" , stringify
+                ! ( h ) ));
+}
+impl Clone for g {
+    fn clone(&self) -> Self { *self }
+}
+impl Default for g {
+    fn default() -> Self { unsafe { ::std::mem::zeroed() } }
+}
+extern "C" {
+    #[link_name = "_Z25Servo_Element_GetSnapshotv"]
+    pub fn Servo_Element_GetSnapshot() -> A;
+}

tests/headers/issue-584-stylo-template-analysis-panic.hpp

@@ -0,0 +1,13 @@
+// bindgen-flags: --blacklist-type RefPtr --whitelist-function 'Servo_.*' --raw-line 'pub type RefPtr<T> = T;' -- -std=c++14
+template <class> class RefPtr;
+class b;
+class A {
+  typedef b a;
+};
+template <class c> class e { RefPtr<c> d; };
+template <class> class f {};
+class g {
+  f<e<int>> h;
+};
+class b : g {};
+A Servo_Element_GetSnapshot();