Cleanup in error messages, try to improve consistency.

--
PiperOrigin-RevId: 143204724
MOS_MIGRATED_REVID=143204724

10 files changed, 89 insertions, 74 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 b050b2ec48..7614df5dc0 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
@@ -159,7 +159,7 @@ public class BuiltinFunction extends BaseFunction {
       if (e instanceof EvalException) {
         throw ((EvalException) e).ensureLocation(loc);
       } else if (e instanceof IllegalArgumentException) {
-        throw new EvalException(loc, "Illegal argument in call to " + getName(), e);
+        throw new EvalException(loc, "illegal argument in call to " + getName(), e);
       }
       // TODO(bazel-team): replace with Throwables.throwIfInstanceOf once Guava 20 is released.
       Throwables.propagateIfInstanceOf(e, InterruptedException.class);
@@ -179,9 +179,12 @@ public class BuiltinFunction extends BaseFunction {
           throw new EvalException(
               loc,
               String.format(
-                  "Method %s is not applicable for arguments %s: '%s' is %s, but should be %s",
-                  getShortSignature(true), printTypeString(args, args.length - extraArgsCount),
-                  paramName, EvalUtils.getDataTypeName(args[i]),
+                  "method %s is not applicable for arguments %s: "
+                      + "'%s' is '%s', but should be '%s'",
+                  getShortSignature(true),
+                  printTypeString(args, args.length - extraArgsCount),
+                  paramName,
+                  EvalUtils.getDataTypeName(args[i]),
                   EvalUtils.getDataTypeNameFromClass(types[i])));
         }
       }
