Raise error if we find an unknown type in native.rule().

Handle more types:

* Boolean
* TriState
* SkylarkValue (eg. FileSetEntry)
* skip Licenses, Distribs.

--
MOS_MIGRATED_REVID=112690550

src/main/java/com/google/devtools/build/lib/packages/PackageFactory.java

@@ -35,6 +35,7 @@
 import com.google.devtools.build.lib.packages.RuleFactory.BuildLangTypedAttributeValuesMap;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature;
 import com.google.devtools.build.lib.skylarkinterface.SkylarkSignature.Param;
+import com.google.devtools.build.lib.skylarkinterface.SkylarkValue;
 import com.google.devtools.build.lib.syntax.AssignmentStatement;
 import com.google.devtools.build.lib.syntax.BaseFunction;
 import com.google.devtools.build.lib.syntax.BuildFileAST;
@@ -842,7 +843,7 @@ static Map<String, Object> callGetRuleFunction(
   }
 
   @Nullable
-  private static Map<String, Object> targetDict(Target target) {
+  private static Map<String, Object> targetDict(Target target) throws NotRepresentableException {
     if (target == null && !(target instanceof Rule)) {
       return null;
     }
@@ -855,55 +856,103 @@ private static Map<String, Object> targetDict(Target target) {
         continue;
       }
 
-      Object val = skylarkifyValue(cont.getAttr(attr.getName()), target.getPackage());
-      if (val == null) {
+      if (attr.getName().equals("distribs")) {
+        // attribute distribs: cannot represent type class java.util.Collections$SingletonSet
+        // in Skylark: [INTERNAL].
         continue;
       }
-      values.put(attr.getName(), val);
+
+      try {
+        Object val = skylarkifyValue(cont.getAttr(attr.getName()), target.getPackage());
+        if (val == null) {
+          continue;
+        }
+        values.put(attr.getName(), val);
+      } catch (NotRepresentableException e) {
+        throw new NotRepresentableException(
+            String.format(
+                "target %s, attribute %s: %s", target.getName(), attr.getName(), e.getMessage()));
+      }
     }
 
     values.put("name", rule.getName());
     values.put("kind", rule.getRuleClass());
     return values;
   }
 
+  static class NotRepresentableException extends EvalException {
+    NotRepresentableException(String msg) {
+      super(null, msg);
+    }
+  };
+
   /**
    * Converts back to type that will work in BUILD and skylark,
    * such as string instead of label, SkylarkList instead of List,
    * Returns null if we don't want to export the value.
    *
    * <p>All of the types returned are immutable. If we want, we can change this to
-   * immutable in the future, but this is the safe choice for now.
+   * immutable in the future, but this is the safe choice for now.o
    */
-  private static Object skylarkifyValue(Object val, Package pkg) {
+  @Nullable
+  private static Object skylarkifyValue(Object val, Package pkg) throws NotRepresentableException {
     if (val == null) {
       return null;
     }
+    if (val instanceof Boolean) {
+      return val;
+    }
     if (val instanceof Integer) {
       return val;
     }
     if (val instanceof String) {
       return val;
     }
+
+    // Maybe we should have an interface for types so they can represent themselves to skylark?
+    if (val instanceof TriState) {
+      switch ((TriState) val) {
+        case AUTO:
+          return new Integer(-1);
+        case YES:
+          return new Integer(1);
+        case NO:
+          return new Integer(0);
+      }
+    }
+
     if (val instanceof Label) {
       Label l = (Label) val;
       if (l.getPackageName().equals(pkg.getName())) {
         return ":" + l.getName();
       }
       return l.getCanonicalForm();
     }
+
     if (val instanceof List) {
       List<Object> l = new ArrayList<>();
       for (Object o : (List) val) {
-        l.add(skylarkifyValue(o, pkg));
+        Object elt = skylarkifyValue(o, pkg);
+        if (elt == null) {
+          continue;
+        }
+
+        l.add(elt);
       }
 
       return SkylarkList.Tuple.copyOf(l);
     }
     if (val instanceof Map) {
       Map<Object, Object> m = new TreeMap<>();
       for (Map.Entry<?, ?> e : ((Map<?, ?>) val).entrySet()) {
-        m.put(skylarkifyValue(e.getKey(), pkg), skylarkifyValue(e.getValue(), pkg));
+        Object key = skylarkifyValue(e.getKey(), pkg);
+        Object mapVal = skylarkifyValue(e.getValue(), pkg);
+
+        if (key == null || mapVal == null) {
+          continue;
+        }
+
+        m.put(key, mapVal);
       }
       return m;
     }
@@ -914,12 +963,24 @@ private static Object skylarkifyValue(Object val, Package pkg) {
       return null;
     }
 
-    // Add any types we want to allow through here.
-    return null;
+    if (val instanceof SkylarkValue) {
+      return val;
+    }
+
+    if (val instanceof License) {
+      // TODO(bazel-team): convert License.getLicenseTypes() to a list of strings.
+      return null;
+    }
+
+    // We are explicit about types we don't understand so we minimize changes to existing callers
+    // if we add more types that we can represent.
+    throw new NotRepresentableException(
+        String.format(
+            "cannot represent %s (%s) in skylark", val.toString(), val.getClass().toString()));
   }
 
-  static Map callGetRulesFunction(FuncallExpression ast, Environment env) throws EvalException {
 
+  static Map callGetRulesFunction(FuncallExpression ast, Environment env) throws EvalException {
     PackageContext context = getContext(env, ast);
     Collection<Target> targets = context.pkgBuilder.getTargets();
 

src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java

@@ -198,18 +198,32 @@ public Object call(Object[] args,
     }
   }
 
+  private static String stacktraceToString(StackTraceElement[] elts) {
+    StringBuilder b = new StringBuilder();
+    for (StackTraceElement e : elts) {
+      b.append(e.toString());
+      b.append("\n");
+    }
+    return b.toString();
+  }
+
   private IllegalStateException badCallException(Location loc, Throwable e, Object... args) {
     // If this happens, it's a bug in our code.
-    return new IllegalStateException(String.format("%s%s (%s)\n"
-            + "while calling %s with args %s\nJava parameter types: %s\nSkylark type checks: %s",
+    return new IllegalStateException(
+        String.format(
+            "%s%s (%s)\n"
+                + "while calling %s with args %s\n"
+                + "Java parameter types: %s\nSkylark type checks: %s",
             (loc == null) ? "" : loc + ": ",
-            e.getClass().getName(), e.getMessage(), this,
             Arrays.asList(args),
+            e.getClass().getName(),
+            stacktraceToString(e.getStackTrace()),
+            this,
             Arrays.asList(invokeMethod.getParameterTypes()),
-            signature.getTypes()), e);
+            signature.getTypes()),
+        e);
   }
 
-
   /** Configure the reflection mechanism */
   @Override
   public void configure(SkylarkSignature annotation) {

src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java

@@ -370,7 +370,10 @@ public void testGetRule() throws Exception {
     scratch.file(
         "test/getrule/BUILD",
         "load('/test/skylark/rulestr', 'rules_dict', 'rule_dict', 'nop_rule', 'consume_rule')",
-        "genrule(name = 'a', outs = ['a.txt'], tools = [ '//test:bla' ], cmd = 'touch $@')",
+        "genrule(name = 'a', outs = ['a.txt'], ",
+        "        licenses = ['notice'],",
+        "        output_to_bindir = False,",
+        "        tools = [ '//test:bla' ], cmd = 'touch $@')",
         "nop_rule(name = 'c', x = ':a')",
         "rlist= rules_dict()",
         "consume_rule(name = 'all_str', s = [rlist['a']['kind'], rlist['a']['name'], ",
@@ -406,16 +409,21 @@ public void testGetRule() throws Exception {
             ImmutableList.<String>of(
                 "cmd",
                 "compatible_with",
+                "executable",
                 "features",
                 "generator_function",
                 "generator_location",
                 "generator_name",
+                "heuristic_label_expansion",
                 "kind",
+                "local",
                 "message",
                 "name",
+                "output_to_bindir",
                 "outs",
                 "restricted_to",
                 "srcs",
+                "stamp",
                 "tags",
                 "tools",
                 "visibility")),