Rollback of commit 5972bee6ebfa53cf165befab9fa7962e19d5f620.

*** Reason for rollback ***

Break Bazel bootstrap on JDK7

*** Original change description ***

Skylark: Give more detailed errors when parsing the arguments
of a SkylarkCallable.

--
MOS_MIGRATED_REVID=134309621

2 files changed, 26 insertions, 80 deletions

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 b347e5c9eb..4397ae74f9 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
@@ -197,32 +197,6 @@ public final class FuncallExpression extends Expression {
     return null;
   }
 
-  private static class ArgumentListConversionResult {
-    private final ImmutableList<Object> arguments;
-    private final String error;
-
-    private ArgumentListConversionResult(ImmutableList<Object> arguments, String error) {
-      this.arguments = arguments;
-      this.error = error;
-    }
-
-    public static ArgumentListConversionResult fromArgumentList(ImmutableList<Object> arguments) {
-      return new ArgumentListConversionResult(arguments, null);
-    }
-
-    public static ArgumentListConversionResult fromError(String error) {
-      return new ArgumentListConversionResult(null, error);
-    }
-
-    public String getError() {
-      return error;
-    }
-
-    public ImmutableList<Object> getArguments() {
-      return arguments;
-    }
-  }
-
   /**
    * An exception class to handle exceptions in direct Java API calls.
    */
