Skylark: support %r format specifier

Refactor the implementation of format.
Add %r. Improve some error messages.

--
MOS_MIGRATED_REVID=96154542

6 files changed, 128 insertions, 59 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
index b7937ad98c..65ea8df75d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/BinaryOperatorExpression.java
@@ -190,18 +190,18 @@ public final class BinaryOperatorExpression extends Expression {
             if (rval instanceof List<?>) {
               List<?> rlist = (List<?>) rval;
               if (EvalUtils.isTuple(rlist)) {
-                return Printer.format(pattern, rlist);
+                return Printer.formatString(pattern, rlist);
               }
               /* string % list: fall thru */
             }
             if (rval instanceof SkylarkList) {
               SkylarkList rlist = (SkylarkList) rval;
               if (rlist.isTuple()) {
-                return Printer.format(pattern, rlist.toList());
+                return Printer.formatString(pattern, rlist.toList());
               }
             }
 
-            return Printer.format(pattern, Collections.singletonList(rval));
+            return Printer.formatString(pattern, Collections.singletonList(rval));
           } catch (IllegalFormatException e) {
             throw new EvalException(getLocation(), e.getMessage());
           }
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 bcdd4e8ce3..e5b06a971d 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
@@ -59,8 +59,8 @@ public final class DotExpression extends Expression {
           throw new EvalException(getLocation(), customErrorMessage);
         }
       }
