diff --git a/changelog/@unreleased/pr-192.v2.yml b/changelog/@unreleased/pr-192.v2.yml new file mode 100644 index 000000000..ac8aa99b0 --- /dev/null +++ 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 index 949193e88..c768ac467 100644 --- 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 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 peekToken(int skip) { ImmutableList 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 index e5f6a6fc1..b798d6524 100644 --- 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 operands = new ArrayList<>(); List 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 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 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> rows, int column, int atLeastM) { Multiset nodeTypes = HashMultiset.create(); diff --git a/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B21305044.output b/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B21305044.output index 63a0009bd..16f11eeec 100644 --- a/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B21305044.output +++ b/palantir-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 index 7faa33254..5814f56f2 100644 --- 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/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-10.input b/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-10.input new file mode 100644 index 000000000..2ffc0edcc --- /dev/null +++ b/palantir-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/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-10.output b/palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-10.output new file mode 100644 index 000000000..c15a8d2f5 --- /dev/null +++ b/palantir-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"); + } +}