@@ -351,7 +354,7 @@ public class BuiltinFunction extends BaseFunction {
     @Override
     public Object call(Object[] args, @Nullable FuncallExpression ast, @Nullable Environment env)
       throws EvalException {
-      throw new EvalException(null, "Tried to invoke a Factory for function " + this);
+      throw new EvalException(null, "tried to invoke a Factory for function " + this);
     }
 
     /** Instantiate the Factory
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 a0ce8fe7bf..5d94498ea7 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
@@ -74,7 +74,7 @@ public final class DotExpression extends Expression {
       throw new EvalException(
           loc,
           Printer.format(
-              "Object of type '%s' has no field %r", EvalUtils.getDataTypeName(objValue), name));
+              "object of type '%s' has no field %r", EvalUtils.getDataTypeName(objValue), name));
     }
     return result;
   }
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 370692336b..bb859c90c6 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
@@ -270,9 +270,12 @@ public final class EvalUtils {
 
   public static Object checkNotNull(Expression expr, Object obj) throws EvalException {
     if (obj == null) {
-      throw new EvalException(expr.getLocation(),
-          "Unexpected null value, please send a bug report. "
-          + "This was generated by '" + expr + "'");
+      throw new EvalException(
+          expr.getLocation(),
+          "unexpected null value, please send a bug report. "
+              + "This was generated by expression '"
+              + expr
+              + "'");
     }
     return obj;
   }
@@ -410,8 +413,9 @@ public final class EvalUtils {
       actualIndex += length;
     }
     if (actualIndex < 0 || actualIndex >= length) {
-      throw new EvalException(loc, "Index out of range (index is "
-          + index + ", but sequence has " + length + " elements)");
+      throw new EvalException(
+          loc,
+          "index out of range (index is " + index + ", but sequence has " + length + " elements)");
     }
     return actualIndex;
   }
@@ -422,8 +426,8 @@ public final class EvalUtils {
   public static int getSequenceIndex(Object index, int length, Location loc)
       throws EvalException {
     if (!(index instanceof Integer)) {
-      throw new EvalException(loc, "Indices must be integers, not "
-          + EvalUtils.getDataTypeName(index));
+      throw new EvalException(
+          loc, "indices must be integers, not " + EvalUtils.getDataTypeName(index));
     }
     return getSequenceIndex(((Integer) index).intValue(), length, loc);
   }
@@ -491,11 +495,11 @@ public final class EvalUtils {
     } else if (stepObj instanceof Integer) {
       step = ((Integer) stepObj).intValue();
     } else {
-      throw new EvalException(loc,
-          String.format("Slice step must be an integer, not '%s'", stepObj));
+      throw new EvalException(
+          loc, String.format("slice step must be an integer, not '%s'", stepObj));
     }
     if (step == 0) {
-      throw new EvalException(loc, "Slice step cannot be zero");
+      throw new EvalException(loc, "slice step cannot be zero");
     }
 
     if (startObj == Runtime.NONE) {
@@ -503,8 +507,8 @@ public final class EvalUtils {
     } else if (startObj instanceof Integer) {
       start = ((Integer) startObj).intValue();
     } else {
-      throw new EvalException(loc,
-          String.format("Slice start must be an integer, not '%s'", startObj));
+      throw new EvalException(
+          loc, String.format("slice start must be an integer, not '%s'", startObj));
     }
     if (endObj == Runtime.NONE) {
       // If step is negative, can't use -1 for end since that would be converted
@@ -513,8 +517,7 @@ public final class EvalUtils {
     } else if (endObj instanceof Integer) {
       end = ((Integer) endObj).intValue();
     } else {
-      throw new EvalException(loc,
-          String.format("Slice end must be an integer, not '%s'", endObj));
+      throw new EvalException(loc, String.format("slice end must be an integer, not '%s'", endObj));
     }
 
     return getSliceIndices(start, end, step, length);
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 7ca5793cc9..b7c2d59ccd 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
@@ -265,10 +265,6 @@ public final class FuncallExpression extends Expression {
     return numPositionalArgs;
   }
 
-  private String functionName() {
-    return "function " + func.getName();
-  }
-
   @Override
   public String toString() {
     StringBuilder sb = new StringBuilder();
@@ -291,7 +287,7 @@ public final class FuncallExpression extends Expression {
     try {
       return methodCache.get(objClass).get(methodName);
     } catch (ExecutionException e) {
-      throw new EvalException(loc, "Method invocation failed: " + e);
+      throw new EvalException(loc, "method invocation failed: " + e);
     }
   }
 
@@ -308,7 +304,7 @@ public final class FuncallExpression extends Expression {
     try {
       Method method = methodDescriptor.getMethod();
       if (obj == null && !Modifier.isStatic(method.getModifiers())) {
-        throw new EvalException(loc, "Method '" + methodName + "' is not static");
+        throw new EvalException(loc, "method '" + methodName + "' is not static");
       }
       // This happens when the interface is public but the implementation classes
       // have reduced visibility.
@@ -321,16 +317,20 @@ public final class FuncallExpression extends Expression {
         if (methodDescriptor.getAnnotation().allowReturnNones()) {
           return Runtime.NONE;
         } else {
-          throw new EvalException(loc,
-              "Method invocation returned None, please contact Skylark developers: " + methodName
-              + Printer.listString(ImmutableList.copyOf(args), "(", ", ", ")", null));
+          throw new EvalException(
+              loc,
+              "method invocation returned None, please file a bug report: "
+                  + methodName
+                  + Printer.listString(ImmutableList.copyOf(args), "(", ", ", ")", null));
         }
       }
       // TODO(bazel-team): get rid of this, by having everyone use the Skylark data structures
       result = SkylarkType.convertToSkylark(result, method, env);
       if (result != null && !EvalUtils.isSkylarkAcceptable(result.getClass())) {
-        throw new EvalException(loc, Printer.format(
-            "Method '%s' returns an object of invalid type %r", methodName, result.getClass()));
+        throw new EvalException(
+            loc,
+            Printer.format(
+                "method '%s' returns an object of invalid type %r", methodName, result.getClass()));
       }
       return result;
     } catch (IllegalAccessException e) {
@@ -344,7 +344,7 @@ public final class FuncallExpression extends Expression {
         throw new EvalExceptionWithJavaCause(loc, e.getCause());
       } else {
         // This is unlikely to happen
-        throw new EvalException(loc, "Method invocation failed: " + e);
+        throw new EvalException(loc, "method invocation failed: " + e);
       }
     }
   }
@@ -374,7 +374,7 @@ public final class FuncallExpression extends Expression {
               throw new EvalException(
                   getLocation(),
                   String.format(
-                      "Type %s has multiple matches for %s",
+                      "type '%s' has multiple matches for function %s",
                       EvalUtils.getDataTypeNameFromClass(objClass), formatMethod(args, kwargs)));
             }
           }
@@ -386,13 +386,13 @@ public final class FuncallExpression extends Expression {
       if (argumentListConversionResult == null || argumentListConversionResult.getError() == null) {
         errorMessage =
             String.format(
-                "Type %s has no %s",
+                "type '%s' has no method %s",
                 EvalUtils.getDataTypeNameFromClass(objClass), formatMethod(args, kwargs));
 
       } else {
         errorMessage =
             String.format(
-                "%s (in %s of %s).",
+                "%s, in method %s of '%s'",
                 argumentListConversionResult.getError(),
                 formatMethod(args, kwargs),
                 EvalUtils.getDataTypeNameFromClass(objClass));
@@ -429,7 +429,7 @@ public final class FuncallExpression extends Expression {
     }
     if (mandatoryPositionals > args.size()
         || args.size() > mandatoryPositionals + callable.parameters().length) {
-      return ArgumentListConversionResult.fromError("Too many arguments");
+      return ArgumentListConversionResult.fromError("too many arguments");
     }
     // First process the legacy positional parameters.
     int i = 0;
@@ -482,25 +482,25 @@ public final class FuncallExpression extends Expression {
         // Use default value
         if (param.defaultValue().isEmpty()) {
           return ArgumentListConversionResult.fromError(
-              String.format("Parameter '%s' has no default value", param.name()));
+              String.format("parameter '%s' has no default value", param.name()));
         }
         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()));
+            String.format("parameter '%s' cannot be None", param.name()));
       }
     }
     if (i < args.size() || !keys.isEmpty()) {
-      return ArgumentListConversionResult.fromError("Too many arguments");
+      return ArgumentListConversionResult.fromError("too many arguments");
     }
     return ArgumentListConversionResult.fromArgumentList(builder.build());
   }
 
   private String formatMethod(List<Object> args, Map<String, Object> kwargs) {
     StringBuilder sb = new StringBuilder();
-    sb.append(functionName()).append("(");
+    sb.append(func.getName()).append("(");
     boolean first = true;
     for (Object obj : args) {
       if (!first) {
@@ -577,12 +577,12 @@ public final class FuncallExpression extends Expression {
     if (!(items instanceof Map<?, ?>)) {
       throw new EvalException(
           location,
-          "Argument after ** must be a dictionary, not " + EvalUtils.getDataTypeName(items));
+          "argument after ** must be a dictionary, not " + EvalUtils.getDataTypeName(items));
     }
     for (Map.Entry<?, ?> entry : ((Map<?, ?>) items).entrySet()) {
       if (!(entry.getKey() instanceof String)) {
         throw new EvalException(
-            location, "Keywords must be strings, not " + EvalUtils.getDataTypeName(entry.getKey()));
+            location, "keywords must be strings, not " + EvalUtils.getDataTypeName(entry.getKey()));
       }
       addKeywordArg(kwargs, (String) entry.getKey(), entry.getValue(), duplicates);
     }
@@ -717,7 +717,7 @@ public final class FuncallExpression extends Expression {
         try {
           return callFunction(javaMethod.first.getMethod().invoke(obj), env);
         } catch (IllegalAccessException e) {
-          throw new EvalException(getLocation(), "Method invocation failed: " + e);
+          throw new EvalException(getLocation(), "method invocation failed: " + e);
         } catch (InvocationTargetException e) {
           if (e.getCause() instanceof FuncallException) {
             throw new EvalException(getLocation(), e.getCause().getMessage());
@@ -725,7 +725,7 @@ public final class FuncallExpression extends Expression {
             throw new EvalExceptionWithJavaCause(getLocation(), e.getCause());
           } else {
             // This is unlikely to happen
-            throw new EvalException(getLocation(), "Method invocation failed: " + e);
+            throw new EvalException(getLocation(), "method invocation failed: " + e);
           }
         }
       }
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
index 575a7d2de1..fd2a59b07d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/IndexExpression.java
@@ -58,9 +58,8 @@ public final class IndexExpression extends Expression {
     throw new EvalException(
         loc,
         Printer.format(
-            "Type %s has no operator [](%s)",
-            EvalUtils.getDataTypeName(objValue),
-            EvalUtils.getDataTypeName(keyValue)));
+            "type '%s' has no operator [](%s)",
+            EvalUtils.getDataTypeName(objValue), EvalUtils.getDataTypeName(keyValue)));
   }
 
   @Override
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
index b4e2a21301..415d4273ca 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/MethodLibrary.java
@@ -1001,7 +1001,7 @@ public class MethodLibrary {
     try {
       return maxOrdering.max(getIterable(args, loc));
     } catch (NoSuchElementException ex) {
-      throw new EvalException(loc, "Expected at least one argument");
+      throw new EvalException(loc, "expected at least one argument");
     }
   }
 
@@ -1207,7 +1207,7 @@ public class MethodLibrary {
             }
             i++;
           }
-          throw new EvalException(loc, Printer.format("Item %r not found in list", x));
+          throw new EvalException(loc, Printer.format("item %r not found in list", x));
         }
       };
 
@@ -1235,7 +1235,7 @@ public class MethodLibrary {
               return Runtime.NONE;
             }
           }
-          throw new EvalException(loc, Printer.format("Item %r not found in list", x));
+          throw new EvalException(loc, Printer.format("item %r not found in list", x));
         }
       };
 
@@ -1749,15 +1749,13 @@ public class MethodLibrary {
               throw new EvalException(
                   location,
                   String.format(
-                      "Sequence #%d has length %d, but exactly two elements are required",
+                      "item #%d has length %d, but exactly two elements are required",
                       pos, numElements));
             }
             return tuple;
           } catch (ConversionException e) {
             throw new EvalException(
-                loc,
-                String.format(
-                    "Cannot convert dictionary update sequence element #%d to a sequence", pos));
+                loc, String.format("cannot convert item #%d to a sequence", pos));
           }
         }
       };
@@ -1945,7 +1943,7 @@ public class MethodLibrary {
             throw new EvalException(
                 loc,
                 Printer.format(
-                    "Object of type '%s' has no attribute %r%s",
+                    "object of type '%s' has no attribute %r%s",
                     EvalUtils.getDataTypeName(obj),
                     name,
                     isRealMethod ? ", however, a method of that name exists" : ""));
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
index d3dac8198c..72c301ff8b 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ReturnStatement.java
@@ -33,8 +33,11 @@ public class ReturnStatement extends Statement {
     Object value;
 
     public ReturnException(Location location, Object value) {
-      super(location, "Return statements must be inside a function",
-          /*dueToIncompleteAST=*/ false, /*fillInJavaStackTrace=*/ false);
+      super(
+          location,
+          "return statements must be inside a function",
+          /*dueToIncompleteAST=*/ false, /*fillInJavaStackTrace=*/
+          false);
       this.value = value;
     }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