-      throw new EvalException(getLocation(), String.format("Object of type '%s' has no field %s",
-              EvalUtils.getDataTypeName(objValue), Printer.repr(name)));
+      throw new EvalException(getLocation(), Printer.format("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/FunctionSignature.java b/src/main/java/com/google/devtools/build/lib/syntax/FunctionSignature.java
index 272fc5e7d3..e1eda63b6a 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
@@ -19,7 +19,6 @@ import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Interner;
 import com.google.common.collect.Interners;
 import com.google.common.collect.Lists;
-
 import com.google.devtools.build.lib.util.StringCanonicalizer;
 
 import java.io.Serializable;
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
index 42b5b097df..9de834f6ab 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/LValue.java
@@ -95,9 +95,9 @@ public class LValue implements Serializable {
       if (variableType != null && !variableType.equals(resultType)
           && !resultType.equals(Environment.NoneType.class)
           && !variableType.equals(Environment.NoneType.class)) {
-        throw new EvalException(loc, String.format("Incompatible variable types, "
-            + "trying to assign %s (type of %s) to variable %s which is already %s",
-            Printer.repr(result),
+        throw new EvalException(loc, Printer.format("Incompatible variable types, "
+            + "trying to assign %r (type of %s) to variable %s which is already %s",
+            result,
             EvalUtils.getDataTypeName(result),
             ident.getName(),
             EvalUtils.getDataTypeNameFromClass(variableType)));
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java b/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java
index b3d68178cc..2e905b774d 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/ListComprehension.java
@@ -120,7 +120,7 @@ public final class ListComprehension extends Expression {
 
     @Override
     public String toString() {
-      return String.format("for %s in %s", variables.toString(), Printer.repr(list));
+      return Printer.format("for %s in %r", variables.toString(), list);
     }
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/syntax/Printer.java b/src/main/java/com/google/devtools/build/lib/syntax/Printer.java
index 8739be8c65..56f129e6b8 100644
--- a/src/main/java/com/google/devtools/build/lib/syntax/Printer.java
+++ b/src/main/java/com/google/devtools/build/lib/syntax/Printer.java
@@ -13,13 +13,12 @@
 // limitations under the License.
 package com.google.devtools.build.lib.syntax;
 
+import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Lists;
-
 import com.google.devtools.build.lib.collect.nestedset.Order;
 import com.google.devtools.build.lib.vfs.PathFragment;
 
 import java.io.IOException;
-import java.util.ArrayList;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Formattable;
@@ -183,6 +182,14 @@ public final class Printer {
     }
   }
 
+  private static Appendable append(Appendable buffer, CharSequence s, int start, int end) {
+    try {
+      return buffer.append(s, start, end);
+    } catch (IOException e) {
+      throw new AssertionError(e);
+    }
+  }
+
   private static Appendable backslashChar(Appendable buffer, char c) {
     return append(append(buffer, '\\'), c);
   }
@@ -320,8 +327,10 @@ public final class Printer {
   /**
    * Convert BUILD language objects to Formattable so JDK can render them correctly.
    * Don't do this for numeric or string types because we want %d, %x, %s to work.
+   * This function is intended for use in assertions such as Precondition.checkArgument
+   * so that you only pay the cost of computing the string when the assertion passes.
    */
-  private static Object strFormattable(final Object o) {
+  public static Object strFormattable(final Object o) {
     if (o instanceof Integer || o instanceof Double || o instanceof String) {
       return o;
     } else {
@@ -339,7 +348,29 @@ public final class Printer {
     }
   }
 
-  private static final Object[] EMPTY = new Object[0];
+  /**
+   * Convert BUILD language objects to Formattable so JDK can render them correctly.
+   * Don't do this for numeric or string types because we want %d, %x, %s to work.
+   * This function is intended for use in assertions such as Precondition.checkArgument
+   * so that you only pay the cost of computing the string when the assertion passes.
+   */
+  public static Object reprFormattable(final Object o) {
+    if (o instanceof Integer || o instanceof Double) {
+      return o;
+    } else {
+      return new Formattable() {
+        @Override
+        public String toString() {
+          return repr(o);
+        }
+
+        @Override
+        public void formatTo(Formatter formatter, int flags, int width, int precision) {
+          write(formatter.out(), o);
+        }
+      };
+    }
+  }
 
   /*
    * N.B. MissingFormatWidthException is the only kind of IllegalFormatException
@@ -347,63 +378,102 @@ public final class Printer {
    */
 
   /**
-   * Perform Python-style string formatting. Implemented by delegation to Java's
-   * own string formatting routine to avoid reinventing the wheel. In more
-   * obscure cases, semantics follow JDK (not Python) rules.
+   * Perform Python-style string formatting.
    *
    * @param pattern a format string.
-   * @param tuple a tuple containing positional arguments
+   * @param arguments a tuple containing positional arguments.
+   * @return the formatted string.
    */
-  public static String format(String pattern, List<?> tuple) throws IllegalFormatException {
-    int count = countPlaceholders(pattern);
-    if (count != tuple.size()) {
-      throw new MissingFormatWidthException(
-          "not all arguments converted during string formatting");
-    }
-
-    List<Object> args = new ArrayList<>();
-
-    for (Object o : tuple) {
-      args.add(strFormattable(o));
-    }
+  public static String formatString(String pattern, List<?> arguments)
+      throws IllegalFormatException {
+    return format(new StringBuilder(), pattern, arguments).toString();
+  }
 
-    try {
-      return String.format(pattern, args.toArray(EMPTY));
-    } catch (IllegalFormatException e) {
-      throw new MissingFormatWidthException(
-          "invalid arguments for format string");
-    }
+  /**
+   * Perform Python-style string formatting.
+   *
+   * @param pattern a format string.
+   * @param arguments positional arguments.
+   * @return the formatted string.
+   */
+  public static String format(String pattern, Object... arguments)
+      throws IllegalFormatException {
+    return formatString(pattern, ImmutableList.copyOf(arguments));
   }
 
-  private static int countPlaceholders(String pattern) {
+  /**
+   * Perform Python-style string formatting, as per pattern % tuple
+   * Limitations: only %d %s %r %% are supported.
+   *
+   * @param buffer an Appendable to output to.
+   * @param pattern a format string.
+   * @param arguments a list containing positional arguments.
+   * @return the buffer, in fluent style.
+   */
+  // TODO(bazel-team): support formatting arguments, and more complex Python patterns.
+  public static Appendable format(Appendable buffer, String pattern, List<?> arguments)
+      throws IllegalFormatException {
+    // N.B. MissingFormatWidthException is the only kind of IllegalFormatException
+    // whose constructor can take and display arbitrary error message, hence its use below.
+
     int length = pattern.length();
-    boolean afterPercent = false;
-    int i = 0;
-    int count = 0;
+    int argLength = arguments.size();
+    int i = 0; // index of next character in pattern
+    int a = 0; // index of next argument in arguments
+
     while (i < length) {
-      switch (pattern.charAt(i)) {
-        case 's':
+      int p = pattern.indexOf('%', i);
+      if (p == -1) {
+        append(buffer, pattern, i, length);
+        break;
+      }
+      if (p > i) {
+        append(buffer, pattern, i, p);
+      }
+      if (p == length - 1) {
+        throw new MissingFormatWidthException(
+            "incomplete format pattern ends with %: " + repr(pattern));
+      }
+      char directive = pattern.charAt(p + 1);
+      i = p + 2;
+      switch (directive) {
+        case '%':
+          append(buffer, '%');
+          continue;
         case 'd':
-          if (afterPercent) {
-            count++;
-            afterPercent = false;
+        case 'r':
+        case 's':
+          if (a >= argLength) {
+            throw new MissingFormatWidthException("not enough arguments for format pattern "
+                + repr(pattern) + ": " + repr(SkylarkList.tuple(arguments)));
           }
-          break;
-
-        case '%':
-          afterPercent = !afterPercent;
-          break;
-
-        default:
-          if (afterPercent) {
-            throw new MissingFormatWidthException("invalid arguments for format string");
+          Object argument = arguments.get(a++);
+          switch (directive) {
+            case 'd':
+              if (argument instanceof Integer) {
+                append(buffer, argument.toString());
+                continue;
+              } else {
+                throw new MissingFormatWidthException(
+                    "invalid argument " + repr(argument) + " for format pattern %d");
+              }
+            case 'r':
+              write(buffer, argument);
+              continue;
+            case 's':
+              print(buffer, argument);
+              continue;
           }
-          afterPercent = false;
-          break;
+        default:
+          throw new MissingFormatWidthException(
+              "unsupported format character " + repr(String.valueOf(directive))
+              + " at index " + (p + 1) + " in " + repr(pattern));
       }
-      i++;
     }
-
-    return count;
+    if (a < argLength) {
+      throw new MissingFormatWidthException(
+          "not all arguments converted during string formatting");
+    }
+    return buffer;
   }
 }