Respect user's existing splits in string concatenations (#192)

Preserve the user's newlines in a long string concatenation (defined as a concatenation with `+` where at least one expression is a literal string). Also allow inlining annotation values.
palantir · Feb 13, 2020 · bab5f62 · bab5f62
1 parent 08684cb
commit bab5f62
Show file tree

Hide file tree

Showing 7 changed files with 107 additions and 13 deletions.
diff --git a/changelog/@unreleased/pr-192.v2.yml b/changelog/@unreleased/pr-192.v2.yml
@@ -0,0 +1,6 @@
+type: improvement
+improvement:
+  description: Preserve the user's newlines in a long string concatenation (defined
+    as a concatenation with `+` where at least one expression is a literal string).
+  links:
+  - https://github.com/palantir/palantir-java-format/pull/192
diff --git a/palantir-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java b/palantir-java-format/src/main/java/com/palantir/javaformat/OpsBuilder.java
@@ -15,11 +15,13 @@
 package com.palantir.javaformat;
 
 import com.google.common.base.MoreObjects;
+import com.google.common.base.Preconditions;
 import com.google.common.collect.ArrayListMultimap;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.Multimap;
 import com.palantir.javaformat.Indent.Const;
+import com.palantir.javaformat.Input.Tok;
 import com.palantir.javaformat.doc.Break;
 import com.palantir.javaformat.doc.BreakTag;
 import com.palantir.javaformat.doc.Comment;
@@ -29,6 +31,7 @@
 import com.palantir.javaformat.doc.NonBreakingSpace;
 import com.palantir.javaformat.doc.State;
 import com.palantir.javaformat.doc.Token;
+import com.palantir.javaformat.doc.Token.RealOrImaginary;
 import com.palantir.javaformat.java.FormatterDiagnostic;
 import com.palantir.javaformat.java.InputMetadata;
 import com.palantir.javaformat.java.InputMetadataBuilder;
@@ -303,6 +306,12 @@ public Optional<String> peekToken() {
         return peekToken(0);
     }
 