index af75d436d1..ad0a97dfe7 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SkylarkDict.java
@@ -189,8 +189,10 @@ public final class SkylarkDict<K, V>
     if (obj instanceof SkylarkDict) {
       return ((SkylarkDict<?, ?>) obj).getContents(keyType, valueType, description);
     }
-    throw new EvalException(null,
-        Printer.format("Illegal argument: %s is not of expected type dict or NoneType",
+    throw new EvalException(
+        null,
+        Printer.format(
+            "%s is not of expected type dict or NoneType",
             description == null ? Printer.repr(obj) : String.format("'%s'", description)));
   }
 
@@ -218,7 +220,7 @@ public final class SkylarkDict<K, V>
   @Override
   public final Object getIndex(Object key, Location loc) throws EvalException {
     if (!this.containsKey(key)) {
-      throw new EvalException(loc, Printer.format("Key %r not found in dictionary", key));
+      throw new EvalException(loc, Printer.format("key %r not found in dictionary", key));
     }
     return this.get(key);
   }
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 87fa01203b..09f2467e70 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
@@ -601,9 +601,8 @@ public abstract class SkylarkType implements Serializable {
    */
   static void checkTypeAllowedInSkylark(Object object, Location loc) throws EvalException {
     if (!isTypeAllowedInSkylark(object)) {
-      throw new EvalException(loc,
-                    "Type is not allowed in Skylark: "
-          + object.getClass().getSimpleName());
+      throw new EvalException(
+          loc, "internal error: type '" + object.getClass().getSimpleName() + "' is not allowed");
     }
   }
 
@@ -660,19 +659,25 @@ public abstract class SkylarkType implements Serializable {
       return ImmutableMap.of();
     }
     if (!(obj instanceof Map<?, ?>)) {
-      throw new EvalException(null, String.format(
-          "Illegal argument: expected a dictionary for %s but got %s instead",
-          what, EvalUtils.getDataTypeName(obj)));
+      throw new EvalException(
+          null,
+          String.format(
+              "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 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())));
+        throw new EvalException(
+            null,
+            String.format(
+                "expected <%s, %s> type for '%s' but got <%s, %s> instead",
+                keyType.getSimpleName(),
+                valueType.getSimpleName(),
+                what,
+                EvalUtils.getDataTypeName(input.getKey()),
+                EvalUtils.getDataTypeName(input.getValue())));
       }
     }
 
@@ -735,8 +740,10 @@ public abstract class SkylarkType implements Serializable {
   public static void checkType(Object object, Class<?> type, @Nullable Object description)
       throws EvalException {
     if (!type.isInstance(object)) {
-      throw new EvalException(null,
-          Printer.format("Illegal argument: expected type %r %sbut got type %s instead",
+      throw new EvalException(
+          null,
+          Printer.format(
+              "expected type '%r' %sbut got type '%s' instead",
               type,
               description == null ? "" : String.format("for %s ", description),
               EvalUtils.getDataTypeName(object)));
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
index f7c36e5a7a..14988f0f7a 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/SliceExpression.java
@@ -89,7 +89,7 @@ public final class SliceExpression extends Expression {
     throw new EvalException(
         loc,
         Printer.format(
-            "Type %s has no operator [:](%s, %s, %s)",
+            "type '%s' has no operator [:](%s, %s, %s)",
             EvalUtils.getDataTypeName(objValue),
             EvalUtils.getDataTypeName(startValue),
             EvalUtils.getDataTypeName(endValue),