Improved error messages for builtin Skylark functions that are invoked with invalid arguments.

--
MOS_MIGRATED_REVID=100386706

5 files changed, 138 insertions, 28 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
index 253e7a8603..5145f4b6b9 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BaseFunction.java
@@ -445,6 +445,7 @@ public abstract class BaseFunction {
   /**
    * Render this object in the form of an equivalent Python function signature.
    */
+  @Override
   public String toString() {
     StringBuilder sb = new StringBuilder();
     sb.append(getName());
@@ -475,4 +476,58 @@ public abstract class BaseFunction {
     Preconditions.checkState(signature != null);
     enforcedArgumentTypes = signature.getTypes();
   }
+
+  protected boolean hasSelfArgument() {
+    Class<?> clazz = getObjectType();
+    if (clazz == null) {
+      return false;
+    }
+    List<SkylarkType> types = signature.getTypes();
+    ImmutableList<String> names = signature.getSignature().getNames();
+
+    return (!types.isEmpty() && types.get(0).canBeCastTo(clazz))
+        || (!names.isEmpty() && names.get(0).equals("self"));
+  }
+
+  protected String getObjectTypeString() {
+    Class<?> clazz = getObjectType();
+    if (clazz == null) {
+      return "";
+    }
+    return EvalUtils.getDataTypeNameFromClass(clazz, false) + ".";
+  }
+
+  /**
+   * Returns the signature as "[className.]methodName(paramType1, paramType2, ...)"
+   */
+  public String getShortSignature() {
+    StringBuilder builder = new StringBuilder();
+    boolean hasSelf = hasSelfArgument();
+
+    builder.append(getObjectTypeString()).append(getName()).append("(");
+    signature.toStringBuilder(builder, true, false, false, hasSelf);
+    builder.append(")");
+
+    return builder.toString();
+  }
+
+  /**
+   * Prints the types of the first {@code howManyArgsToPrint} given arguments as
+   * "(type1, type2, ...)"
+   */
+  protected String printTypeString(Object[] args, int howManyArgsToPrint) {
+    StringBuilder builder = new StringBuilder();
+    builder.append("(");
+
+    int start = hasSelfArgument() ? 1 : 0;
+    for (int pos = start; pos < howManyArgsToPrint; ++pos) {
+      builder.append(EvalUtils.getDataTypeName(args[pos]));
+
+      if (pos < howManyArgsToPrint - 1) {
+        builder.append(", ");
+      }
+    }  
+    builder.append(")");
+    return builder.toString();
+  }
 }
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 3c041b73c9..7fc85baa3f 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
@@ -164,21 +164,25 @@ public class BuiltinFunction extends BaseFunction {
         throw badCallException(loc, e, args);
       }
     } catch (IllegalArgumentException e) {
-        // Either this was thrown by Java itself, or it's a bug
-        // To cover the first case, let's manually check the arguments.
-        final int len = args.length - ((extraArgs == null) ? 0 : extraArgs.length);
-        final Class<?>[] types = invokeMethod.getParameterTypes();
-        for (int i = 0; i < args.length; i++) {
-          if (args[i] != null && !types[i].isAssignableFrom(args[i].getClass())) {
-            String paramName = i < len
-                ? signature.getSignature().getNames().get(i) : extraArgs[i - len].name();
-            throw new EvalException(loc, String.format(
-                "expected %s for '%s' while calling %s but got %s instead",
-                EvalUtils.getDataTypeNameFromClass(types[i]), paramName, getName(),
-                EvalUtils.getDataTypeName(args[i])));
-          }
+      // Either this was thrown by Java itself, or it's a bug
+      // To cover the first case, let's manually check the arguments.
+      final int len = args.length - ((extraArgs == null) ? 0 : extraArgs.length);
+      final Class<?>[] types = invokeMethod.getParameterTypes();
+      for (int i = 0; i < args.length; i++) {
+        if (args[i] != null && !types[i].isAssignableFrom(args[i].getClass())) {
+          String paramName =
+              i < len ? signature.getSignature().getNames().get(i) : extraArgs[i - len].name();
+          int extraArgsCount = (extraArgs == null) ? 0 : extraArgs.length;
+          throw new EvalException(
+              loc,
+              String.format(
+                  "Method %s is not applicable for arguments %s: '%s' is %s, but should be %s",
+                  getShortSignature(), printTypeString(args, args.length - extraArgsCount),
+                  paramName, EvalUtils.getDataTypeName(args[i]),
+                  EvalUtils.getDataTypeNameFromClass(types[i])));
         }
-        throw badCallException(loc, e, args);
+      }
+      throw badCallException(loc, e, args);
     } catch (IllegalAccessException e) {
       throw badCallException(loc, e, args);
     }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
index 52b7e1f5e3..e16ffa28b9 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/EvalUtils.java
@@ -257,6 +257,15 @@ public abstract class EvalUtils {
    * Returns a pretty name for the datatype equivalent of class 'c' in the Build language.
    */
   public static String getDataTypeNameFromClass(Class<?> c) {
+    return getDataTypeNameFromClass(c, true);
+  }
+
+  /**
+   * Returns a pretty name for the datatype equivalent of class 'c' in the Build language.
+   * @param highlightNameSpaces Determines whether the result should also contain a special comment
+   * when the given class identifies a Skylark name space.
+   */
+  public static String getDataTypeNameFromClass(Class<?> c, boolean highlightNameSpaces) {
     if (c.equals(Object.class)) {
       return "unknown";
     } else if (c.equals(String.class)) {
@@ -292,7 +301,7 @@ public abstract class EvalUtils {
     } else if (c.isAnnotationPresent(SkylarkModule.class)) {
       SkylarkModule module = c.getAnnotation(SkylarkModule.class);
       return c.getAnnotation(SkylarkModule.class).name()
-          + (module.namespace() ? " (a language module)" : "");
+          + ((module.namespace() && highlightNameSpaces) ? " (a language module)" : "");
     } else {
       if (c.getSimpleName().isEmpty()) {
         return c.getName();
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java b/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
index e1eda63b6a..b38556d34e 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
@@ -343,10 +343,24 @@ public abstract class FunctionSignature implements Serializable {
           FunctionSignature.<T>valueListOrNull(types));
     }
 
+    public StringBuilder toStringBuilder(final StringBuilder sb) {
+      return toStringBuilder(sb, true, true, true, false);
+    }
+
     /**
-     * Append a representation of this signature to a string buffer.
+     * Appends a representation of this signature to a string buffer.
+     * @param sb Output StringBuffer
+     * @param showNames Determines whether the names of arguments should be printed
+     * @param showDefaults Determines whether the default values of arguments should be printed (if
+     * present)
+     * @param skipMissingTypeNames Determines whether missing type names should be omitted (true) or
+     * replaced with "object" (false). If showNames is false, "object" is always used as a type name
+     * to prevent blank spaces.
+     * @param skipFirstMandatory Determines whether the first mandatory parameter should be omitted.
      */
-    public StringBuilder toStringBuilder(final StringBuilder sb) {
+    public StringBuilder toStringBuilder(final StringBuilder sb, final boolean showNames,
+        final boolean showDefaults, final boolean skipMissingTypeNames,
+        final boolean skipFirstMandatory) {
       FunctionSignature signature = getSignature();
       Shape shape = signature.getShape();
       final ImmutableList<String> names = signature.getNames();
@@ -377,28 +391,46 @@ public abstract class FunctionSignature implements Serializable {
           isMore = true;
         }
         public void type(int i) {
-          if (types != null && types.get(i) != null) {
-            sb.append(": ").append(types.get(i).toString());
+          // We have to assign an artificial type string when the type is null.
+          // This happens when either
+          // a) there is no type defined (such as in user-defined functions) or
+          // b) the type is java.lang.Object.
+          boolean noTypeDefined = (types == null || types.get(i) == null);
+          String typeString = noTypeDefined ? "object" : types.get(i).toString();
+          if (noTypeDefined && showNames && skipMissingTypeNames) {
+            // This is the only case where we don't want to append typeString.
+            // If showNames = false, we ignore skipMissingTypeNames = true and append "object"
+            // in order to prevent blank spaces.
+          } else {
+            // We only append colons when there is a name.
+            if (showNames) {
+              sb.append(": ");
+            }
+            sb.append(typeString);
           }
         }
         public void mandatory(int i) {
           comma();
-          sb.append(names.get(i));
+          if (showNames) {
+            sb.append(names.get(i));
+          }
           type(i);
         }
         public void optional(int i) {
           mandatory(i);
-          sb.append(" = ");
-          if (defaultValues == null) {
-            sb.append("?");
-          } else {
-            Printer.write(sb, defaultValues.get(j++));
+          if (showDefaults) {
+            sb.append(" = ");
+            if (defaultValues == null) {
+              sb.append("?");
+            } else {
+              Printer.write(sb, defaultValues.get(j++));
+            }
           }
         }
       };
       Show show = new Show();
 
-      int i = 0;
+      int i = skipFirstMandatory ? 1 : 0;
       for (; i < mandatoryPositionals; i++) {
         show.mandatory(i);
       }
@@ -408,7 +440,7 @@ public abstract class FunctionSignature implements Serializable {
       if (hasStar) {
         show.comma();
         sb.append("*");
-        if (starArg) {
+        if (starArg && showNames) {
           sb.append(names.get(iStarArg));
         }
       }
@@ -421,7 +453,9 @@ public abstract class FunctionSignature implements Serializable {
       if (kwArg) {
         show.comma();
         sb.append("**");
-        sb.append(names.get(iKwArg));
+        if (showNames) {
+          sb.append(names.get(iKwArg));
+        }
       }
 
       return sb;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java b/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
index 4a5968cd36..5831a28602 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/UserDefinedFunction.java
@@ -46,6 +46,14 @@ public class UserDefinedFunction extends BaseFunction {
     return location;
   }
 
+  /**
+   * Since the types of parameters of user defined functions are unknown, we just return
+   * "name(parameterCount)"
+   */
+  @Override
+  public String getShortSignature() {
+    return String.format("%s(%d)", getName(), getArgArraySize());
+  }
 
   @Override
   public Object call(Object[] arguments, FuncallExpression ast, Environment env)