Skylark: make ConversionException an EvalException

This avoids using a RuntimeException (IllegalArgumentException)
to circumvent declaration issues, which when we were catching it too well
was hiding actual issues of RuntimeException.

--
MOS_MIGRATED_REVID=95767534

3 files changed, 35 insertions, 29 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
index af33276aa7..3c041b73c9 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BuiltinFunction.java
@@ -15,7 +15,6 @@ package com.google.devtools.build.lib.syntax;
 
 import com.google.common.base.Preconditions;
 import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.packages.Type.ConversionException;
 import com.google.devtools.build.lib.syntax.SkylarkSignatureProcessor.HackHackEitherList;
 import com.google.devtools.build.lib.syntax.SkylarkType.SkylarkFunctionType;
 
@@ -152,16 +151,14 @@ public class BuiltinFunction extends BaseFunction {
     } catch (InvocationTargetException x) {
       Throwable e = x.getCause();
       if (e instanceof EvalException) {
-        throw (EvalException) e;
+        throw ((EvalException) e).ensureLocation(loc);
       } else if (e instanceof InterruptedException) {
         throw (InterruptedException) e;
-      } else if (e instanceof ConversionException
-          || e instanceof ClassCastException
+      } else if (e instanceof ClassCastException
           || e instanceof ExecutionException
           || e instanceof IllegalStateException) {
-        throw new EvalException(loc, e);
+        throw new EvalException(loc, "in call to " + getName(), e);
       } else if (e instanceof IllegalArgumentException) {
-        // Assume it was thrown by SkylarkType.cast and has a good message.
         throw new EvalException(loc, "Illegal argument in call to " + getName(), e);
       } else {
         throw badCallException(loc, e, args);
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java
index ae9c04dd09..361ce9f178 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalException.java
@@ -32,7 +32,7 @@ import java.util.logging.Level;
  */
 public class EvalException extends Exception {
 
-  private final Location location;
+  private Location location;
   private final String message;
   private final boolean dueToIncompleteAST;
 
@@ -118,6 +118,18 @@ public class EvalException extends Exception {
   }
 
   /**
+   * Ensures that this EvalException has proper location information.
+   * Does nothing if the exception already had a location, or if no location is provided.
+   * @return this EvalException, in fluent style.
+   */
+  public EvalException ensureLocation(Location loc) {
+    if (location == null && loc != null) {
+      location = loc;
+    }
+    return this;
+  }
+
+  /**
    * A class to support a special case of EvalException when the cause of the error is an
    * Exception during a direct Java call. Allow the throwing code to provide context in a message.
    */
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
index 23275eff39..62ffcab801 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkType.java
@@ -19,10 +19,8 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.Interner;
 import com.google.common.collect.Interners;
-import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.collect.nestedset.NestedSet;
 import com.google.devtools.build.lib.events.Location;
-import com.google.devtools.build.lib.packages.Type.ConversionException;
 
 import java.lang.reflect.Method;
 import java.lang.reflect.ParameterizedType;
@@ -631,24 +629,22 @@ public abstract class SkylarkType {
 
   /** Cast a List or SkylarkList object into an Iterable of the given type. null means empty List */
   public static <TYPE> Iterable<TYPE> castList(
-      Object obj, final Class<TYPE> type, final String what) throws ConversionException {
+      Object obj, final Class<TYPE> type, final String what) throws EvalException {
     if (obj == null) {
       return ImmutableList.of();
     }
-    return Iterables.transform(com.google.devtools.build.lib.packages.Type.LIST.convert(obj, what),
-        new com.google.common.base.Function<Object, TYPE>() {
-          @Override
-          public TYPE apply(Object input) {
-            try {
-              return type.cast(input);
-            } catch (ClassCastException e) {
-              throw new IllegalArgumentException(String.format(
-                  "expected %s type for '%s' but got %s instead",
-                  EvalUtils.getDataTypeNameFromClass(type), what,
-                  EvalUtils.getDataTypeName(input)));
-            }
-          }
-    });
+    List<TYPE> results = new ArrayList<>();
+    for (Object object : com.google.devtools.build.lib.packages.Type.LIST.convert(obj, what)) {
+      try {
+        results.add(type.cast(object));
+      } catch (ClassCastException e) {
+        throw new EvalException(null, String.format(
+            "Illegal argument: expected %s type for '%s' but got %s instead",
+            EvalUtils.getDataTypeNameFromClass(type), what,
+            EvalUtils.getDataTypeName(object)));
+      }
+    }
+    return results;
   }
 
   /**
@@ -660,21 +656,22 @@ public abstract class SkylarkType {
    */
   @SuppressWarnings("unchecked")
   public static <KEY_TYPE, VALUE_TYPE> Map<KEY_TYPE, VALUE_TYPE> castMap(Object obj,
-      Class<KEY_TYPE> keyType, Class<VALUE_TYPE> valueType, String what) {
+      Class<KEY_TYPE> keyType, Class<VALUE_TYPE> valueType, String what)
+      throws EvalException {
     if (obj == null) {
       return ImmutableMap.of();
     }
     if (!(obj instanceof Map<?, ?>)) {
-      throw new IllegalArgumentException(String.format(
-          "expected a dictionary for %s but got %s instead",
+      throw new EvalException(null, String.format(
+          "Illegal argument: expected a dictionary for %s but got %s instead",
           what, EvalUtils.getDataTypeName(obj)));
     }
 
     for (Map.Entry<?, ?> input : ((Map<?, ?>) obj).entrySet()) {
       if (!keyType.isAssignableFrom(input.getKey().getClass())
           || !valueType.isAssignableFrom(input.getValue().getClass())) {
-        throw new IllegalArgumentException(String.format(
-            "expected <%s, %s> type for '%s' but got <%s, %s> instead",
+        throw new EvalException(null, String.format(
+            "Illegal argument: expected <%s, %s> type for '%s' but got <%s, %s> instead",
             keyType.getSimpleName(), valueType.getSimpleName(), what,
             EvalUtils.getDataTypeName(input.getKey()),
             EvalUtils.getDataTypeName(input.getValue())));