Don't always break at method calls with prefix

changelog/@unreleased/pr-94.v2.yml

@@ -0,0 +1,5 @@
+type: fix
+fix:
+  description: Stop indiscriminately stopping an inlining at method calls like `SafeArg.of`.
+  links:
+  - https://github.com/palantir/palantir-java-format/pull/94

palantir-java-format/src/main/java/com/palantir/javaformat/LastLevelBreakability.java

@@ -2,7 +2,6 @@
 
 import com.palantir.javaformat.doc.Break;
 import com.palantir.javaformat.doc.Doc;
-import com.palantir.javaformat.java.JavaInputAstVisitor;
 
 /**
  * How to decide whether to break the last inner level ("this level") of a parent level with {@link
@@ -11,26 +10,21 @@
 public enum LastLevelBreakability {
     /**
      * Default behaviour. When processing a {@link BreakBehaviour.Cases#preferBreakingLastInnerLevel} chain, if we've
-     * arrived at a last level with this breakability, then we should abort the chain.
+     * arrived at a level with this breakability, then we should abort the chain.
      */
     ABORT,
     /**
-     * Unconditionally allow breaking this level. This should only be used when you know that the first non-Level {@link
-     * Doc} inside this level, if you flatten it, is a {@link Break}.
+     * Unconditionally allow ending an inline chain at this level, after which this level may be broken as usual, or a
+     * prefix thereof could be inlined further (if it has the appropriate break behaviour of {@link
+     * BreakBehaviour.Cases#preferBreakingLastInnerLevel}). This should only be used when you know that the first
+     * non-Level {@link Doc} inside this level, if you flatten it, is a {@link Break}.
      */
-    BREAK_HERE,
+    ACCEPT_INLINE_CHAIN,
     /**
      * Delegate to the {@link LastLevelBreakability} of _this_ level's last inner level. Typically, this will be true if
      * this level is not immediately followed by a break (see StartsWithBreakVisitor). Behaves the same as {@link
      * #ABORT} if this level is not {@link BreakBehaviour.Cases#preferBreakingLastInnerLevel}.
      */
     CHECK_INNER,
-    /**
-     * Check whether the <i>first</i> inner level can fit on the same line. This assumes that the next Doc after that
-     * starts with a {@link Break} (see {@link StartsWithBreakVisitor}) and makes sense in contexts like {@link
-     * JavaInputAstVisitor#visitDotWithPrefix} where we want to treat first doc (the longest prefix) as a single entity
-     * to be fit onto the same line.
-     */
-    ONLY_IF_FIRST_LEVEL_FITS,
     ;
 }

palantir-java-format/src/main/java/com/palantir/javaformat/OpenOp.java

@@ -50,6 +50,11 @@ public LastLevelBreakability breakabilityIfLastLevel() {
         return LastLevelBreakability.ABORT;
     }
 
+    @Default
+    public PartialInlineability partialInlineability() {
+        return PartialInlineability.ALWAYS_PARTIALLY_INLINEABLE;
+    }
+
     public abstract Optional<String> debugName();
 
     /** Custom max column limit that contents of this level <em>before the last break</em> may not exceed. */
