Nested method chains also adhere to column limit (#123)

Nested method chains now also adhere to column limit. Also, we've made the logic a bit more specific in terms of what are method chains. We now only apply this restricted column limit to breaks that come before actual method calls, not field accesses or parts of a fully qualified class name.
palantir · Jan 14, 2020 · 9a52f03 · 9a52f03
1 parent 6417e10
commit 9a52f03
Show file tree

Hide file tree

Showing 15 changed files with 138 additions and 33 deletions.
diff --git a/changelog/@unreleased/pr-123.v2.yml b/changelog/@unreleased/pr-123.v2.yml
@@ -0,0 +1,5 @@
+type: fix
+fix:
+  description: Nested method chains now also adhere to column limit.
+  links:
+  - https://github.com/palantir/palantir-java-format/pull/123
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
@@ -438,6 +438,11 @@ public void breakOp(FillMode fillMode, String flat, Indent plusIndent, Optional<
         add(Break.make(fillMode, flat, plusIndent, optionalTag));
     }
 
+    /** Emit a self-built {@link Break}. */
+    public void breakOp(Break breakOp) {
+        add(breakOp);
+    }
+
     private int lastPartialFormatBoundary = -1;
 
     /**

diff --git a/palantir-java-format/src/main/java/com/palantir/javaformat/doc/Break.java b/palantir-java-format/src/main/java/com/palantir/javaformat/doc/Break.java
@@ -38,6 +38,9 @@ public abstract class Break extends Doc implements Op {
 
     public abstract Optional<BreakTag> optTag();
 
+    /** If this break is restricted by its parent {@link Level}'s {@link Level#getColumnLimitBeforeLastBreak()}. */
+    public abstract boolean hasColumnLimit();
+
     /**
      * Make a {@code Break}.
      *
@@ -47,7 +50,12 @@ public abstract class Break extends Doc implements Op {
      * @return the new {@code Break}
      */
     public static Break make(FillMode fillMode, String flat, Indent plusIndent) {
-        return builder().fillMode(fillMode).flat(flat).plusIndent(plusIndent).build();
+        return builder()
+                .fillMode(fillMode)
+                .flat(flat)
+                .plusIndent(plusIndent)
+                .hasColumnLimit(false)
+                .build();
     }
 
     /**
@@ -65,6 +73,7 @@ public static Break make(FillMode fillMode, String flat, Indent plusIndent, Opti
                 .flat(flat)
                 .plusIndent(plusIndent)
                 .optTag(optTag)
+                .hasColumnLimit(false)
                 .build();
     }
 
@@ -78,6 +87,7 @@ public static Break makeForced() {
                 .fillMode(FillMode.FORCED)
                 .flat("")
                 .plusIndent(Indent.Const.ZERO)
+                .hasColumnLimit(false)
                 .build();
     }
 

diff --git a/palantir-java-format/src/main/java/com/palantir/javaformat/doc/Level.java b/palantir-java-format/src/main/java/com/palantir/javaformat/doc/Level.java
@@ -123,25 +123,33 @@ public State computeBreaks(CommentsHelper commentsHelper, int maxWidth, State st
      *     maxWidth}
      */
     private Optional<Integer> tryToFitOnOneLine(int maxWidth, State state, Iterable<Doc> docs) {
-        if (getColumnLimitBeforeLastBreak().isPresent()) {
-            float width = 0.0f;
-            float widthBeforeLastBreak = 0.0f;
-            for (Doc doc : docs) {
-                if (doc instanceof Break) {
-                    widthBeforeLastBreak = width;
+        int column = state.column();
+        int columnBeforeLastBreak = 0; // Not activated by default
+        for (Doc doc : docs) {
+            if (doc instanceof Break && ((Break) doc).hasColumnLimit()) {
+                columnBeforeLastBreak = column;
+            } else if (doc instanceof Level) {
+                // Levels might have nested levels that have a 'columnLimitBeforeLastBreak' set, so recurse.
+                State newState = state.withColumn(column);
+                Level innerLevel = (Level) doc;
+                Optional<Integer> newWidth = innerLevel.tryToFitOnOneLine(maxWidth, newState, innerLevel.getDocs());
+                if (!newWidth.isPresent()) {
+                    return Optional.empty();
                 }
-                width += doc.getWidth();
-            }
-            // Make an additional check that widthBeforeLastBreak fits in the column limit
-            if (state.column() + widthBeforeLastBreak > getColumnLimitBeforeLastBreak().getAsInt()) {
-                return Optional.empty();
+                column = newWidth.get();
+                continue;
             }
+            column += doc.getWidth();
+        }
+        // Make an additional check that widthBeforeLastBreak fits in the column limit
+        if (getColumnLimitBeforeLastBreak().isPresent()
+                && columnBeforeLastBreak > getColumnLimitBeforeLastBreak().getAsInt()) {
+            return Optional.empty();
         }
 
         // Check that the entirety of this level fits on the current line.
-        float thisWidth = getWidth(docs);
-        if (state.column() + thisWidth <= maxWidth) {
-            return Optional.of(state.column() + (int) thisWidth);
+        if (column <= maxWidth) {
+            return Optional.of(column);
         }
         return Optional.empty();
     }
@@ -538,6 +546,10 @@ public OpenOp getOpenOp() {
         return openOp;
     }
 
+    /**
+     * An optional, more restrictive column limit for inner breaks that are marked as {@link Break#hasColumnLimit()}. If
+     * the level is to be considered one-lineable, the last such break must not start at a column higher than this.
+     */
     public OptionalInt getColumnLimitBeforeLastBreak() {
         return openOp.columnLimitBeforeLastBreak();
     }

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
@@ -2727,7 +2727,12 @@ private void visitRegularDot(List<ExpressionTree> items, boolean needDot) {
         for (ExpressionTree e : items) {
             if (needDot) {
                 if (length > minLength) {
-                    builder.breakOp(FillMode.UNIFIED, "", ZERO);
+                    builder.breakOp(Break.builder()
+                            .fillMode(FillMode.UNIFIED)
+                            .flat("")
+                            .plusIndent(ZERO)
+                            .hasColumnLimit(shouldHaveColumnLimit(e))
+                            .build());
                 }
                 token(".");
                 length++;
@@ -2821,7 +2826,13 @@ private void visitDotWithPrefix(
                     fillMode = FillMode.UNIFIED;
                 }
 
-                builder.breakOp(fillMode, "", ZERO, Optional.of(nameTag));
+                builder.breakOp(Break.builder()
+                        .fillMode(fillMode)
+                        .flat("")
+                        .plusIndent(ZERO)
+                        .optTag(Optional.of(nameTag))
+                        .hasColumnLimit(shouldHaveColumnLimit(e))
+                        .build());
                 token(".");
             }
             BreakTag tyargTag = new BreakTag();
@@ -2841,6 +2852,22 @@ private void visitDotWithPrefix(
         builder.close();
     }
 
+    /**
+     * We only want to limit the max column of expressions that are method invocations. All of these expressions below
+     * would have a column limit.
+     *
+     * <ul>
+     *   <li>{@code foo().bar()}
+     *   <li>{@code foo().bar()[0]}
+     *   <li>{@code foo().bar()[0][0]}
+     * </ul>
+     *
+     * Whereas an expression like a name {@code com.palantir.foo.bar.Baz} would not.
+     */
+    private boolean shouldHaveColumnLimit(ExpressionTree expr) {
+        return getArrayBase(expr).getKind() == METHOD_INVOCATION;
+    }
+
     /** Returns the simple names of expressions in a "." chain. */
     private List<String> simpleNames(Deque<ExpressionTree> stack) {
         ImmutableList.Builder<String> simpleNames = ImmutableList.builder();

diff --git a/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B20128760.output b/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B20128760.output
@@ -11,8 +11,9 @@ class B20128760 {
                 .xxxxx();
 
         xxxxxxXxxx(xxx.xxxXxXxxxxxxx(new XxxxxxxxxXxxxxxxx.Xxxxxxx()
-                                .xxxxXxxxXxx(Xxxxx.xxxXxxxxXxxx(new XxxxXx(
-                                        xxxxxxxxx1.xxxXxxxXx().xxxXxxxx() + xxxxxxxxx2.xxxXxxxXx().xxxXxxxx())))
+                                .xxxxXxxxXxx(Xxxxx.xxxXxxxxXxxx(
+                                        new XxxxXx(xxxxxxxxx1.xxxXxxxXx().xxxXxxxx()
+                                                + xxxxxxxxx2.xxxXxxxXx().xxxXxxxx())))
                                 .xxxxx())
                         .xxxXxxxxxx())
                 .xxxxxxxxXxxxxxx();

diff --git a/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B20701054.output b/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B20701054.output
@@ -14,14 +14,37 @@ class B20701054 {
         ImmutableList<String> x = ImmutableList.INSTANCE.add(1).build();
         ImmutableList<String> x = ImmutableList.INSTANCE.add(1).add(2).build();
         ImmutableList<String> x = ImmutableList.INSTANCE.add(1).add(2).add(3).build();
-        ImmutableList<String> x = ImmutableList.INSTANCE.add(1).add(2).add(3).add(4).build();
+        ImmutableList<String> x = ImmutableList.INSTANCE
+                .add(1)
+                .add(2)
+                .add(3)
+                .add(4)
+                .build();
 
         ImmutableList<String> x = ImmutableList.builder().add(1).build();
         ImmutableList<String> x = ImmutableList.builder().add(1).add(2).build();
         ImmutableList<String> x = ImmutableList.builder().add(1).add(2).add(3).build();
-        ImmutableList<String> x = ImmutableList.builder().add(1).add(2).add(3).add(4).build();
-        ImmutableList<String> x = ImmutableList.builder().add(1).add(2).add(3).add(4).add(5).build();
-        ImmutableList<String> x = ImmutableList.builder().add(1).add(2).add(3).add(4).add(5).add(6).build();
+        ImmutableList<String> x = ImmutableList.builder()
+                .add(1)
+                .add(2)
+                .add(3)
+                .add(4)
+                .build();
+        ImmutableList<String> x = ImmutableList.builder()
+                .add(1)
+                .add(2)
+                .add(3)
+                .add(4)
+                .add(5)
+                .build();
+        ImmutableList<String> x = ImmutableList.builder()
+                .add(1)
+                .add(2)
+                .add(3)
+                .add(4)
+                .add(5)
+                .add(6)
+                .build();
 
         ImmutableList<String> x = new ImmutableList.Builder<>()
                 .add(xxxxx)

diff --git a/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B21465217.output b/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B21465217.output
@@ -4,7 +4,9 @@ class B21465217 {
                 BufferedOutputStream bout = new BufferedOutputStream(out2);
                 OutputStreamWriter writer = new OutputStreamWriter(bout, UTF_8___________________________)) {}
 
-        try (Writer sourceWriter = env.getFiler().createSourceFile(qualifiedNamezzzzzzzz).openWriter()) {
+        try (Writer sourceWriter = env.getFiler()
+                .createSourceFile(qualifiedNamezzzzzzzz)
+                .openWriter()) {
             sourceWriter.append(fileContents);
         }
     }

diff --git a/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B22610221.output b/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B22610221.output
@@ -1,6 +1,9 @@
 class B22610221 {
     {
-        for (Map.Entry<Key<Object>, Object> entry : FOO.bar().bazs().<Object>thing(someThing).entrySet()) {
+        for (Map.Entry<Key<Object>, Object> entry : FOO.bar()
+                .bazs()
+                .<Object>thing(someThing)
+                .entrySet()) {
             output.put(entry.getKey(), entry.getValue());
         }
     }

diff --git a/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B24909927.output b/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B24909927.output
@@ -122,8 +122,9 @@ class Xxx {
                                                         .xxxXxxx(XxxxXxxx.XXX)
                                                         .xxxxx())
                                                 .xxxxx())
-                                        .xxxXxxxxxxxxxXxxXxxxXxxxxxxxx(
-                                                XxxxxxxxxxXxxXxxxXxxxxxxxx.xxxXxxxxxx().xxxXxxxxxxxxxXxxxXx(42).xxxxx())
+                                        .xxxXxxxxxxxxxXxxXxxxXxxxxxxxx(XxxxxxxxxxXxxXxxxXxxxxxxxx.xxxXxxxxxx()
+                                                .xxxXxxxxxxxxxXxxxXx(42)
+                                                .xxxxx())
                                         .xxxxx())
                                 .xxxxx())
                         .xxxXxxxxxx(XxxxxxxXxxx.xxxXxxxxxx()

diff --git a/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B32114928.output b/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B32114928.output
@@ -1,6 +1,7 @@
 class B32114928 {
     {
-        Class<T> tClass = (Class<T>) verifyNotNull((ParameterizedType) getClass().getGenericSuperclass())
-                .getActualTypeArguments()[0];
+        Class<T> tClass = (Class<T>)
+                verifyNotNull((ParameterizedType) getClass().getGenericSuperclass())
+                        .getActualTypeArguments()[0];
     }
 }
diff --git a/.../test/resources/com/palantir/javaformat/java/testdata/palantir-deeply-nested-calls.output b/.../test/resources/com/palantir/javaformat/java/testdata/palantir-deeply-nested-calls.output
@@ -5,8 +5,9 @@ class PalantirDeeplyNestedCalls {
                     .foooos(ImmutableList.of(testFieldTwoV1, testFieldThreeV1))
                     .baaars(ImmutableList.of(MagicConfigV1.builder()
                             .baaars(ImmutableList.of(MagicConfigV1.builder()
-                                    .baaars(ImmutableList.of(
-                                            MagicConfigV1.builder().foooos(ImmutableList.of(testFieldFourV1)).build()))
+                                    .baaars(ImmutableList.of(MagicConfigV1.builder()
+                                            .foooos(ImmutableList.of(testFieldFourV1))
+                                            .build()))
                                     .build()))
                             .build()))
                     .build()))

diff --git a/...ormat/src/test/resources/com/palantir/javaformat/java/testdata/palantir-lamda-cast.output b/...ormat/src/test/resources/com/palantir/javaformat/java/testdata/palantir-lamda-cast.output
@@ -10,7 +10,7 @@ public class Foo {
     }
 
     static String baz(Optional<String> optional) {
-        return optional.orElseGet(
-                (String) () -> bar(Optional.of("some thing that is very very very looooooong")).get());
+        return optional.orElseGet((String) () ->
+                bar(Optional.of("some thing that is very very very looooooong")).get());
     }
 }
diff --git a/...test/resources/com/palantir/javaformat/java/testdata/palantir-long-method-chain-arg.input b/...test/resources/com/palantir/javaformat/java/testdata/palantir-long-method-chain-arg.input
@@ -0,0 +1,5 @@
+class PalantirLongMethodChainArg {
+    void foo() {
+        return cache.get(ImmutableRequest.builder().group(group).name(artifact).addRepositories("release-jar").build());
+    }
+}
diff --git a/...est/resources/com/palantir/javaformat/java/testdata/palantir-long-method-chain-arg.output b/...est/resources/com/palantir/javaformat/java/testdata/palantir-long-method-chain-arg.output
@@ -0,0 +1,9 @@
+class PalantirLongMethodChainArg {
+    void foo() {
+        return cache.get(ImmutableRequest.builder()
+                .group(group)
+                .name(artifact)
+                .addRepositories("release-jar")
+                .build());
+    }
+}