Join adjacent string literals that share a line

When a concatenation of string literals ended up on one line, the +
between them stayed, as in "one " + "two". The string wrapping pass
only looked at lines past the column limit, so the output depended on
where the author had broken the string. StringWrapper now joins each
run of adjacent string literals that share a line into one literal,
whether the author wrote them so or the layout brought them together,
and the code is laid out again around it. A line with a quote on both
sides of a + now sets the pass off, and the pass returns at once when
it finds nothing to do.

Literals stay apart where joining would lose or change something: on
different lines, which is wrapping done on purpose; with a comment
between them; at a text block; on a line that ends in a line comment,
which may speak of them as they are written (a NON-NLS marker counts
the literals of its line, and a table of literals has a comment for
each row); where the left one ends in an octal escape of one or two
digits and the right one starts with an octal digit ("\1" + "2" is two
characters, "\12" one); and where either holds a Unicode escape. A run
that a reflowed chain or a text block overlaps is left to that pass.

From palantir#1781 by Zayan Khan, closed there
unmerged, which addresses palantir#68. Adapted
while bringing it over: the line comment rule and the Unicode escape
rule are new. Without the first, java/util/LocaleISOData.java of the
JDK, a table of rows such as "aa" + "aar" // Afar, became a column of
"aaaar" // Afar, and the NON-NLS markers of #23 lost their literals.
Without the second, "\u005c1" + "2" became "\u005c12", one character
instead of two, which the AST check rejected, so the file failed to
format. The goldens and unit tests became two cases, the expected
outputs of NON-NLS and palantir-10 join their literals, and the
changelog entry is left out.

Of the 15,747 files of the JDK 21 sources, 787 format differently, in
2,303 places, and each is a join: with the + between two literals
taken out and blanks ignored, both versions read the same. 737 files
join on lines that stay as they are; in 167 the code is then laid out
again, mostly a call whose argument now fits on the line of the call.
The files that change on a second run go down from 67 to 48, none of
them new: in 19, a second run moved the pieces of a chain about that
are now joined in the first.

Tests on JDK 21: FormatterCasesTest 887 -> 889; 1410 tests in the
module, 1471 in all modules.

Fixes #148.

Author: ZayanKhan-12

open-java-format/src/main/java/com/palantir/javaformat/java/StringWrapper.java

@@ -53,7 +53,7 @@
 import java.util.stream.Collectors;
 import javax.annotation.Nullable;
 