+    /** Return whether the last token emitted is followed by a newline. */
+    public boolean mostRecentTokenFollowedByNewline() {
+        Preconditions.checkState(tokenI > 0, "No token was emitted yet");
+        return input.getTokens().get(tokenI - 1).getToksAfter().stream().anyMatch(Tok::isNewline);
+    }
+
     /** Return the text of an upcoming {@link Input.Token}, or absent if there is none. */
     public Optional<String> peekToken(int skip) {
         ImmutableList<? extends Input.Token> tokens = input.getTokens();
@@ -329,7 +338,7 @@ public void token(
         if (token.equals(peekToken().orElse(null))) { // Found the input token. Output it.
             add(Token.make(
                     tokens.get(tokenI++),
-                    Token.RealOrImaginary.REAL,
+                    RealOrImaginary.REAL,
                     plusIndentCommentsBefore,
                     breakAndIndentTrailingComment));
         } else {

diff --git a/palantir-java-format/src/main/java/com/palantir/javaformat/java/JavaInputAstVisitor.java b/palantir-java-format/src/main/java/com/palantir/javaformat/java/JavaInputAstVisitor.java
@@ -84,6 +84,7 @@
 import java.util.Map;
 import java.util.Optional;
 import java.util.Set;
+import java.util.function.IntFunction;
 import java.util.regex.Pattern;
 import java.util.stream.Stream;
 import javax.annotation.Nullable;
@@ -1144,18 +1145,25 @@ public Void visitBinary(BinaryTree node, Void unused) {
         List<ExpressionTree> operands = new ArrayList<>();
         List<String> operators = new ArrayList<>();
         walkInfix(precedence(node), node, operands, operators);
-        FillMode fillMode = hasOnlyShortItems(operands) ? INDEPENDENT : UNIFIED;
+        boolean isStringConcat = isStringConcat(node);
+        boolean shouldPreserveNewlines = isStringConcat && lineSpan(node) > 2;
+        FillMode fillMode = hasOnlyShortItems(operands) || isStringConcat ? INDEPENDENT : UNIFIED;
+
         builder.open(
                 plusFour,
                 BreakBehaviours.breakThisLevel(),
                 LastLevelBreakability.ACCEPT_INLINE_CHAIN_IF_SIMPLE_OTHERWISE_CHECK_INNER);
         scan(operands.get(0), null);
         int operatorsN = operators.size();
+        boolean shouldEnforceNewline = builder.mostRecentTokenFollowedByNewline();
         for (int i = 0; i < operatorsN; i++) {
-            builder.breakOp(fillMode, " ", ZERO);
+            FillMode nextFillMode = shouldPreserveNewlines && shouldEnforceNewline ? UNIFIED : fillMode;
+            builder.breakOp(nextFillMode, " ", ZERO);
             builder.op(operators.get(i));
+            shouldEnforceNewline = builder.mostRecentTokenFollowedByNewline();
             builder.space();
             scan(operands.get(i + 1), null);
+            shouldEnforceNewline = shouldEnforceNewline || builder.mostRecentTokenFollowedByNewline();
         }
         builder.close();
         return null;
@@ -1779,7 +1787,10 @@ boolean visitSingleMemberAnnotation(AnnotationTree node) {
             return false;
         }
         boolean isArrayInitializer = value.getKind() == NEW_ARRAY;
-        builder.open(isArrayInitializer ? ZERO : plusFour);
+        builder.open(
+                isArrayInitializer ? ZERO : plusFour,
+                BreakBehaviours.preferBreakingLastInnerLevel(true),
+                LastLevelBreakability.CHECK_INNER);
         token("@");
         scan(node.getAnnotationType(), null);
         token("(");
@@ -3148,12 +3159,35 @@ private boolean isFormatMethod(List<? extends ExpressionTree> arguments) {
         if (arguments.size() < 2) {
             return false;
         }
-        return isStringConcat(arguments.get(0));
+        return isFormatString(arguments.get(0));
     }
 
     private static final Pattern FORMAT_SPECIFIER = Pattern.compile("%|\\{[0-9]\\}");
 
     private boolean isStringConcat(ExpressionTree first) {
+        final boolean[] stringConcat = {false};
+        new TreeScanner() {
+            @Override
+            public void scan(JCTree tree) {
+                if (tree == null) {
+                    return;
+                }
+                switch (tree.getKind()) {
+                    case STRING_LITERAL:
+                        stringConcat[0] = true;
+                        break;
+                    case PLUS:
+                        super.scan(tree);
+                        break;
+                    default:
+                        break;
+                }
+            }
+        }.scan((JCTree) first);
+        return stringConcat[0];
+    }
+
+    private boolean isFormatString(ExpressionTree first) {
         final boolean[] stringLiteral = {true};
         final boolean[] formatString = {false};
         new TreeScanner() {
@@ -3268,6 +3302,17 @@ private Integer actualColumn(ExpressionTree expression) {
         return positionToColumnMap.get(builder.actualStartColumn(getStartPosition(expression)));
     }
 
+    /** How many lines does this node take up in the input. Returns at least 1. */
+    int lineSpan(Tree node) {
+        IntFunction<Integer> lineNumberAt = tokenPosition -> {
+            Input.Token token = builder.getInput().getPositionTokenMap().get(tokenPosition);
+            return builder.getInput().getLineNumber(token.getTok().getPosition());
+        };
+        return lineNumberAt.apply(getEndPosition(node, getCurrentPath()))
+                - lineNumberAt.apply(getStartPosition(node))
+                + 1;
+    }
+
     /** Returns true if {@code atLeastM} of the expressions in the given column are the same kind. */
     private static boolean expressionsAreParallel(List<List<ExpressionTree>> rows, int column, int atLeastM) {
         Multiset<Tree.Kind> nodeTypes = HashMultiset.create();

diff --git a/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B21305044.output b/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B21305044.output
@@ -17,11 +17,9 @@ class B21305044 {
                         ? super
                                 @Crazy(
                                         key = value,
-                                        other =
-                                                @Crazier({
-                                                    1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19,
-                                                    20
-                                                }))
+                                        other = @Crazier({
+                                            1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20
+                                        }))
                                 Object>
                 c;
     }

diff --git a/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/S.output b/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/S.output
@@ -11,9 +11,8 @@ class S {
 
     int x = 0;
 
-    @SingleMemberAnnotation(
-            0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0
-                    + 0 + 0 + 0 + 0 + 0)
+    @SingleMemberAnnotation(0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0
+            + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0 + 0)
     S() {
         super();
     }

diff --git a/...ir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-10.input b/...ir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-10.input
@@ -0,0 +1,20 @@
+class Palantir10 {
+    // We preserve input newlines for this long string concatenation (more than 2 lines)
+    @SqlUpdate("CREATE TABLE things "
+            + "(id VARCHAR(" + MAX_LENGTH + ") NOT NULL, "
+            + "title VARCHAR(" + MAX_LENGTH + ")  NOT NULL, "
+            + "description VARCHAR(" + MAX_DESCRIPTION_LENGTH + ") NOT NULL, "
+            + "standard BOOLEAN NOT NULL, "
+            + "documentJson VARCHAR(" + MAX_DOCUMENT_LENGTH + ") NOT NULL, "
+            + "PRIMARY KEY (id))")
+    void createTable();
+
+    void foo() {
+        // Test that we still reflow short string concatenations (spanning exactly 2 lines)
+        foo(
+                "foo"
+                    + "bar" + THING + "baz");
+        foo("foo"
+                + "bar" + THING + "baz");
+    }
+}
diff --git a/...r-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-10.output b/...r-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-10.output
@@ -0,0 +1,17 @@
+class Palantir10 {
+    // We preserve input newlines for this long string concatenation (more than 2 lines)
+    @SqlUpdate("CREATE TABLE things "
+            + "(id VARCHAR(" + MAX_LENGTH + ") NOT NULL, "
+            + "title VARCHAR(" + MAX_LENGTH + ")  NOT NULL, "
+            + "description VARCHAR(" + MAX_DESCRIPTION_LENGTH + ") NOT NULL, "
+            + "standard BOOLEAN NOT NULL, "
+            + "documentJson VARCHAR(" + MAX_DOCUMENT_LENGTH + ") NOT NULL, "
+            + "PRIMARY KEY (id))")
+    void createTable();
+
+    void foo() {
+        // Test that we still reflow short string concatenations (spanning exactly 2 lines)
+        foo("foo" + "bar" + THING + "baz");
+        foo("foo" + "bar" + THING + "baz");
+    }
+}