Nested method chains also adhere to column limit

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

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;
 
     /**

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();
     }
 

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();
     }

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();

palantir-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();

palantir-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)

palantir-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);
         }
     }

palantir-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());
         }
     }

palantir-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()

palantir-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];
     }
 }

palantir-java-format/src/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()))

palantir-java-format/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());
     }
 }

palantir-java-format/src/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());
+    }
+}

palantir-java-format/src/test/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());
+    }
+}