diff options
4 files changed, 260 insertions, 39 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 98f498ae9b..56da7abe9b 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 @@ -273,15 +273,7 @@ public final class FuncallExpression extends Expression { sb.append(obj).append("."); } sb.append(func); - String backup = sb.toString(); - try { - Printer.printList(sb, args, "(", ", ", ")", /* singletonTerminator */ null); - } catch (OutOfMemoryError ex) { - // export_files might lead to an OOM error (e.g. in - // PackageSerializationTest#testMassivePackageDeserializesFine). - // TODO(b/23967033): make the Printer limit its own output. - return backup + "(<too long>)"; - } + Printer.printList(sb, args, "(", ", ", ")", /* singletonTerminator */ null); return sb.toString(); } diff --git a/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java b/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java index 22dcec8810..55e855611c 100644 --- a/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java +++ b/src/main/java/com/google/devtools/build/lib/syntax/ListLiteral.java @@ -70,33 +70,10 @@ public final class ListLiteral extends Expression { return kind == Kind.TUPLE; } - private static char startChar(Kind kind) { - switch(kind) { - case LIST: return '['; - case TUPLE: return '('; - } - return '['; - } - - private static char endChar(Kind kind) { - switch(kind) { - case LIST: return ']'; - case TUPLE: return ')'; - } - return ']'; - } - @Override public String toString() { StringBuilder sb = new StringBuilder(); - sb.append(startChar(kind)); - String sep = ""; - for (Expression e : exprs) { - sb.append(sep); - sb.append(e); - sep = ", "; - } - sb.append(endChar(kind)); + Printer.printList(sb, exprs, isTuple(), '"'); return sb.toString(); } 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 0fa29641fe..f607b11253 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,7 +13,10 @@ // limitations under the License. package com.google.devtools.build.lib.syntax; +import com.google.common.annotations.VisibleForTesting; import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableSet; +import com.google.common.collect.Iterables; import com.google.devtools.build.lib.syntax.SkylarkList.Tuple; import com.google.devtools.build.lib.vfs.PathFragment; @@ -27,6 +30,8 @@ import java.util.MissingFormatWidthException; import java.util.Set; import java.util.SortedMap; import java.util.TreeMap; +import java.util.regex.Matcher; +import java.util.regex.Pattern; /** * (Pretty) Printing of Skylark values @@ -35,6 +40,19 @@ public final class Printer { private static final char SKYLARK_QUOTATION_MARK = '"'; + /* + * Maximum number of list elements that should be printed via printList(). + */ + @VisibleForTesting + static final int CRITICAL_LIST_ELEMENTS_COUNT = 10; + + /* + * printList() will start to shorten the values of list elements when their string length reaches + * this value. + */ + @VisibleForTesting + static final int CRITICAL_LIST_ELEMENTS_STRING_LENGTH = 128; + private Printer() { } @@ -257,21 +275,54 @@ public final class Printer { String after, String singletonTerminator, char quotationMark) { + return printList(buffer, list, before, separator, after, singletonTerminator, quotationMark, + CRITICAL_LIST_ELEMENTS_COUNT, CRITICAL_LIST_ELEMENTS_STRING_LENGTH); + } + + private static Appendable printList( + Appendable buffer, + Iterable<?> list, + String before, + String separator, + String after, + String singletonTerminator, + char quotationMark, + int maxItemsToPrint, + int criticalItemsStringLength) { boolean printSeparator = false; // don't print the separator before the first element + boolean skipArgs = false; + int items = Iterables.size(list); int len = 0; - append(buffer, before); + // We don't want to print "1 more arguments", hence we don't skip arguments if there is only one + // above the limit. + int itemsToPrint = (items - maxItemsToPrint == 1) ? items : maxItemsToPrint; + + LengthLimitedAppendable appendable = + LengthLimitedAppendable.create(buffer, criticalItemsStringLength); + append(appendable, before); + appendable.enforceLimit(); for (Object o : list) { + // We don't want to print "1 more arguments", even if we hit the string limit. + if (len == itemsToPrint || (appendable.hasHitLimit() && len < items - 1)) { + skipArgs = true; + break; + } if (printSeparator) { - append(buffer, separator); + append(appendable, separator); } - write(buffer, o, quotationMark); + write(appendable, o, quotationMark); printSeparator = true; len++; } + appendable.ignoreLimit(); + if (skipArgs) { + append(appendable, separator); + append(appendable, String.format("<%d more arguments>", items - len)); + } if (singletonTerminator != null && len == 1) { - append(buffer, singletonTerminator); + append(appendable, singletonTerminator); } - return append(buffer, after); + return append(appendable, after); } public static Appendable printList(Appendable buffer, Iterable<?> list, String before, @@ -443,4 +494,134 @@ public final class Printer { } return buffer; } + + /** + * Helper class for {@code Appendable}s that want to limit the length of their input. + * + * <p>Instances of this class act as a proxy for one {@code Appendable} object and decide whether + * the given input (or parts of it) can be written to the underlying {@code Appendable}, depending + * on whether the specified maximum length has been met or not. + */ + private static final class LengthLimitedAppendable implements Appendable { + + private static final ImmutableSet<Character> SPECIAL_CHARS = + ImmutableSet.of(',', ' ', '"', '\'', ':', '(', ')', '[', ']', '{', '}'); + + private static final Pattern ARGS_PATTERN = Pattern.compile("<\\d+ more arguments>"); + + private final Appendable original; + private int limit; + private boolean ignoreLimit; + private boolean previouslyShortened; + + private LengthLimitedAppendable(Appendable original, int limit) { + this.original = original; + this.limit = limit; + } + + public static LengthLimitedAppendable create(Appendable original, int limit) { + // We don't want to overwrite the limit if original is already an instance of this class. + return (original instanceof LengthLimitedAppendable) + ? (LengthLimitedAppendable) original : new LengthLimitedAppendable(original, limit); + } + + @Override + public Appendable append(CharSequence csq) throws IOException { + if (ignoreLimit || hasOnlySpecialChars(csq)) { + // Don't update limit. + original.append(csq); + previouslyShortened = false; + } else { + int length = csq.length(); + if (length <= limit) { + limit -= length; + original.append(csq); + } else { + original.append(csq, 0, limit); + // We don't want to append multiple ellipses. + if (!previouslyShortened) { + original.append("..."); + } + appendTrailingSpecialChars(csq, limit); + previouslyShortened = true; + limit = 0; + } + } + return this; + } + + /** + * Appends any trailing "special characters" (e.g. brackets, quotation marks) in the given + * sequence to the output buffer, regardless of the limit. + * + * <p>For example, let's look at foo(['too long']). Without this method, the shortened result + * would be foo(['too...) instead of the prettier foo(['too...']). + * + * <p>If the input string was already shortened and contains "<x more arguments>", this part + * will also be appended. + */ + private void appendTrailingSpecialChars(CharSequence csq, int limit) throws IOException { + int length = csq.length(); + Matcher matcher = ARGS_PATTERN.matcher(csq); + // We assume that everything following the "x more arguments" part has to be copied, too. + int start = matcher.find() ? matcher.start() : length; + // Find the left-most non-arg char that has to be copied. + for (int i = start - 1; i > limit; --i) { + if (isSpecialChar(csq.charAt(i))) { + start = i; + } else { + break; + } + } + if (start < length) { + original.append(csq, start, csq.length()); + } + } + + /** + * Returns whether the given sequence denotes characters that are not part of the value of an + * argument. + * + * <p>Examples are brackets, braces and quotation marks. + */ + private boolean hasOnlySpecialChars(CharSequence csq) { + for (int i = 0; i < csq.length(); ++i) { + if (!isSpecialChar(csq.charAt(i))) { + return false; + } + } + return true; + } + + private boolean isSpecialChar(char c) { + return SPECIAL_CHARS.contains(c); + } + + @Override + public Appendable append(CharSequence csq, int start, int end) throws IOException { + return append(csq.subSequence(start, end)); + } + + @Override + public Appendable append(char c) throws IOException { + return append(String.valueOf(c)); + } + + public boolean hasHitLimit() { + return limit <= 0; + } + + public void enforceLimit() { + ignoreLimit = false; + } + + public void ignoreLimit() { + ignoreLimit = true; + } + + @Override + public String toString() { + return original.toString(); + } + } } 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 2dfc28fc5f..92e3d5ec0f 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 @@ -1,4 +1,4 @@ -// Copyright 2006-2015 Google Inc. All Rights Reserved. +// Copyright 2015 The Bazel Authors. All Rights Reserved. // // Licensed under the Apache License, Version 2.0 (the "License"); // you may not use this file except in compliance with the License. @@ -18,6 +18,7 @@ import static com.google.common.truth.Truth.assertThat; import static org.junit.Assert.assertEquals; import static org.junit.Assert.fail; +import com.google.common.base.Strings; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.cmdline.Label; @@ -28,6 +29,7 @@ import org.junit.runners.JUnit4; import java.util.Arrays; import java.util.HashMap; import java.util.IllegalFormatException; +import java.util.LinkedList; import java.util.List; import java.util.Map; @@ -166,4 +168,73 @@ public class PrinterTest { assertThat(Printer.repr(dict, '\'')) .isEqualTo("{1: ('foo', 'bar'), 2: ['foo', 'bar'], 'foo': []}"); } + + @Test + public void testListLimitStringLength() throws Exception { + String limit = Strings.repeat("x", Printer.CRITICAL_LIST_ELEMENTS_STRING_LENGTH); + String half = Strings.repeat("x", Printer.CRITICAL_LIST_ELEMENTS_STRING_LENGTH / 2); + + List<String> list = Arrays.asList(limit + limit); + + // String is way too long -> shorten. + assertThat(Printer.str(list)).isEqualTo("[\"" + limit + "...\"]"); + + LinkedList<List<String>> nestedList = new LinkedList<>(); + nestedList.add(list); + + // Same as above, but with one additional level of indirection. + assertThat(Printer.str(nestedList)).isEqualTo("[[\"" + limit + "...\"]]"); + + // The inner list alone would meet the limit, but because of the first element, it has to be + // shortened. + assertThat(Printer.str(Arrays.asList(half, Arrays.asList(limit)))) + .isEqualTo("[\"" + half + "\", [\"" + half + "...\"]]"); + + // String is too long, but the ellipsis make it even longer. + assertThat(Printer.str(Arrays.asList(limit + "x"))).isEqualTo("[\"" + limit + "...\"]"); + + // We hit the limit exactly -> everything is printed. + assertThat(Printer.str(Arrays.asList(limit))).isEqualTo("[\"" + limit + "\"]"); + + // Exact hit, but with two arguments -> everything is printed. + assertThat(Printer.str(Arrays.asList(half, half))) + .isEqualTo("[\"" + half + "\", \"" + half + "\"]"); + + // First argument hits the limit -> remaining argument is shortened. + assertThat(Printer.str(Arrays.asList(limit, limit))).isEqualTo("[\"" + limit + "\", \"...\"]"); + + String limitMinusOne = limit.substring(0, limit.length() - 1); + + // First arguments is one below the limit -> print first character of remaining argument. + assertThat(Printer.str(Arrays.asList(limitMinusOne, limit))) + .isEqualTo("[\"" + limitMinusOne + "\", \"x...\"]"); + + // First argument hits the limit -> we skip the remaining two arguments. + assertThat(Printer.str(Arrays.asList(limit, limit, limit))) + .isEqualTo("[\"" + limit + "\", <2 more arguments>]"); + } + + @Test + public void testListLimitTooManyArgs() throws Exception { + StringBuilder builder = new StringBuilder(); + List<Integer> maxLength = new LinkedList<>(); + + int next; + for (next = 0; next < Printer.CRITICAL_LIST_ELEMENTS_COUNT; ++next) { + maxLength.add(next); + if (next > 0) { + builder.append(", "); + } + builder.append(next); + } + + // There is one too many, but we print every argument nonetheless. + maxLength.add(next); + assertThat(Printer.str(maxLength)).isEqualTo("[" + builder + ", " + next + "]"); + + // There are two too many, hence we don't print them. + ++next; + maxLength.add(next); + assertThat(Printer.str(maxLength)).isEqualTo("[" + builder + ", <2 more arguments>]"); + } } |