-/** Wraps string literals that exceed the column limit. */
+/** Wraps string literals that exceed the column limit, and joins adjacent ones that share a line. */
 public final class StringWrapper {
 
     public static final String TEXT_BLOCK_DELIMITER = "\"\"\"";
@@ -66,7 +66,10 @@ public final class StringWrapper {
      */
     private static final int MAX_ROUNDS = 5;
 
-    /** Reflows string literals in the given Java source code that extend past the given column limit. */
+    /**
+     * Reflows string literals in the given Java source code that extend past the given column limit, and joins
+     * adjacent string literals that share a line.
+     */
     static String wrap(final int columnLimit, String input, Formatter formatter) throws FormatterException {
         String result = wrapOnce(columnLimit, input, formatter);
         for (int round = 1; round < MAX_ROUNDS && !result.equals(input); round++) {
@@ -86,6 +89,10 @@ private static String wrapOnce(final int columnLimit, String input, Formatter fo
         // style, eight in OJF.
         var continuationIndent = 4 * formatter.options().indentationMultiplier();
         TreeRangeMap<Integer, String> replacements = getReflowReplacements(columnLimit, continuationIndent, input);
+        if (replacements.asMapOfRanges().isEmpty()) {
+            // needWrapping is an over-approximation, so there may be nothing to do after all.
+            return input;
+        }
         String firstPass =
                 formatter.formatSource(input, replacements.asMapOfRanges().keySet());
 
@@ -179,9 +186,105 @@ protected TreeRangeMap<Integer, String> getReflowReplacements() {
             TreeRangeMap<Integer, String> replacements = TreeRangeMap.create();
             indentTextBlocks(replacements, textBlocks);
             wrapLongStrings(replacements, longStringLiterals, stringLiterals);
+            // Last: it leaves alone the literals that the passes above rewrite.
+            joinAdjacentLiterals(replacements);
             return replacements;
         }
 
+        /**
+         * Joins adjacent string literals that share a line, so that {@code "one " + "two"} becomes {@code "one two"}:
+         * there the {@code +} breaks nothing, whether the author wrote it or the formatter brought the literals onto
+         * one line. Literals on different lines are wrapped on purpose and stay as they are.
+         */
+        private void joinAdjacentLiterals(TreeRangeMap<Integer, String> replacements) {
+            new ConcatenationScanner(replacements).scan(new TreePath(unit), null);
+        }
+
+        private class ConcatenationScanner extends TreePathScanner<Void, Void> {
+
+            private final TreeRangeMap<Integer, String> replacements;
+
+            ConcatenationScanner(TreeRangeMap<Integer, String> replacements) {
+                this.replacements = replacements;
+            }
+
+            @Override
+            public Void visitBinary(BinaryTree node, Void unused) {
+                // Only the root of a chain of +, so that `a + b + c` is flattened once.
+                if (node.getKind() == Kind.PLUS
+                        && getCurrentPath().getParentPath().getLeaf().getKind() != Kind.PLUS) {
+                    joinRuns(flattenExpressionTree(node));
+                }
+                return super.visitBinary(node, null);
+            }
+
+            /** Replaces each longest run of adjacent literals that can be joined with the one literal they make. */
+            private void joinRuns(List<Tree> flat) {
+                var start = 0;
+                while (start < flat.size()) {
+                    if (!isJoinableLiteral(flat.get(start))) {
+                        start++;
+                        continue;
+                    }
+                    var end = start + 1;
+                    while (end < flat.size() && canJoinWithPrevious(flat.get(end - 1), flat.get(end))) {
+                        end++;
+                    }
+                    if (end - start > 1) {
+                        putJoinReplacement(flat.subList(start, end));
+                    }
+                    start = end;
+                }
+            }
+
+            private boolean canJoinWithPrevious(Tree previous, Tree next) {
+                return isJoinableLiteral(next)
+                        && lineMap.getLineNumber(getEndPosition(unit, previous))
+                                == lineMap.getLineNumber(getStartPosition(next))
+                        && noComments(input, unit, previous, next)
+                        && !joiningWouldChangeEscapes(literalText(previous), literalText(next))
+                        && !endsInLineComment(getStartPosition(next));
+            }
+
+            private boolean isJoinableLiteral(Tree tree) {
+                return tree.getKind() == Kind.STRING_LITERAL
+                        // indentTextBlocks re-indents a text block, which cannot be spliced into a literal.
+                        && !input.startsWith(TEXT_BLOCK_DELIMITER, getStartPosition(tree));
+            }
+
+            /**
+             * Whether the line of a literal that starts at a position ends in a line comment. The literals of such a
+             * line stay as they are written, because the comment may speak of them that way: a NON-NLS marker counts
+             * the literals of its line, see {@link NonNlsMarkers}, and a table of literals has a comment for each row.
+             */
+            private boolean endsInLineComment(int position) {
+                var lineEnd = position;
+                while (lineEnd < input.length() && Newlines.hasNewlineAt(input, lineEnd) == -1) {
+                    lineEnd++;
+                }
+                return lineCommentStart(input.substring(position, lineEnd)) >= 0;
+            }
+
+            private void putJoinReplacement(List<Tree> run) {
+                var range =
+                        Range.closedOpen(getStartPosition(run.get(0)), getEndPosition(unit, run.get(run.size() - 1)));
+                if (!replacements.subRangeMap(range).asMapOfRanges().isEmpty()) {
+                    // A text block or a reflowed chain overlaps it; that pass wins.
+                    return;
+                }
+                var joined = new StringBuilder("\"");
+                for (var tree : run) {
+                    joined.append(literalText(tree));
+                }
+                replacements.put(range, joined.append('"').toString());
+            }
+
+            /** The source text of a literal between its quotes, with its escapes as they are written. */
+            private String literalText(Tree tree) {
+                return input.substring(getStartPosition(tree) + 1, getEndPosition(unit, tree) - 1);
+            }
+        }
+
         private class LongStringsAndTextBlockScanner extends TreePathScanner<Void, Void> {
 
             private final List<TreePath> longStringLiterals;
@@ -831,6 +934,62 @@ private static List<Tree> flattenExpressionTree(Tree parent) {
         return flat;
     }
 
+    /**
+     * Whether appending the source text {@code right} to {@code left} would change what it stands for. That happens
+     * when {@code left} ends in an octal escape short enough to take in another digit: {@code "\1" + "2"} holds
+     * {@code \1} and then {@code 2}, but {@code "\12"} is a single character. An octal escape is at most three digits,
+     * so one of three is safe, as is a run of digits that is not preceded by an odd number of backslashes, which is
+     * text rather than an escape. It also happens when either holds a Unicode escape: javac reads those before the
+     * escapes of a literal, so one can stand for the backslash or the digit of such an escape.
+     */
+    private static boolean joiningWouldChangeEscapes(String left, String right) {
+        if (left.contains("\\u") || right.contains("\\u")) {
+            return true;
+        }
+        if (right.isEmpty() || !isOctalDigit(right.charAt(0))) {
+            return false;
+        }
+        var index = left.length();
+        while (index > 0 && isOctalDigit(left.charAt(index - 1))) {
+            index--;
+        }
+        var digits = left.length() - index;
+        if (digits == 0 || digits > 2) {
+            return false;
+        }
+        var backslashes = 0;
+        while (index - backslashes > 0 && left.charAt(index - backslashes - 1) == '\\') {
+            backslashes++;
+        }
+        return backslashes % 2 == 1;
+    }
+
+    private static boolean isOctalDigit(char character) {
+        return character >= '0' && character <= '7';
+    }
+
+    /**
+     * Whether a line may hold two adjacent string literals to join: a {@code +} with a quote right before and after
+     * it, blanks aside. A cheap over-approximation that keeps the fast paths fast; the {@link Reflower} decides what
+     * is joined.
+     */
+    private static boolean mayHaveJoinableLiterals(String line) {
+        for (var plus = line.indexOf('+'); plus != -1; plus = line.indexOf('+', plus + 1)) {
+            var before = plus - 1;
+            while (before >= 0 && (line.charAt(before) == ' ' || line.charAt(before) == '\t')) {
+                before--;
+            }
+            var after = plus + 1;
+            while (after < line.length() && (line.charAt(after) == ' ' || line.charAt(after) == '\t')) {
+                after++;
+            }
+            if (before >= 0 && line.charAt(before) == '"' && after < line.length() && line.charAt(after) == '"') {
+                return true;
+            }
+        }
+        return false;
+    }
+
     private static boolean noComments(String input, JCTree.JCCompilationUnit unit, Tree one, Tree two) {
         return STRING_CONCAT_DELIMITER.matchesAllOf(
                 input.subSequence(getEndPosition(unit, one), getStartPosition(two)));
@@ -848,15 +1007,18 @@ private static int getStartPosition(Tree tree) {
     }
 
     /**
-     * Returns true if any lines in the given Java source exceed the column limit or contain text blocks.
-     * Keep this method and {@code linesNeedWrapping} in line.
+     * Returns true if any lines in the given Java source exceed the column limit, contain text blocks or may hold
+     * adjacent string literals to join. Keep this method and {@code linesNeedWrapping} in line.
      * */
     private static boolean needWrapping(int columnLimit, String input) {
         // TODO(cushon): consider adding Newlines.lineIterable?
         Iterator<String> it = Newlines.lineIterator(input);
         while (it.hasNext()) {
             String line = it.next();
-            if (line.length() > columnLimit || line.contains(TEXT_BLOCK_DELIMITER)) {
+            if (line.length() > columnLimit
+                    || line.contains(TEXT_BLOCK_DELIMITER)
+                    // Literals joined into a line that fits leave no over-long line to set off the wrapping.
+                    || mayHaveJoinableLiterals(line)) {
                 return true;
             }
         }
@@ -865,8 +1027,8 @@ private static boolean needWrapping(int columnLimit, String input) {
 
     /**
      * Returns true if the lines containing the {@code initialRangesToChange} in the given Java source exceed the
-     * column limit or contain text blocks. Used by the Intellij plugin to check if we need to run StringWrapper.wrap.
-     * Keep this method and {@code needWrapping} in line.
+     * column limit, contain text blocks or may hold adjacent string literals to join. Used by the Intellij plugin to
+     * check if we need to run StringWrapper.wrap. Keep this method and {@code needWrapping} in line.
      * */
     public static boolean linesNeedWrapping(int columnLimit, String input, RangeSet<Integer> initialRangesToChange) {
         // TODO(cushon): consider adding Newlines.lineIterable?
@@ -876,7 +1038,7 @@ public static boolean linesNeedWrapping(int columnLimit, String input, RangeSet<
         boolean insideTextBlock = false;
         while (it.hasNext()) {
             String line = it.next();
-            if (line.length() > columnLimit) {
+            if (line.length() > columnLimit || mayHaveJoinableLiterals(line)) {
                 linesToChange.add(Range.closedOpen(i, i + 1));
             }
             if (!insideTextBlock && line.contains(TEXT_BLOCK_DELIMITER)) {

open-java-format/src/test/resources/test-cases/stringwrapper/adjacentLiteralsAreJoined.case

@@ -0,0 +1,83 @@
+# Adjacent string literals that share a line become one literal: as an argument, around other operands of the +, in
+# parentheses, after a text block, which stays as it is, and in an annotation and a case label. A chain that the
+# formatter brings onto one line is joined too, and the code is laid out again around the joined literal.
+# Escapes are copied as they are written: an octal escape of three digits cannot take in the digit after it, a digit
+# after an escaped backslash is text, and 8 is not an octal digit.
+----------
+class T {
+  void brokenByTheFormatter() {
+    throw new SafeIllegalArgumentException("Invalid recipient field - contains multiple "
+        + "foreign key properties.", UnsafeArg.of("field", maybeField));
+  }
+
+  void runsAroundOtherOperands(String value) {
+    foo("one "+"two "+ value +"three "  +  "four");
+    int length = ( "left"+"right" ).length() + count;
+    log("a" + 'b' + "c" + 1 + 2 + "d"
+        + "e");
+  }
+
+  String twoLines() {
+    return "a concatenation that the formatter "
+        + "pulls onto one line";
+  }
+
+  void escapes() {
+    String escapes = "tab\there " + "quote\"there " +  "backslash\\" + "1";
+    String maximal = "c" + "\377"+"7" + "d";
+    String digits = "12" + "3"+"\1" + "8";
+  }
+
+  void nextToATextBlock() {
+    String block = """
+        text
+        """+"tail"+ " more";
+  }
+
+  @SuppressWarnings("unchecked"
+      + "")
+  void constants(String key) {
+    switch (key) {
+      case "first " + "case" -> {}
+      default -> {}
+    }
+  }
+}
+----------
+class T {
+    void brokenByTheFormatter() {
+        throw new SafeIllegalArgumentException(
+                "Invalid recipient field - contains multiple foreign key properties.",
+                UnsafeArg.of("field", maybeField));
+    }
+
+    void runsAroundOtherOperands(String value) {
+        foo("one two " + value + "three four");
+        int length = ("leftright").length() + count;
+        log("a" + 'b' + "c" + 1 + 2 + "de");
+    }
+
+    String twoLines() {
+        return "a concatenation that the formatter pulls onto one line";
+    }
+
+    void escapes() {
+        String escapes = "tab\there quote\"there backslash\\1";
+        String maximal = "c\3777d";
+        String digits = "123\18";
+    }
+
+    void nextToATextBlock() {
+        String block = """
+            text
+            """ + "tail more";
+    }
+
+    @SuppressWarnings("unchecked")
+    void constants(String key) {
+        switch (key) {
+            case "first case" -> {}
+            default -> {}
+        }
+    }
+}

open-java-format/src/test/resources/test-cases/stringwrapper/adjacentLiteralsThatStayApart.case

@@ -0,0 +1,43 @@
+# Adjacent string literals on one line stay apart where joining them would lose something or change their text: a
+# comment between them, a line comment at the end of their line, which may speak of them as they are written (a table
+# with a comment for each row, a NON-NLS marker that counts the literals of its line), an octal escape of one or two
+# digits that would take in the digit after it, and a Unicode escape, which javac reads before the escapes of the
+# literal, so that it can stand for such a digit or backslash.
+----------
+class T {
+  void comments() {
+    String between = "left "   /* keep me here */+"right";
+    String table = "aa"+ "aar" // Afar
+        + "ab" + "abk"; // Abkhazian
+    log("first " + "half", "second "+"half"); // the call logs two halves
+  }
+
+  void shortOctalEscapes() {
+    String octal = "a" + "\1"+ "2" + "b";
+    String twoDigits = "\12"+"3" + "\0"  + "0";
+  }
+
+  void unicodeEscapes() {
+    String backslash = "\u005c1"+ "2";
+    String digit = "\1" +   "\u0032";
+  }
+}
+----------
+class T {
+    void comments() {
+        String between = "left " /* keep me here */ + "right";
+        String table = "aa" + "aar" // Afar
+                + "ab" + "abk"; // Abkhazian
+        log("first " + "half", "second " + "half"); // the call logs two halves
+    }
+
+    void shortOctalEscapes() {
+        String octal = "a\1" + "2b";
+        String twoDigits = "\12" + "3\0" + "0";
+    }
+
+    void unicodeEscapes() {
+        String backslash = "\u005c1" + "2";
+        String digit = "\1" + "\u0032";
+    }
+}

open-java-format/src/test/resources/test-cases/testdata/NON-NLS-cr.case

@@ -1 +1 @@
-----------package test;class MultilineStringConstant {    // NON-NLS comments are required for i18n, it's important they are kept with their strings.    private static final String MULTIPLE_LINE_NON_NLS =            "field_0," + //$NON-NLS-1$                    "field_1," + //$NON-NLS-1$                    "field_2," + //$NON-NLS-1$                    "field_3," + //$NON-NLS-1$                    "field_4"; //$NON-NLS-1$    private static final String MULTIPLE_LINE_NO_COMMENT =            "field_0," +                    "field_1," +                    "field_2," +                    "field_3," +                    "field_4";}----------package test;class MultilineStringConstant {    // NON-NLS comments are required for i18n, it's important they are kept with their strings.    private static final String MULTIPLE_LINE_NON_NLS = "field_0," //$NON-NLS-1$            + "field_1," //$NON-NLS-1$            + "field_2," //$NON-NLS-1$            + "field_3," //$NON-NLS-1$            + "field_4"; //$NON-NLS-1$    private static final String MULTIPLE_LINE_NO_COMMENT =            "field_0," + "field_1," + "field_2," + "field_3," + "field_4";}
+----------package test;class MultilineStringConstant {    // NON-NLS comments are required for i18n, it's important they are kept with their strings.    private static final String MULTIPLE_LINE_NON_NLS =            "field_0," + //$NON-NLS-1$                    "field_1," + //$NON-NLS-1$                    "field_2," + //$NON-NLS-1$                    "field_3," + //$NON-NLS-1$                    "field_4"; //$NON-NLS-1$    private static final String MULTIPLE_LINE_NO_COMMENT =            "field_0," +                    "field_1," +                    "field_2," +                    "field_3," +                    "field_4";}----------package test;class MultilineStringConstant {    // NON-NLS comments are required for i18n, it's important they are kept with their strings.    private static final String MULTIPLE_LINE_NON_NLS = "field_0," //$NON-NLS-1$            + "field_1," //$NON-NLS-1$            + "field_2," //$NON-NLS-1$            + "field_3," //$NON-NLS-1$            + "field_4"; //$NON-NLS-1$    private static final String MULTIPLE_LINE_NO_COMMENT = "field_0,field_1,field_2,field_3,field_4";}

open-java-format/src/test/resources/test-cases/testdata/NON-NLS-crlf.case

@@ -31,6 +31,5 @@ class MultilineStringConstant {
             + "field_3," //$NON-NLS-1$
             + "field_4"; //$NON-NLS-1$
 
-    private static final String MULTIPLE_LINE_NO_COMMENT =
-            "field_0," + "field_1," + "field_2," + "field_3," + "field_4";
+    private static final String MULTIPLE_LINE_NO_COMMENT = "field_0,field_1,field_2,field_3,field_4";
 }

open-java-format/src/test/resources/test-cases/testdata/NON-NLS-lf.case

@@ -31,6 +31,5 @@ class MultilineStringConstant {
             + "field_3," //$NON-NLS-1$
             + "field_4"; //$NON-NLS-1$
 
-    private static final String MULTIPLE_LINE_NO_COMMENT =
-            "field_0," + "field_1," + "field_2," + "field_3," + "field_4";
+    private static final String MULTIPLE_LINE_NO_COMMENT = "field_0,field_1,field_2,field_3,field_4";
 }