@@ -399,20 +373,18 @@ public final class FuncallExpression extends Expression {
   // TODO(bazel-team): If there's exactly one usable method, this works. If there are multiple
   // matching methods, it still can be a problem. Figure out how the Java compiler does it
   // exactly and copy that behaviour.
-  // Throws an EvalException when it cannot find a matching function.
   private Pair<MethodDescriptor, List<Object>> findJavaMethod(
       Class<?> objClass, String methodName, List<Object> args, Map<String, Object> kwargs)
       throws EvalException {
     Pair<MethodDescriptor, List<Object>> matchingMethod = null;
     List<MethodDescriptor> methods = getMethods(objClass, methodName, getLocation());
-    ArgumentListConversionResult argumentListConversionResult = null;
     if (methods != null) {
       for (MethodDescriptor method : methods) {
         if (!method.getAnnotation().structField()) {
-          argumentListConversionResult = convertArgumentList(args, kwargs, method);
-          if (argumentListConversionResult.getArguments() != null) {
+          List<Object> resultArgs = convertArgumentList(args, kwargs, method);
+          if (resultArgs != null) {
             if (matchingMethod == null) {
-              matchingMethod = new Pair<>(method, argumentListConversionResult.getArguments());
+              matchingMethod = new Pair<>(method, resultArgs);
             } else {
               throw new EvalException(
                   getLocation(),
@@ -425,22 +397,11 @@ public final class FuncallExpression extends Expression {
       }
     }
     if (matchingMethod == null) {
-      String errorMessage;
-      if (argumentListConversionResult == null || argumentListConversionResult.getError() == null) {
-        errorMessage =
-            String.format(
-                "Type %s has no %s",
-                EvalUtils.getDataTypeNameFromClass(objClass), formatMethod(args, kwargs));
-
-      } else {
-        errorMessage =
-            String.format(
-                "%s (in %s of %s).",
-                argumentListConversionResult.getError(),
-                formatMethod(args, kwargs),
-                EvalUtils.getDataTypeNameFromClass(objClass));
-      }
-      throw new EvalException(getLocation(), errorMessage);
+      throw new EvalException(
+          getLocation(),
+          String.format(
+              "Type %s has no %s",
+              EvalUtils.getDataTypeNameFromClass(objClass), formatMethod(args, kwargs)));
     }
     return matchingMethod;
   }
@@ -455,10 +416,10 @@ public final class FuncallExpression extends Expression {
 
   /**
    * Constructs the parameters list to actually pass to the method, filling with default values if
-   * any. If the type does not match, returns null and fills in methodTypeMatchError with the
-   * appropriate message.
+   * any. If the type does not match, returns null.
    */
-  private ArgumentListConversionResult convertArgumentList(
+  @Nullable
+  private ImmutableList<Object> convertArgumentList(
       List<Object> args, Map<String, Object> kwargs, MethodDescriptor method) {
     ImmutableList.Builder<Object> builder = ImmutableList.builder();
     Class<?>[] params = method.getMethod().getParameterTypes();
@@ -473,17 +434,15 @@ public final class FuncallExpression extends Expression {
     }
     if (mandatoryPositionals > args.size()
         || args.size() > mandatoryPositionals + callable.parameters().length) {
-      return ArgumentListConversionResult.fromError("Too many arguments");
+      return null;
     }
-    // First process the legacy positional parameters.
+    // First process the legacy positional parameters
     int i = 0;
     if (mandatoryPositionals > 0) {
       for (Class<?> param : params) {
         Object value = args.get(i);
         if (!param.isAssignableFrom(value.getClass())) {
-          return ArgumentListConversionResult.fromError(
-              String.format(
-                  "Cannot convert parameter at position %d to type %s", i, param.toString()));
+          return null;
         }
         builder.add(value);
         i++;
@@ -493,7 +452,6 @@ public final class FuncallExpression extends Expression {
         }
       }
     }
-
     // Then the parameters specified in callable.parameters()
     Set<String> keys = new HashSet<>(kwargs.keySet());
     for (Param param : callable.parameters()) {
@@ -504,41 +462,32 @@ public final class FuncallExpression extends Expression {
       Object value = null;
       if (i < args.size()) {
         value = args.get(i);
-        if (!param.positional()) {
-          return ArgumentListConversionResult.fromError(
-              String.format("Parameter '%s' is not positional", param.name()));
-        } else if (!type.contains(value)) {
-          return ArgumentListConversionResult.fromError(
-              String.format(
-                  "Cannot convert parameter '%s' to type %s", param.name(), type.toString()));
+        if (!param.positional() || !type.contains(value)) {
+          return null; // next positional argument is not of the good type
         }
         i++;
       } else if (param.named() && keys.remove(param.name())) {
         // Named parameters
         value = kwargs.get(param.name());
         if (!type.contains(value)) {
-          return ArgumentListConversionResult.fromError(
-              String.format(
-                  "Cannot convert parameter '%s' to type %s", param.name(), type.toString()));
+          return null; // type does not match
         }
       } else {
         // Use default value
         if (param.defaultValue().isEmpty()) {
-          return ArgumentListConversionResult.fromError(
-              String.format("Parameter '%s' has no default value", param.name()));
+          return null; // no default value
         }
         value = SkylarkSignatureProcessor.getDefaultValue(param, null);
       }
       builder.add(value);
       if (!param.noneable() && value instanceof NoneType) {
-        return ArgumentListConversionResult.fromError(
-            String.format("Parameter '%s' cannot be None", param.name()));
+        return null; // cannot be None
       }
     }
     if (i < args.size() || !keys.isEmpty()) {
-      return ArgumentListConversionResult.fromError("Too many arguments");
+      return null; // too many arguments
     }
-    return ArgumentListConversionResult.fromArgumentList(builder.build());
+    return builder.build();
   }
 
   private String formatMethod(List<Object> args, Map<String, Object> kwargs) {
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
index b9f58081bb..b7650e0a4d 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/SkylarkEvaluationTest.java
@@ -675,14 +675,12 @@ public class SkylarkEvaluationTest extends EvaluationTest {
         .update("mock", new Mock())
         .setUp("")
         .testIfExactError(
-            "Parameter 'named' has no default value (in function with_params(int, bool) of Mock).",
-            "mock.with_params(1, True)");
+            "Type Mock has no function with_params(int, bool)", "mock.with_params(1, True)");
     new SkylarkTest()
         .update("mock", new Mock())
         .setUp("")
         .testIfExactError(
-            "Parameter 'named' has no default value (in function with_params(int, bool, bool) "
-                + "of Mock).",
+            "Type Mock has no function with_params(int, bool, bool)",
             "mock.with_params(1, True, True)");
     new SkylarkTest()
         .update("mock", new Mock())
@@ -700,15 +698,14 @@ public class SkylarkEvaluationTest extends EvaluationTest {
         .update("mock", new Mock())
         .setUp("")
         .testIfExactError(
-            "Too many arguments (in function with_params(int, bool, bool named, "
-                + "bool posOrNamed, int n) of Mock).",
+            "Type Mock has no function with_params(int, bool, bool named, bool posOrNamed, int n)",
             "mock.with_params(1, True, named=True, posOrNamed=True, n=2)");
     new SkylarkTest()
         .update("mock", new Mock())
         .setUp("")
         .testIfExactError(
-            "Parameter 'nonNoneable' cannot be None (in function with_params(int, bool, bool, "
-                + "bool named, bool optionalNamed, NoneType nonNoneable) of Mock).",
+            "Type Mock has no function with_params(int, bool, bool, bool named, bool optionalNamed,"
+                + " NoneType nonNoneable)",
             "mock.with_params(1, True, True, named=True, optionalNamed=False, nonNoneable=None)");
   }