Clean up exception handling in DotExpression and FuncallExpression.

Factor the exception handling where possible.
Also, don't print the exception name in error messages.

--
MOS_MIGRATED_REVID=94083727

3 files changed, 83 insertions, 91 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
index 9cd37e7369..0b3271ef49 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/DotExpression.java
@@ -17,9 +17,7 @@ import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.events.Location;
 import com.google.devtools.build.lib.syntax.FuncallExpression.MethodDescriptor;
 
-import java.lang.reflect.InvocationTargetException;
 import java.util.List;
-import java.util.concurrent.ExecutionException;
 
 /**
  * Syntax node for a dot expression.
@@ -71,29 +69,23 @@ public final class DotExpression extends Expression {
    * Returns the field of the given name of the struct objValue, or null if no such field exists.
    */
   public static Object eval(Object objValue, String name, Location loc) throws EvalException {
-    Object result = null;
     if (objValue instanceof ClassObject) {
-      result = ((ClassObject) objValue).getValue(name);
+      Object result = ((ClassObject) objValue).getValue(name);
       result = SkylarkType.convertToSkylark(result, loc);
       // If we access NestedSets using ClassObject.getValue() we won't know the generic type,
       // so we have to disable it. This should not happen.
       SkylarkType.checkTypeAllowedInSkylark(result, loc);
-    } else {
-      try {
-        List<MethodDescriptor> methods = FuncallExpression.getMethods(objValue.getClass(), name, 0);
-        if (methods != null && !methods.isEmpty()) {
-          MethodDescriptor method = Iterables.getOnlyElement(methods);
-          if (method.getAnnotation().structField()) {
-            result = FuncallExpression.callMethod(
-                method, name, objValue, new Object[] {}, loc);
-          }
-        }
-      } catch (ExecutionException | IllegalAccessException | InvocationTargetException e) {
-        throw new EvalException(loc, "Method invocation failed: " + e);
+      return result;
+    }
+    List<MethodDescriptor> methods = FuncallExpression.getMethods(objValue.getClass(),
+          name, 0, loc);
+    if (methods != null && !methods.isEmpty()) {
+      MethodDescriptor method = Iterables.getOnlyElement(methods);
+      if (method.getAnnotation().structField()) {
+        return FuncallExpression.callMethod(method, name, objValue, new Object[] {}, loc);
       }
     }
-
-    return result;
+    return null;
   }
 
   @Override
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 8a8ff9e3ef..ae9c04dd09 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
@@ -89,8 +89,7 @@ public class EvalException extends Exception {
    * Returns the error message with location info if exists.
    */
   public String print() { // TODO(bazel-team): do we also need a toString() method?
-    return this.getClass().getName()
-        + (getLocation() == null ? "" : " at " + getLocation()) + ": "
+    return (getLocation() == null ? "" : getLocation()) + ": "
         + (message == null ? "" : message + "\n")
         + (dueToIncompleteAST ? "due to incomplete AST\n" : "")
         + (getCause() != null && getCause().getMessage() != null ? getCause().getMessage() : "");
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
index 7bdc0c0323..5f2c512252 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FuncallExpression.java
@@ -283,9 +283,13 @@ public final class FuncallExpression extends Expression {
    * Returns the list of Skylark callable Methods of objClass with the given name
    * and argument number.
    */
-  public static List<MethodDescriptor> getMethods(Class<?> objClass, String methodName, int argNum)
-      throws ExecutionException {
-    return methodCache.get(objClass).get(methodName + "#" + argNum);
+  public static List<MethodDescriptor> getMethods(Class<?> objClass, String methodName, int argNum,
+      Location loc) throws EvalException {
+    try {
+      return methodCache.get(objClass).get(methodName + "#" + argNum);
+    } catch (ExecutionException e) {
+      throw new EvalException(loc, "Method invocation failed: " + e);
+    }
   }
 
   /**
@@ -308,34 +312,48 @@ public final class FuncallExpression extends Expression {
   }
 
   static Object callMethod(MethodDescriptor methodDescriptor, String methodName, Object obj,
-      Object[] args, Location loc) throws EvalException, IllegalAccessException,
-      IllegalArgumentException, InvocationTargetException {
-    Method method = methodDescriptor.getMethod();
-    if (obj == null && !Modifier.isStatic(method.getModifiers())) {
-      throw new EvalException(loc, "Method '" + methodName + "' is not static");
-    }
-    // This happens when the interface is public but the implementation classes
-    // have reduced visibility.
-    method.setAccessible(true);
-    Object result = method.invoke(obj, args);
-    if (method.getReturnType().equals(Void.TYPE)) {
-      return Environment.NONE;
-    }
-    if (result == null) {
-      if (methodDescriptor.getAnnotation().allowReturnNones()) {
+      Object[] args, Location loc) throws EvalException {
+    try {
+      Method method = methodDescriptor.getMethod();
+      if (obj == null && !Modifier.isStatic(method.getModifiers())) {
+        throw new EvalException(loc, "Method '" + methodName + "' is not static");
+      }
+      // This happens when the interface is public but the implementation classes
+      // have reduced visibility.
+      method.setAccessible(true);
+      Object result = method.invoke(obj, args);
+      if (method.getReturnType().equals(Void.TYPE)) {
         return Environment.NONE;
+      }
+      if (result == null) {
+        if (methodDescriptor.getAnnotation().allowReturnNones()) {
+          return Environment.NONE;
+        } else {
+          throw new EvalException(loc,
+              "Method invocation returned None, please contact Skylark developers: " + methodName
+              + "(" + EvalUtils.prettyPrintValues(", ", ImmutableList.copyOf(args))  + ")");
+        }
+      }
+      result = SkylarkType.convertToSkylark(result, method);
+      if (result != null && !EvalUtils.isSkylarkImmutable(result.getClass())) {
+        throw new EvalException(loc, "Method '" + methodName
+            + "' returns a mutable object (type of " + EvalUtils.getDataTypeName(result) + ")");
+      }
+      return result;
+    } catch (IllegalAccessException e) {
+      // TODO(bazel-team): Print a nice error message. Maybe the method exists
+      // and an argument is missing or has the wrong type.
+      throw new EvalException(loc, "Method invocation failed: " + e);
+    } catch (InvocationTargetException e) {
+      if (e.getCause() instanceof FuncallException) {
+        throw new EvalException(loc, e.getCause().getMessage());
+      } else if (e.getCause() != null) {
+        throw new EvalExceptionWithJavaCause(loc, e.getCause());
       } else {
-        throw new EvalException(loc,
-            "Method invocation returned None, please contact Skylark developers: " + methodName
-          + "(" + EvalUtils.prettyPrintValues(", ", ImmutableList.copyOf(args))  + ")");
+        // This is unlikely to happen
+        throw new EvalException(loc, "Method invocation failed: " + e);
       }
     }
-    result = SkylarkType.convertToSkylark(result, method);
-    if (result != null && !EvalUtils.isSkylarkImmutable(result.getClass())) {
-      throw new EvalException(loc, "Method '" + methodName
-          + "' returns a mutable object (type of " + EvalUtils.getDataTypeName(result) + ")");
-    }
-    return result;
   }
 
   // TODO(bazel-team): If there's exactly one usable method, this works. If there are multiple
@@ -344,54 +362,37 @@ public final class FuncallExpression extends Expression {
   // TODO(bazel-team): check if this and SkylarkBuiltInFunctions.createObject can be merged.
   private Object invokeJavaMethod(
       Object obj, Class<?> objClass, String methodName, List<Object> args) throws EvalException {
-    try {
-      MethodDescriptor matchingMethod = null;
-      List<MethodDescriptor> methods = getMethods(objClass, methodName, args.size());
-      if (methods != null) {
-        for (MethodDescriptor method : methods) {
-          Class<?>[] params = method.getMethod().getParameterTypes();
-          int i = 0;
-          boolean matching = true;
-          for (Class<?> param : params) {
-            if (!param.isAssignableFrom(args.get(i).getClass())) {
-              matching = false;
-              break;
-            }
-            i++;
+    MethodDescriptor matchingMethod = null;
+    List<MethodDescriptor> methods = getMethods(objClass, methodName, args.size(), getLocation());
+    if (methods != null) {
+      for (MethodDescriptor method : methods) {
+        Class<?>[] params = method.getMethod().getParameterTypes();
+        int i = 0;
+        boolean matching = true;
+        for (Class<?> param : params) {
+          if (!param.isAssignableFrom(args.get(i).getClass())) {
+            matching = false;
+            break;
           }
-          if (matching) {
-            if (matchingMethod == null) {
-              matchingMethod = method;
-            } else {
-              throw new EvalException(func.getLocation(),
-                  "Multiple matching methods for " + formatMethod(methodName, args)
-                  + " in " + EvalUtils.getDataTypeNameFromClass(objClass));
-            }
+          i++;
+        }
+        if (matching) {
+          if (matchingMethod == null) {
+            matchingMethod = method;
+          } else {
+            throw new EvalException(func.getLocation(),
+                "Multiple matching methods for " + formatMethod(methodName, args)
+                + " in " + EvalUtils.getDataTypeNameFromClass(objClass));
           }
         }
       }
-      if (matchingMethod != null && !matchingMethod.getAnnotation().structField()) {
-        return callMethod(matchingMethod, methodName, obj, args.toArray(), getLocation());
-      } else {
-        throw new EvalException(getLocation(), "No matching method found for "
-            + formatMethod(methodName, args) + " in "
-            + EvalUtils.getDataTypeNameFromClass(objClass));
-      }
-    } catch (IllegalAccessException e) {
-      // TODO(bazel-team): Print a nice error message. Maybe the method exists
-      // and an argument is missing or has the wrong type.
-      throw new EvalException(getLocation(), "Method invocation failed: " + e);
-    } catch (InvocationTargetException e) {
-      if (e.getCause() instanceof FuncallException) {
-        throw new EvalException(getLocation(), e.getCause().getMessage());
-      } else if (e.getCause() != null) {
-        throw new EvalExceptionWithJavaCause(getLocation(), e.getCause());
-      } else {
-        // This is unlikely to happen
-        throw new EvalException(getLocation(), "Method invocation failed: " + e);
-      }
-    } catch (ExecutionException e) {
-      throw new EvalException(getLocation(), "Method invocation failed: " + e);
+    }
+    if (matchingMethod != null && !matchingMethod.getAnnotation().structField()) {
+      return callMethod(matchingMethod, methodName, obj, args.toArray(), getLocation());
+    } else {
+      throw new EvalException(getLocation(), "No matching method found for "
+          + formatMethod(methodName, args) + " in "
+          + EvalUtils.getDataTypeNameFromClass(objClass));
     }
   }