Skylark: support %r format specifier

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

--
MOS_MIGRATED_REVID=96154542

9 files changed, 164 insertions, 90 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/packages/MethodLibrary.java b/src/main/java/com/google/devtools/build/lib/packages/MethodLibrary.java
index f67b0feec9..794e0392e6 100644
--- a/src/main/java/com/google/devtools/build/lib/packages/MethodLibrary.java
+++ b/src/main/java/com/google/devtools/build/lib/packages/MethodLibrary.java
@@ -472,8 +472,7 @@ public class MethodLibrary {
         Location loc) throws EvalException, ConversionException {
       int res = stringFind(false, self, sub, start, end, "'end' argument to rindex");
       if (res < 0) {
-        throw new EvalException(loc, String.format("substring %s not found in %s",
-                Printer.repr(sub), Printer.repr(self)));
+        throw new EvalException(loc, Printer.format("substring %r not found in %r", sub, self));
       }
       return res;
     }
@@ -498,8 +497,7 @@ public class MethodLibrary {
         Location loc) throws EvalException, ConversionException {
       int res = stringFind(true, self, sub, start, end, "'end' argument to index");
       if (res < 0) {
-        throw new EvalException(loc, String.format("substring %s not found in %s",
-                Printer.repr(sub), Printer.repr(self)));
+        throw new EvalException(loc, Printer.format("substring %r not found in %r", sub, self));
       }
       return res;
     }
@@ -718,8 +716,7 @@ public class MethodLibrary {
       } else if (self instanceof Map<?, ?>) {
         Map<?, ?> dictionary = (Map<?, ?>) self;
         if (!dictionary.containsKey(key)) {
-          throw new EvalException(loc, String.format("Key %s not found in dictionary",
-                  Printer.repr(key)));
+          throw new EvalException(loc, Printer.format("Key %r not found in dictionary", key));
         }
         return dictionary.get(key);
 
@@ -922,7 +919,7 @@ public class MethodLibrary {
         }
       } else {
         throw new EvalException(loc,
-            String.format("%s is not of type string or int or bool", Printer.repr(x)));
+            Printer.format("%r is not of type string or int or bool", x));
       }
     }
   };
@@ -1121,8 +1118,8 @@ public class MethodLibrary {
         if (defaultValue != Environment.NONE) {
           return defaultValue;
         } else {
-          throw new EvalException(loc, String.format("Object of type '%s' has no field %s",
-                  EvalUtils.getDataTypeName(obj), Printer.repr(name)));
+          throw new EvalException(loc, Printer.format("Object of type '%s' has no field %r",
+                  EvalUtils.getDataTypeName(obj), name));
         }
       }
       return result;
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;
   }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
index 7177c7f1d0..b764413ce4 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/EvaluationTest.java
@@ -607,7 +607,7 @@ public class EvaluationTest extends EvaluationTestCase {
   @Test
   public void testPercOnObjectInvalidFormat() throws Exception {
     update("obj", createObjWithStr());
-    checkEvalError("invalid arguments for format string", "'%d' % obj");
+    checkEvalError("invalid argument str marker for format pattern %d", "'%d' % obj");
   }
 
   @SuppressWarnings("unchecked")
diff --git a/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java b/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java
index 8578ecd9a0..d9637414dd 100644
--- a/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java
+++ b/src/test/java/com/google/devtools/build/lib/syntax/PrinterTest.java
@@ -98,9 +98,9 @@ public class PrinterTest {
                  Printer.repr(makeFilesetEntry()));
   }
 
-  private void checkFormatPositionalFails(String format, List<?> tuple, String errorMessage) {
+  private void checkFormatPositionalFails(String errorMessage, String format, Object... arguments) {
     try {
-      Printer.format(format, tuple);
+      Printer.format(format, arguments);
       fail();
     } catch (IllegalFormatException e) {
       assertThat(e).hasMessage(errorMessage);
@@ -109,28 +109,36 @@ public class PrinterTest {
 
   @Test
   public void testFormatPositional() throws Exception {
-    assertEquals("foo 3", Printer.format("%s %d", makeTuple("foo", 3)));
+    assertEquals("foo 3", Printer.formatString("%s %d", makeTuple("foo", 3)));
+    assertEquals("foo 3", Printer.format("%s %d", "foo", 3));
 
     // Note: formatString doesn't perform scalar x -> (x) conversion;
     // The %-operator is responsible for that.
-    assertThat(Printer.format("", makeTuple())).isEmpty();
-    assertEquals("foo", Printer.format("%s", makeTuple("foo")));
-    assertEquals("3.14159", Printer.format("%s", makeTuple(3.14159)));
-    checkFormatPositionalFails("%s", makeTuple(1, 2, 3),
-        "not all arguments converted during string formatting");
-    assertEquals("%foo", Printer.format("%%%s", makeTuple("foo")));
-    checkFormatPositionalFails("%%s", makeTuple("foo"),
-        "not all arguments converted during string formatting");
-    checkFormatPositionalFails("% %s", makeTuple("foo"),
-        "invalid arguments for format string");
-    assertEquals("[1, 2, 3]", Printer.format("%s", makeTuple(makeList(1, 2, 3))));
-    assertEquals("(1, 2, 3)", Printer.format("%s", makeTuple(makeTuple(1, 2, 3))));
-    assertEquals("[]", Printer.format("%s", makeTuple(makeList())));
-    assertEquals("()", Printer.format("%s", makeTuple(makeTuple())));
-
-    checkFormatPositionalFails("%.3g", makeTuple(), "invalid arguments for format string");
-    checkFormatPositionalFails("%.3g", makeTuple(1, 2), "invalid arguments for format string");
-    checkFormatPositionalFails("%.s", makeTuple(), "invalid arguments for format string");
+    assertThat(Printer.formatString("", makeTuple())).isEmpty();
+    assertEquals("foo", Printer.format("%s", "foo"));
+    assertEquals("3.14159", Printer.format("%s", 3.14159));
+    checkFormatPositionalFails("not all arguments converted during string formatting",
+        "%s", 1, 2, 3);
+    assertEquals("%foo", Printer.format("%%%s", "foo"));
+    checkFormatPositionalFails("not all arguments converted during string formatting",
+        "%%s", "foo");
+    checkFormatPositionalFails("unsupported format character \" \" at index 1 in \"% %s\"",
+        "% %s", "foo");
+    assertEquals("[1, 2, 3]", Printer.format("%s", makeList(1, 2, 3)));
+    assertEquals("(1, 2, 3)", Printer.format("%s", makeTuple(1, 2, 3)));
+    assertEquals("[]", Printer.format("%s", makeList()));
+    assertEquals("()", Printer.format("%s", makeTuple()));
+    assertEquals("% 1 \"2\" 3", Printer.format("%% %d %r %s", 1, "2", "3"));
+
+    checkFormatPositionalFails(
+        "invalid argument \"1\" for format pattern %d",
+        "%d", "1");
+    checkFormatPositionalFails("unsupported format character \".\" at index 1 in \"%.3g\"",
+        "%.3g");
+    checkFormatPositionalFails("unsupported format character \".\" at index 1 in \"%.3g\"",
+        "%.3g", 1, 2);
+    checkFormatPositionalFails("unsupported format character \".\" at index 1 in \"%.s\"",
+        "%.s");
   }
 
   private String createExpectedFilesetEntryString(FilesetEntry.SymlinkBehavior symlinkBehavior) {