Auto merge of #633 - fitzgen:stylo-stuff, r=emilio

Finish fixing blacklisting + template analysis

This is a follow up to c8a206a, and the support for blacklisting in the named template parameter usage analysis. This ensures that ever item we ever call `constrain` on has an entry in `used` for the set of template parameters it uses. Additionally, it adds extra assertions to enforce the invariant.

We cannot completely avoid analyzing blacklisted items because we want to consider all of a blacklisted template's parameters as used. This is why we ensure that blacklisted items have a used template parameter set rather than ensuring that blacklisted items never end up in the worklist.

This fixes the panic I saw in servo/servo#16392, but there are still issues leftover in the resulting bindings that I am tracking down.

r? @emilio

Author: bors-servo

src/ir/named.rs

@@ -322,12 +322,14 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> {
 
         for item in whitelisted_items.iter().cloned() {
             dependencies.entry(item).or_insert(vec![]);
-            used.insert(item, Some(ItemSet::new()));
+            used.entry(item).or_insert(Some(ItemSet::new()));
 
             {
                 // We reverse our natural IR graph edges to find dependencies
                 // between nodes.
                 item.trace(ctx, &mut |sub_item, _| {
+                    used.entry(sub_item).or_insert(Some(ItemSet::new()));
+
                     // 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
@@ -353,12 +355,17 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> {
                     &TypeKind::TemplateInstantiation(ref inst) => {
                         let decl = ctx.resolve_type(inst.template_definition());
                         let args = inst.template_arguments();
+
                         // Although template definitions should always have
                         // template parameters, there is a single exception:
                         // opaque templates. Hence the unwrap_or.
                         let params = decl.self_template_params(ctx)
                             .unwrap_or(vec![]);
+
                         for (arg, param) in args.iter().zip(params.iter()) {
+                            used.entry(*arg).or_insert(Some(ItemSet::new()));
+                            used.entry(*param).or_insert(Some(ItemSet::new()));
+
                             dependencies.entry(*arg)
                                 .or_insert(vec![])
                                 .push(*param);
@@ -368,6 +375,28 @@ impl<'ctx, 'gen> MonotoneFramework for UsedTemplateParameters<'ctx, 'gen> {
                 });
         }
 
+        if cfg!(feature = "testing_only_extra_assertions") {
+            // Invariant: The `used` map has an entry for every whitelisted
+            // item, as well as all explicitly blacklisted items that are
+            // reachable from whitelisted items.
+            //
+            // (This is so that every item we call `constrain` on is guaranteed
+            // to have a set of template parameters, and we can allow
+            // blacklisted templates to use all of their parameters).
+            for item in whitelisted_items.iter() {
+                extra_assert!(used.contains_key(item));
+                item.trace(ctx, &mut |sub_item, _| {
+                    extra_assert!(used.contains_key(&sub_item));
+                }, &())
+            }
+
+            // Invariant: the `dependencies` map has an entry for every
+            // whitelisted item.
+            for item in whitelisted_items.iter() {
+                extra_assert!(dependencies.contains_key(item));
+            }
+        }
+
         UsedTemplateParameters {
             ctx: ctx,
             used: used,

src/lib.rs

@@ -235,11 +235,11 @@ impl Builder {
         if !self.options.whitelist_recursively {
             output_vector.push("--no-recursive-whitelist".into());
         }
-  
+
         if self.options.objc_extern_crate {
             output_vector.push("--objc-extern-crate".into());
         }
-    
+
         if self.options.builtins {
             output_vector.push("--builtins".into());
         }
@@ -249,15 +249,6 @@ impl Builder {
             output_vector.push(prefix.clone());
         }
 
-        self.options
-            .clang_args
-            .iter()
-            .map(|item| {
-                     output_vector.push("--clang-args".into());
-                     output_vector.push(item.trim_left_matches("^").trim_right_matches("$").into());
-                 })
-            .count();
-
         if let Some(ref dummy) = self.options.dummy_uses {
             output_vector.push("--dummy-uses".into());
             output_vector.push(dummy.clone());
@@ -316,7 +307,7 @@ impl Builder {
         if self.options.codegen_config.destructors {
             options.push("destructors".into());
         }
-        
+
         output_vector.push(options.join(","));
 
         if !self.options.codegen_config.methods {
@@ -410,6 +401,18 @@ impl Builder {
                  })
             .count();
 
+        if !self.options.clang_args.is_empty() {
+            output_vector.push("--".into());
+            self.options
+                .clang_args
+                .iter()
+                .cloned()
+                .map(|item| {
+                    output_vector.push(item);
+                })
+                .count();
+        }
+
         output_vector
     }
 
@@ -1243,7 +1246,7 @@ fn commandline_flag_unit_test_function() {
 
     assert!(test_cases.iter().all(|ref x| command_line_flags.contains(x)) );
 
-    //Test 2 
+    //Test 2
     let bindings = ::builder().header("input_header")
                               .whitelisted_type("Distinct_Type")
                               .whitelisted_function("safe_function");