Merge pull request #176 from openjavaformat/fix/join-string-literals

Join adjacent string literals that share a line

Author: abashev

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";
 }