@@ -58,33 +63,18 @@ public LastLevelBreakability breakabilityIfLastLevel() {
     /**
      * Make an ordinary {@code OpenOp}.
      *
-     * @see #make(Indent, BreakBehaviour, LastLevelBreakability)
+     * @see #builder()
      */
     public static Op make(Indent plusIndent) {
         return builder().plusIndent(plusIndent).build();
     }
 
-    /**
-     * Make an ordinary {@code OpenOp}.
-     *
-     * @param plusIndent the indent for breaks at this level
-     * @param breakBehaviour how to decide whether to break this level or not
-     * @return the {@code OpenOp}
-     */
-    public static Op make(
-            Indent plusIndent, BreakBehaviour breakBehaviour, LastLevelBreakability breakabilityIfLastLevel) {
-        return builder()
-                .plusIndent(plusIndent)
-                .breakBehaviour(breakBehaviour)
-                .breakabilityIfLastLevel(breakabilityIfLastLevel)
-                .build();
-    }
-
     @Override
     public void add(DocBuilder builder) {
         builder.open(this);
     }
 
+    /** @see ImmutableOpenOp.Builder#Builder() */
     public static Builder builder() {
         return new Builder();
     }

palantir-java-format/src/main/java/com/palantir/javaformat/PartialInlineability.java

@@ -0,0 +1,53 @@
+package com.palantir.javaformat;
+
+import com.palantir.javaformat.doc.Break;
+import com.palantir.javaformat.java.JavaInputAstVisitor;
+
+/**
+ * What are the conditions for a level to be partially inlineable. Partial inlining refers to the behaviour of {@link
+ * BreakBehaviours#breakOnlyIfInnerLevelsThenFitOnOneLine} where a level is too large to fit on the current line, but a
+ * prefix thereof is partially inlined onto the current line.
+ *
+ * <p>Specifically, when inlining a level with the above behaviour, the partial inlineability of its first
+ * <em>child</em> level (and <em>that</em> level's first child, recursively) is queried in order to determine if we need
+ * to ensure there's enough room for some additional prefix of that level.
+ *
+ * <p>The reason for this is to prevent degenerate formattings like
+ *
+ * <pre>
+ * Object foo = someSuperLongMethod(some |
+ *         .fully                        |
+ *         .qualified                    |
+ *         .ClassName                    |
+ *         .doSomething());              |
+ * </pre>
+ *
+ * and instead prefer breaking earlier to keep the prefix on the same line, like:
+ *
+ * <pre>
+ * Object foo = someSuperLongMethod(     |
+ *         some.fully.qualified.ClassName|
+ *                 .doSomething());      |
+ * </pre>
+ *
+ * <p>Note that this works as a mandatory access control. Namely, if it's <em>allowed</em> to partially inline a level,
+ * what are the additional conditions that have to be met in order for the inlining to go ahead.
+ */
+public enum PartialInlineability {
+    /**
+     * The level may always be partially inlined, regardless of how much space is left on the current line.
+     *
+     * <p>This is usually only appropriate for levels that start with a direct {@link Break}, as opposed to a Break
+     * that's nested inside some other levels.
+     */
+    ALWAYS_PARTIALLY_INLINEABLE,
+
+    /**
+     * Partially inlineable if the <em>first</em> inner level of this level fits on the current line.
+     *
+     * <p>This assumes that the next Doc after that starts with a {@link Break} (see {@link StartsWithBreakVisitor}) and
+     * makes sense in contexts like {@link JavaInputAstVisitor#visitDotWithPrefix} where we want to treat first doc (the
+     * longest prefix) as a single entity to be fit onto the same line.
+     */
+    IF_FIRST_LEVEL_FITS,
+}

palantir-java-format/src/main/java/com/palantir/javaformat/doc/CountWidthUntilBreakVisitor.java

@@ -17,7 +17,7 @@
 package com.palantir.javaformat.doc;
 
 import com.google.common.base.Preconditions;
-import com.palantir.javaformat.LastLevelBreakability;
+import com.palantir.javaformat.PartialInlineability;
 import com.palantir.javaformat.doc.StartsWithBreakVisitor.Result;
 import java.util.List;
 import java.util.OptionalInt;
@@ -56,7 +56,7 @@ public Float visitBreak(Break doc) {
 
     @Override
     public Float visitLevel(Level level) {
-        if (level.getBreakabilityIfLastLevel() == LastLevelBreakability.ONLY_IF_FIRST_LEVEL_FITS
+        if (level.partialInlineability() == PartialInlineability.IF_FIRST_LEVEL_FITS
                 // If this prefix wouldn't fit on a new line (within the availableWidth), then don't
                 // consider it at all, because there's no point, it would always be broken.
                 && level.getDocs().get(0).getWidth() <= availableWidth) {

palantir-java-format/src/main/java/com/palantir/javaformat/doc/Level.java

@@ -30,6 +30,7 @@
 import com.palantir.javaformat.LastLevelBreakability;
 import com.palantir.javaformat.OpenOp;
 import com.palantir.javaformat.Output;
+import com.palantir.javaformat.PartialInlineability;
 import com.palantir.javaformat.doc.Obs.Exploration;
 import com.palantir.javaformat.doc.StartsWithBreakVisitor.Result;
 import java.util.ArrayList;
@@ -306,52 +307,46 @@ private Optional<State> tryBreakLastLevel(
         //  * the lastLevel wants to be split, i.e. has Breakability.BREAK_HERE, then we continue
         //  * the lastLevel indicates we should check inside it for a potential split candidate.
         //    In this case, recurse rather than go into computeBreaks.
-        if (lastLevel.getBreakabilityIfLastLevel() == LastLevelBreakability.CHECK_INNER) {
-            // Try to fit the entire inner prefix if it's that kind of level.
-            return BreakBehaviours.caseOf(lastLevel.getBreakBehaviour())
-                    .preferBreakingLastInnerLevel(keepIndentWhenInlined -> {
-                        State state2 = state1;
-                        if (keepIndentWhenInlined) {
-                            state2 = state2.withIndentIncrementedBy(lastLevel.getPlusIndent());
-                        }
-                        State state3 = state2;
-                        return explorationNode
-                                .newChildNode(lastLevel, state2)
-                                .maybeExplore("recurse into inner tryBreakLastLevel", state3, exp ->
-                                        lastLevel.tryBreakLastLevel(commentsHelper, maxWidth, state3, exp))
-                                .map(expl -> expl.markAccepted()); // collapse??
-                    })
-                    // We don't know how to fit the inner level on the same line, so bail out.
-                    .otherwise_(Optional.empty());
-
-        } else if (lastLevel.getBreakabilityIfLastLevel() == LastLevelBreakability.ONLY_IF_FIRST_LEVEL_FITS) {
-            // Otherwise, we may be able to check if the first inner level of the lastLevel fits.
-            // This is safe because we assume (and check) that a newline comes after it, even though
-            // it might be nested somewhere deep in the 2nd level.
-
-            float firstLevelWidth = lastLevel.docs.get(0).getWidth();
-            boolean enoughRoom = state1.column() + firstLevelWidth <= maxWidth;
-
-            // Enforce our assumption.
-            if (lastLevel.docs.size() > 1) {
-                assertStartsWithBreakOrEmpty(state, lastLevel.docs.get(1));
-            }
-
-            if (!enoughRoom) {
+        switch (lastLevel.getBreakabilityIfLastLevel()) {
+            case CHECK_INNER:
+                // Try to fit the entire inner prefix if it's that kind of level.
+                return BreakBehaviours.caseOf(lastLevel.getBreakBehaviour())
+                        .preferBreakingLastInnerLevel(keepIndentWhenInlined -> {
+                            State state2 = state1;
+                            if (keepIndentWhenInlined) {
+                                state2 = state2.withIndentIncrementedBy(lastLevel.getPlusIndent());
+                            }
+                            State state3 = state2;
+                            return explorationNode
+                                    .newChildNode(lastLevel, state2)
+                                    .maybeExplore("recurse into inner tryBreakLastLevel", state3, exp ->
+                                            lastLevel.tryBreakLastLevel(commentsHelper, maxWidth, state3, exp))
+                                    .map(expl -> expl.markAccepted()); // collapse??
+                        })
+                        // We don't know how to fit the inner level on the same line, so bail out.
+                        .otherwise_(Optional.empty());
+            case ACCEPT_INLINE_CHAIN:
+                // Ok then, we are allowed to break here, but first verify that we have enough room to inline this last
+                // level's prefix.
+                Preconditions.checkState(
+                        lastLevel.partialInlineability() == PartialInlineability.ALWAYS_PARTIALLY_INLINEABLE,
+                        "tryBreakLastLevel doesn't currently support ending the inlining chain at a level with a "
+                                + "custom inlineability: %s",
+                        lastLevel.openOp);
+                // Note: computeBreaks, not computeBroken, so it can try to do this logic recursively for the
+                // lastLevel
+                return Optional.of(
+                        explorationNode
+                                .newChildNode(lastLevel, state1)
+                                .explore("end tryBreakLastLevel chain", state1, exp ->
+                                        lastLevel.computeBreaks(commentsHelper, maxWidth, state1, exp))
+                                .markAccepted());
+            case ABORT:
                 return Optional.empty();
-            }
-
-            // Else, fall back to computeBreaks which will try both with / without break.
+            default:
+                throw new RuntimeException(
+                        "Unexpected lastLevelBreakability: " + lastLevel.getBreakabilityIfLastLevel());
         }
-
-        // Note: computeBreaks, not computeBroken, so it can try to do this logic recursively for the
-        // lastLevel
-        return Optional.of(
-                explorationNode
-                        .newChildNode(lastLevel, state1)
-                        .explore("end tryBreakLastLevel chain", state1, exp ->
-                                lastLevel.computeBreaks(commentsHelper, maxWidth, state1, exp))
-                        .markAccepted());
     }
 
     private static void assertStartsWithBreakOrEmpty(State state, Doc doc) {
@@ -508,6 +503,10 @@ LastLevelBreakability getBreakabilityIfLastLevel() {
         return openOp.breakabilityIfLastLevel();
     }
 
+    public PartialInlineability partialInlineability() {
+        return openOp.partialInlineability();
+    }
+
     public Optional<String> getDebugName() {
         return openOp.debugName();
     }