Auto merge of #623 - fitzgen:issue-584-stylo-blacklisting-in-template-analysis, r=emilio

Correctly handle blacklisted items in the template analysis

The template analysis operates on whitelisted items, and uses our tracing
infrastructure to move between them. Usually, that means we can only reach other
whitelisted items by tracing, because the set of whitelisted items is the
transitive closure of all the items explicitly whitelisted. The exception is
when some type is explicitly blacklisted. It could still be reachable via
tracing from a whitelisted item, but is not considered whitelisted due to the
blacklisting.

The easy fix is to run the template analysis on the whole IR graph rather than
just the whitelisted set. This is an approximately one line change in the
analysis, however is not desirable due to performance concerns. The whole point
of whitelisting is that there may be *many* types in a header, but only a *few*
the user cares about, or there might be types that aren't explicitly needed and
that are too complicated for bindgen to handle generally (often in
`<type_traits>`). In these situations, we don't want to waste cycles or even
confuse ourselves by considering such types!

Instead, we keep the whitelisted item set around and check by hand whether any
given item is in it during the template type parameter analysis.

Additionally, we make the decision that blacklisted template definitions use all
of their type parameters. This seems like a reasonable choice because the type
will likely be ported to Rust manually by the bindgen user, and they will be
looking at the C++ definition with all of its type parameters. They can always
insert `PhantomData`s manually, so it also gives the most flexibility.

Fixes #584

r? @emilio

4 files changed, 145 insertions, 14 deletions

diff --git a/src/ir/context.rs b/src/ir/context.rs
index 9b9ad8bd..d00e4899 100644
--- a/src/ir/context.rs
+++ b/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
diff --git a/src/ir/named.rs b/src/ir/named.rs
index 67b36915..1af98a26 100644
--- a/src/ir/named.rs
+++ b/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;
                     }
 
diff --git a/tests/expectations/tests/issue-584-stylo-template-analysis-panic.rs b/tests/expectations/tests/issue-584-stylo-template-analysis-panic.rs
new file mode 100644
index 00000000..91b8e49a
--- /dev/null
+++ b/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;
+}
diff --git a/tests/headers/issue-584-stylo-template-analysis-panic.hpp b/tests/headers/issue-584-stylo-template-analysis-panic.hpp
new file mode 100644
index 00000000..3bf00e5d
--- /dev/null
+++ b/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();