From b9fd8d2242869ea7c9efd10cbbe7278c4b6611b1 Mon Sep 17 00:00:00 2001 From: google-java-format Team Date: Wed, 10 Feb 2021 03:16:08 -0800 Subject: [PATCH] Fix an off-by-one issue on the reflowing of string literals. During the initial width calculation, it seems we are off by one. The existing tests where actually also wrong (wrapped at 99). PiperOrigin-RevId: 356697501 --- .../googlejavaformat/java/StringWrapper.java | 14 +++-- .../java/FormatterIntegrationTest.java | 4 +- .../googlejavaformat/java/MainTest.java | 4 +- .../java/testdata/B173808510.input | 8 +++ .../java/testdata/B173808510.output | 9 +++ .../java/testdata/LiteralReflow.input | 44 +++++++++++++++ .../java/testdata/LiteralReflow.output | 55 +++++++++++++++++++ 7 files changed, 130 insertions(+), 8 deletions(-) create mode 100644 core/src/test/resources/com/google/googlejavaformat/java/testdata/B173808510.input create mode 100644 core/src/test/resources/com/google/googlejavaformat/java/testdata/B173808510.output create mode 100644 core/src/test/resources/com/google/googlejavaformat/java/testdata/LiteralReflow.input create mode 100644 core/src/test/resources/com/google/googlejavaformat/java/testdata/LiteralReflow.output diff --git a/core/src/main/java/com/google/googlejavaformat/java/StringWrapper.java b/core/src/main/java/com/google/googlejavaformat/java/StringWrapper.java index 063843622..c0f16e9b5 100644 --- a/core/src/main/java/com/google/googlejavaformat/java/StringWrapper.java +++ b/core/src/main/java/com/google/googlejavaformat/java/StringWrapper.java @@ -239,9 +239,10 @@ static int hasEscapedNewlineAt(String input, int idx) { * @param separator the line separator * @param columnLimit the number of columns to wrap at * @param startColumn the column position of the beginning of the original text - * @param trailing extra space to leave after the last line - * @param components the text to reflow - * @param first0 true if the text includes the beginning of its enclosing concat chain, i.e. a + * @param trailing extra space to leave after the last line, to accommodate a ; or ) + * @param components the text to reflow. This is a list of “words” of a single literal. Its first + * and last quotes have been stripped + * @param first0 true if the text includes the beginning of its enclosing concat chain */ private static String reflow( String separator, @@ -251,7 +252,7 @@ private static String reflow( ImmutableList components, boolean first0) { // We have space between the start column and the limit to output the first line. - // Reserve two spaces for the quotes. + // Reserve two spaces for the start and end quotes. int width = columnLimit - startColumn - 2; Deque input = new ArrayDeque<>(components); List lines = new ArrayList<>(); @@ -259,10 +260,13 @@ private static String reflow( while (!input.isEmpty()) { int length = 0; List line = new ArrayList<>(); + // If we know this is going to be the last line, then remove a bit of width to account for the + // trailing characters. if (input.stream().mapToInt(String::length).sum() <= width) { + // This isn’t quite optimal, but arguably good enough. See b/179561701 width -= trailing; } - while (!input.isEmpty() && (length <= 4 || (length + input.peekFirst().length()) < width)) { + while (!input.isEmpty() && (length <= 4 || (length + input.peekFirst().length()) <= width)) { String text = input.removeFirst(); line.add(text); length += text.length(); diff --git a/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java b/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java index 44ba63925..fdcf6f8a8 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/FormatterIntegrationTest.java @@ -125,7 +125,9 @@ public FormatterIntegrationTest(String name, String input, String expected) { @Test public void format() { try { - String output = new Formatter().formatSource(input); + Formatter formatter = new Formatter(); + String output = formatter.formatSource(input); + output = StringWrapper.wrap(output, formatter); assertEquals("bad output for " + name, expected, output); } catch (FormatterException e) { fail(String.format("Formatter crashed on %s: %s", name, e.getMessage())); diff --git a/core/src/test/java/com/google/googlejavaformat/java/MainTest.java b/core/src/test/java/com/google/googlejavaformat/java/MainTest.java index fa23486fd..a7e10fee7 100644 --- a/core/src/test/java/com/google/googlejavaformat/java/MainTest.java +++ b/core/src/test/java/com/google/googlejavaformat/java/MainTest.java @@ -525,8 +525,8 @@ public void reflowLongStrings() throws Exception { "class T {", " String s =", " \"one long incredibly unbroken sentence moving from topic to topic so that no one had" - + " a\"", - " + \" chance to interrupt\";", + + " a chance\"", + " + \" to interrupt\";", "}", "", }; diff --git a/core/src/test/resources/com/google/googlejavaformat/java/testdata/B173808510.input b/core/src/test/resources/com/google/googlejavaformat/java/testdata/B173808510.input new file mode 100644 index 000000000..e3e849399 --- /dev/null +++ b/core/src/test/resources/com/google/googlejavaformat/java/testdata/B173808510.input @@ -0,0 +1,8 @@ +class B173808510 { + // b/173808510 + @FlagSpec( + name = "myFlag", + help = + "areallyreallyreallyreallyreallyreallyreallyreallyreallyreallyreallyreallyloongword word1 word2") + Flag dummy = null; +} diff --git a/core/src/test/resources/com/google/googlejavaformat/java/testdata/B173808510.output b/core/src/test/resources/com/google/googlejavaformat/java/testdata/B173808510.output new file mode 100644 index 000000000..45a939efb --- /dev/null +++ b/core/src/test/resources/com/google/googlejavaformat/java/testdata/B173808510.output @@ -0,0 +1,9 @@ +class B173808510 { + // b/173808510 + @FlagSpec( + name = "myFlag", + help = + "areallyreallyreallyreallyreallyreallyreallyreallyreallyreallyreallyreallyloongword word1" + + " word2") + Flag dummy = null; +} diff --git a/core/src/test/resources/com/google/googlejavaformat/java/testdata/LiteralReflow.input b/core/src/test/resources/com/google/googlejavaformat/java/testdata/LiteralReflow.input new file mode 100644 index 000000000..2aa4de330 --- /dev/null +++ b/core/src/test/resources/com/google/googlejavaformat/java/testdata/LiteralReflow.input @@ -0,0 +1,44 @@ +class LiteralReflow { + static class TestLineBreak { + String doesNotBreakAt100 = "A very long long long long long long long long long loong sentence"; + String breaksAt101 = "A very long long long long long long long long long long loooong sentence"; + } + + static class TestReflowLimit { + String doesNotReflowAt100 = + "A very long long long long long long long long long long long long long looooong sentence"; + String reflowsWhenLongerThan100 = + "A very long long long long long long long long long long long long long long long sentence"; + } + + static class TestReflowLocation { + String accommodatesWordsUpTo100 = + "A very long long long long long long long long long long long long long long long looooong sentence"; + String breaksBeforeWordsReach101 = + "A very long long long long long long long long long long long long long long long loooooong sentence"; + } + + static class Test2LineReflowLimit { + String doesNotReflowEitherLinesAt100 = + "A very long long long long long long long long long long long long long looooong sentence. And a second very long long long long long long long long long long loong sentence"; + String reflowsLastLineAt101 = + "A very long long long long long long long long long long long long long looooong sentence. And a second very long long long long long long long long long long looong sentence"; + } + + static class TestWithTrailingCharacters { + String fitsLastLineUpTo100WithTrailingCharacters = + f( + f( + "A very long long long long long long long long long long long long loong sentence. And a second very long long long long long long long long loong sentence")); + String reflowsLastLineToAccommodateTrailingCharacters = + f( + f( + "A very long long long long long long long long long long long long loong sentence. And a second very long long long long long long long long looong sentence")); + // Tests an off-by-one issue, but see b/179561701 for a similar issue that is not yet fixed + String doesNotOverTriggerLastLineReflow = + f( + f( + "A very long long long long long long long long long long long long loong sentence." + + " And a second very loong sentence with trailing a a a a a a a a a a a a a a a")); + } +} diff --git a/core/src/test/resources/com/google/googlejavaformat/java/testdata/LiteralReflow.output b/core/src/test/resources/com/google/googlejavaformat/java/testdata/LiteralReflow.output new file mode 100644 index 000000000..50ed7bda1 --- /dev/null +++ b/core/src/test/resources/com/google/googlejavaformat/java/testdata/LiteralReflow.output @@ -0,0 +1,55 @@ +class LiteralReflow { + static class TestLineBreak { + String doesNotBreakAt100 = "A very long long long long long long long long long loong sentence"; + String breaksAt101 = + "A very long long long long long long long long long long loooong sentence"; + } + + static class TestReflowLimit { + String doesNotReflowAt100 = + "A very long long long long long long long long long long long long long looooong sentence"; + String reflowsWhenLongerThan100 = + "A very long long long long long long long long long long long long long long long" + + " sentence"; + } + + static class TestReflowLocation { + String accommodatesWordsUpTo100 = + "A very long long long long long long long long long long long long long long long looooong" + + " sentence"; + String breaksBeforeWordsReach101 = + "A very long long long long long long long long long long long long long long long" + + " loooooong sentence"; + } + + static class Test2LineReflowLimit { + String doesNotReflowEitherLinesAt100 = + "A very long long long long long long long long long long long long long looooong sentence." + + " And a second very long long long long long long long long long long loong sentence"; + String reflowsLastLineAt101 = + "A very long long long long long long long long long long long long long looooong sentence." + + " And a second very long long long long long long long long long long looong" + + " sentence"; + } + + static class TestWithTrailingCharacters { + String fitsLastLineUpTo100WithTrailingCharacters = + f( + f( + "A very long long long long long long long long long long long long loong sentence." + + " And a second very long long long long long long long long loong sentence")); + String reflowsLastLineToAccommodateTrailingCharacters = + f( + f( + "A very long long long long long long long long long long long long loong sentence." + + " And a second very long long long long long long long long looong" + + " sentence")); + // Tests an off-by-one issue, but see b/179561701 for a similar issue that is not yet fixed + String doesNotOverTriggerLastLineReflow = + f( + f( + "A very long long long long long long long long long long long long loong sentence." + + " And a second very loong sentence with trailing a a a a a a a a a a a a a a" + + " a")); + } +}