Allow inlining method calls if current inlining is simple (#99)

Bring back inlining the prefix of method calls *iff* the levels inlined so far have all been simple. By simple, in practice we mean a single method call (rather than chained method calls), or a single method parameter. This is defined as a property on the Level `OpenOp`s created in `JavaInputAstVisitor`.
palantir · Dec 6, 2019 · 2eab361 · 2eab361
1 parent c2c71e9
commit 2eab361
Show file tree

Hide file tree

Showing 16 changed files with 590 additions and 679 deletions.
diff --git a/changelog/@unreleased/pr-99.v2.yml b/changelog/@unreleased/pr-99.v2.yml
@@ -0,0 +1,7 @@
+type: fix
+fix:
+  description: Bring back inlining the prefix of method calls *iff* the levels inlined
+    so far have all been simple. This fixes the regression introduced in 0.3.4 where
+    long nested method calls would end up with too many indents.
+  links:
+  - https://github.com/palantir/palantir-java-format/pull/99
diff --git a/palantir-java-format/src/main/java/com/palantir/javaformat/LastLevelBreakability.java b/palantir-java-format/src/main/java/com/palantir/javaformat/LastLevelBreakability.java
@@ -26,5 +26,10 @@ public enum LastLevelBreakability {
      * #ABORT} if this level is not {@link BreakBehaviour.Cases#preferBreakingLastInnerLevel}.
      */
     CHECK_INNER,
+    /**
+     * Behaves like {@link #ACCEPT_INLINE_CHAIN} if the current inlined levels are all <em>simple</em> (according to
+     * {@link com.palantir.javaformat.doc.Level#isSimpleLevel}), otherwise behave like {@link #CHECK_INNER}.
+     */
+    ACCEPT_INLINE_CHAIN_IF_SIMPLE_OTHERWISE_CHECK_INNER,
     ;
 }
diff --git a/palantir-java-format/src/main/java/com/palantir/javaformat/OpenOp.java b/palantir-java-format/src/main/java/com/palantir/javaformat/OpenOp.java
@@ -55,6 +55,54 @@ public PartialInlineability partialInlineability() {
         return PartialInlineability.ALWAYS_PARTIALLY_INLINEABLE;
     }
 
+    /**
+     * A level is "simple" if it doesn't have multiple parameters (in the case of a method call), or multiple chained
+     * method calls.
+     *
+     * <p>This is used to poison the ability to partially inline method arguments down the line if a parent level was
+     * too complicated, so that you can't end up with this:
+     *
+     * <pre>
+     * method(arg1, arg2, arg3.foo().stream()
+     *         .filter(...)
+     *         .map(...));
+     * </pre>
+     *
+     * or
+     *
+     * <pre>
+     * log.info("Message", exception, SafeArg.of(
+     *         "foo", foo);
+     * </pre>
+     *
+     * But you can still get this (see test B20128760):
+     *
+     * <pre>
+     * Stream<ItemKey> itemIdsStream = stream(members).flatMap(m -> m.getFieldValues().entrySet().stream()
+     *         .filter(...)
+     *         .map(...));
+     * </pre>
+     *
+     * or this:
+     *
+     * <pre>
+     * method(anotherMethod(arg3.foo().stream()
+     *         .filter(...)
+     *         .map(...)));
+     * </pre>
+     *
+     * or this:
+     *
+     * <pre>
+     * method(anotherMethod(
+     *         ...)); // long arguments
+     * </pre>
+     */
+    @Default
+    public boolean isSimple() {
+        return true;
+    }
+
     public abstract Optional<String> debugName();
 
     /** Custom max column limit that contents of this level <em>before the last break</em> may not exceed. */

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
@@ -19,7 +19,6 @@
 import static com.google.common.collect.Iterables.getLast;
 
 import com.google.common.base.MoreObjects;
-import com.google.common.base.Preconditions;
 import com.google.common.base.Suppliers;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Range;
@@ -32,6 +31,7 @@
 import com.palantir.javaformat.Output;
 import com.palantir.javaformat.PartialInlineability;
 import com.palantir.javaformat.doc.Obs.Exploration;
+import com.palantir.javaformat.doc.Obs.ExplorationNode;
 import com.palantir.javaformat.doc.StartsWithBreakVisitor.Result;
 import java.util.ArrayList;
 import java.util.List;
@@ -178,7 +178,7 @@ public State preferBreakingLastInnerLevel(boolean _keepIndentWhenInlined) {
                 State state1 = state.withNoIndent();
                 Optional<Obs.Exploration> lastLevelBroken = levelNode.maybeExplore(
                         "tryBreakLastLevel", state1, (explorationNode) ->
-                                tryBreakLastLevel(commentsHelper, maxWidth, state1, explorationNode));
+                                tryBreakLastLevel(commentsHelper, maxWidth, state1, explorationNode, true));
 
                 if (lastLevelBroken.isPresent()) {
                     if (lastLevelBroken.get().state().numLines() < broken.state().numLines()) {
@@ -271,7 +271,11 @@ private Optional<State> handleBreakOnlyIfInnerLevelsThenFitOnOneLine(
     }
 
     private Optional<State> tryBreakLastLevel(
-            CommentsHelper commentsHelper, int maxWidth, State state, Obs.ExplorationNode explorationNode) {
+            CommentsHelper commentsHelper,
+            int maxWidth,
+            State state,
+            ExplorationNode explorationNode,
+            boolean isSimpleInliningSoFar) {
         if (docs.isEmpty() || !(getLast(docs) instanceof Level)) {
             return Optional.empty();
         }
@@ -295,52 +299,28 @@ private Optional<State> tryBreakLastLevel(
 
         SplitsBreaks prefixSplitsBreaks = splitByBreaks(leadingDocs);
 
+        boolean isSimpleInlining = isSimpleInliningSoFar && Level.this.openOp.isSimple();
+
         State state1 = tryToLayOutLevelOnOneLine(commentsHelper, maxWidth, state, prefixSplitsBreaks, explorationNode);
         // If a break was still forced somehow even though we could fit the leadingWidth, then abort.
         // This could happen if inner levels have set a `columnLimitBeforeLastBreak` or something like that.
         if (state1.numLines() != state.numLines()) {
             return Optional.empty();
         }
 
-        // Ok now how to handle the last level?
-        // There are two options:
-        //  * 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.
         switch (lastLevel.getBreakabilityIfLastLevel()) {
+            case ACCEPT_INLINE_CHAIN_IF_SIMPLE_OTHERWISE_CHECK_INNER:
+                if (isSimpleInlining) {
+                    return tryBreakLastLevel_acceptInlineChain(
+                            commentsHelper, maxWidth, explorationNode, lastLevel, state1);
+                }
+                // Otherwise, fall through to CHECK_INNER.
             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());
+                return tryBreakLastLevel_checkInner(
+                        commentsHelper, maxWidth, explorationNode, lastLevel, isSimpleInlining, state1);
             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());
+                return tryBreakLastLevel_acceptInlineChain(
+                        commentsHelper, maxWidth, explorationNode, lastLevel, state1);
             case ABORT:
                 return Optional.empty();
             default:
@@ -349,11 +329,54 @@ private Optional<State> tryBreakLastLevel(
         }
     }
 
-    private static void assertStartsWithBreakOrEmpty(State state, Doc doc) {
-        Preconditions.checkState(
-                StartsWithBreakVisitor.INSTANCE.visit(doc) != Result.NO,
-                "Doc should have started with a break but didn't:\n%s",
-                new LevelDelimitedFlatValueDocVisitor(state).visit(doc));
+    private static Optional<State> tryBreakLastLevel_acceptInlineChain(
+            CommentsHelper commentsHelper,
+            int maxWidth,
+            ExplorationNode explorationNode,
+            Level lastLevel,
+            State state) {
+        // Ok then, we are allowed to break here, but first verify that we have enough room to inline this last
+        // level's prefix.
+        float extraWidth = new CountWidthUntilBreakVisitor(maxWidth - state.indent()).visit(lastLevel);
+        boolean stillFits = !Float.isInfinite(extraWidth) && state.column() + extraWidth <= maxWidth;
+        if (!stillFits) {
+            return Optional.empty();
+        }
+
+        // Note: computeBreaks, not computeBroken, so it can try to do this logic recursively for the
+        // lastLevel
+        return Optional.of(
+                explorationNode
+                        .newChildNode(lastLevel, state)
+                        .explore("end tryBreakLastLevel chain", state, exp ->
+                                lastLevel.computeBreaks(commentsHelper, maxWidth, state, exp))
+                        .markAccepted());
+    }
+
+    private static Optional<State> tryBreakLastLevel_checkInner(
+            CommentsHelper commentsHelper,
+            int maxWidth,
+            ExplorationNode explorationNode,
+            Level lastLevel,
+            boolean isSimpleInlining,
+            State state) {
+        // Try to fit the entire inner prefix if it's that kind of level.
+        return BreakBehaviours.caseOf(lastLevel.getBreakBehaviour())
+                .preferBreakingLastInnerLevel(keepIndentWhenInlined -> {
+                    State state2 = state;
+                    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, isSimpleInlining))
+                            .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());
     }
 
     /**

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
@@ -2711,8 +2711,9 @@ private void visitRegularDot(List<ExpressionTree> items, boolean needDot) {
                     .debugName("visitRegularDot")
                     .plusIndent(plusFour)
                     .breakBehaviour(BreakBehaviours.preferBreakingLastInnerLevel(false))
-                    .breakabilityIfLastLevel(LastLevelBreakability.CHECK_INNER)
+                    .breakabilityIfLastLevel(LastLevelBreakability.ACCEPT_INLINE_CHAIN_IF_SIMPLE_OTHERWISE_CHECK_INNER)
                     .columnLimitBeforeLastBreak(METHOD_CHAIN_COLUMN_LIMIT)
+                    .isSimple(!trailingDereferences)
                     .build());
         }
         // don't break after the first element if it is every small, unless the
@@ -2794,9 +2795,10 @@ private void visitDotWithPrefix(
                 .debugName("visitDotWithPrefix")
                 .plusIndent(plusFour)
                 .breakBehaviour(BreakBehaviours.preferBreakingLastInnerLevel(false))
-                .breakabilityIfLastLevel(LastLevelBreakability.CHECK_INNER)
+                .breakabilityIfLastLevel(LastLevelBreakability.ACCEPT_INLINE_CHAIN_IF_SIMPLE_OTHERWISE_CHECK_INNER)
                 .partialInlineability(PartialInlineability.IF_FIRST_LEVEL_FITS)
                 .columnLimitBeforeLastBreak(METHOD_CHAIN_COLUMN_LIMIT)
+                .isSimple(!trailingDereferences)
                 .build());
 
         for (int times = 0; times < prefixes.size(); times++) {
@@ -3003,6 +3005,7 @@ void addArguments(List<? extends ExpressionTree> arguments, Indent plusIndent) {
                 .plusIndent(plusIndent)
                 .breakBehaviour(BreakBehaviours.preferBreakingLastInnerLevel(false))
                 .breakabilityIfLastLevel(LastLevelBreakability.CHECK_INNER)
+                .isSimple(arguments.size() <= 1)
                 .build());
         token("(");
         if (!arguments.isEmpty()) {
@@ -3051,6 +3054,7 @@ private void argList(List<? extends ExpressionTree> arguments) {
                 .plusIndent(ZERO)
                 .breakBehaviour(BreakBehaviours.preferBreakingLastInnerLevel(true))
                 .breakabilityIfLastLevel(LastLevelBreakability.CHECK_INNER)
+                .isSimple(arguments.size() <= 1)
                 .build());
         boolean first = true;
         FillMode fillMode = hasOnlyShortItems(arguments) ? FillMode.INDEPENDENT : FillMode.UNIFIED;

diff --git a/palantir-java-format/src/test/java/com/palantir/javaformat/java/PartialFormattingTest.java b/palantir-java-format/src/test/java/com/palantir/javaformat/java/PartialFormattingTest.java
@@ -542,14 +542,13 @@ public void blankAndComment() throws Exception {
                 "    definitionService.insert(createIncrementalDefinition(4));",
                 "",
                 "    // No maxResults",
-                "    assertThat(",
-                "            achievementFirstPartyHelper",
-                "                .listDefinitionsByApplication(",
-                "                    STUB_GAIA_ID,",
-                "                    STUB_APPLICATION_ID,",
-                "                    Optional.<Integer>absent(),",
-                "                    Optional.<String>absent())",
-                "                .getAchievements())",
+                "    assertThat(achievementFirstPartyHelper",
+                "            .listDefinitionsByApplication(",
+                "                STUB_GAIA_ID,",
+                "                STUB_APPLICATION_ID,",
+                "                Optional.<Integer>absent(),",
+                "                Optional.<String>absent())",
+                "            .getAchievements())",
                 "        .containsExactly(",
                 "            createExpectedDefinition(1),",
                 "            createIncrementalExpectedDefinition(2),",

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
@@ -10,12 +10,11 @@ class B20128760 {
                         XxxxXxxxxxxxxx.xxxXxxxxxxxxXxxxxXxxxxXxXxxxxx(Xxxxxxxxxx.xxxXxxxXxxxx(x)))
                 .xxxxx();
 
-        xxxxxxXxxx(
-                        xxx.xxxXxXxxxxxxx(new XxxxxxxxxXxxxxxxx.Xxxxxxx()
-                                        .xxxxXxxxXxx(Xxxxx.xxxXxxxxXxxx(new XxxxXx(
-                                                xxxxxxxxx1.xxxXxxxXx().xxxXxxxx() + xxxxxxxxx2.xxxXxxxXx().xxxXxxxx())))
-                                        .xxxxx())
-                                .xxxXxxxxxx())
+        xxxxxxXxxx(xxx.xxxXxXxxxxxxx(new XxxxxxxxxXxxxxxxx.Xxxxxxx()
+                                .xxxxXxxxXxx(Xxxxx.xxxXxxxxXxxx(new XxxxXx(
+                                        xxxxxxxxx1.xxxXxxxXx().xxxXxxxx() + xxxxxxxxx2.xxxXxxxXx().xxxXxxxx())))
+                                .xxxxx())
+                        .xxxXxxxxxx())
                 .xxxxxxxxXxxxxxx();
 
         XXxxxx<Xxxx, XxxxxxxXxxxxxxxxxxXxxxxxxxx> xxxxxxxxxXxxXxxXxXxxxxxxxXx = XX.xxxxXxxXxxx2(
@@ -27,18 +26,16 @@ class B20128760 {
                         xxxxxxxxXxxxxxxxxXxxXx,
                         XX.xxxxxXx(XX.xxxxx(), XX.xxxxxx(XxxxxxxXxxxxxxxxxxXxxxxxxxx.xxxxx)));
 
-        xxXxxxxxxxXxxxXxxx = (XxxxXxxx)
-                xxxxxxxxXxxxxxxxxx
-                        .xxx()
-                        .xxxxxXxxxxxx
-                        .xxxXxxxxxxx(xxxxxxxxXxxxxxxxxx.xxx().xxxxXxxxx.xxxXxxxxxxXxxx() - 1)
-                        .xxxXxxx()
-                        .xxxxXxxxXxXx(X.xx.xxxxxxxxxxxxxxxxxxxxxxxxxxx);
+        xxXxxxxxxxXxxxXxxx = (XxxxXxxx) xxxxxxxxXxxxxxxxxx
+                .xxx()
+                .xxxxxXxxxxxx
+                .xxxXxxxxxxx(xxxxxxxxXxxxxxxxxx.xxx().xxxxXxxxx.xxxXxxxxxxXxxx() - 1)
+                .xxxXxxx()
+                .xxxxXxxxXxXx(X.xx.xxxxxxxxxxxxxxxxxxxxxxxxxxx);
 
-        xxxxxxXxxx(
-                        xxxxxxxxxx
-                                .xxxXxxxXxxxxXxxxxxxxxxxXxxxXxxxXxxxxXxxx(XXXxXXXXxXX, XXXXXXxXXXxXXXXxXX, XXXXXxXX)
-                                .xxxxxxxxxxxXxxxXxxxXxxxxx())
+        xxxxxxXxxx(xxxxxxxxxx
+                        .xxxXxxxXxxxxXxxxxxxxxxxXxxxXxxxXxxxxXxxx(XXXxXXXXxXX, XXXXXXxXXXxXXXXxXX, XXXXXxXX)
+                        .xxxxxxxxxxxXxxxXxxxXxxxxx())
                 .xxxxxxxxXxxxxxx(
                         xxxxXxxxxxxxxxxXxxxXxxxXxxxx(xxxx1Xxxx, Xxxxx.XXxXXX),
                         xxxxXxxxxxxxxxxXxxxXxxxXxxxx(xxxx2Xxxx, Xxxxx.XXxXXX));
@@ -72,10 +69,9 @@ class B20128760 {
                     Xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx {}
 
     {
-        Stream<ItemKey> itemIdsStream = stream(members).flatMap(m ->
-                m.getFieldValues().entrySet().stream()
-                        .filter(fv -> itemLinkFieldIds.contains(fv.getKey()))
-                        .flatMap(fv -> FieldDTO.deserializeStringToListOfStrings(fv.getValue()).stream().map(id ->
-                                new ItemKey(fieldsById.get(fv.getKey()).getItemTypeId(), id))));
+        Stream<ItemKey> itemIdsStream = stream(members).flatMap(m -> m.getFieldValues().entrySet().stream()
+                .filter(fv -> itemLinkFieldIds.contains(fv.getKey()))
+                .flatMap(fv -> FieldDTO.deserializeStringToListOfStrings(fv.getValue()).stream()
+                        .map(id -> new ItemKey(fieldsById.get(fv.getKey()).getItemTypeId(), id))));
     }
 }
diff --git a/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B21954779.output b/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B21954779.output
@@ -4,17 +4,15 @@ class B21954779 {
                         VM_METADATA, Optional.<InstanceProto>absent(), AssignReservedIp.YES, AttachDataDisk.YES))
                 .thenReturn(OPERATION);
 
-        fail(
-                String.format(
-                        "       Experiment name %s is used multiple times in %s",
-                        expName, envName, zzzzzzzzzzzz, zzzzzzzzzzzz));
+        fail(String.format(
+                "       Experiment name %s is used multiple times in %s",
+                expName, envName, zzzzzzzzzzzz, zzzzzzzzzzzz));
 
         new Handler(Looper.getMainLooper()).post(new Runnable() {
             @Override
             public void run() {
-                utils.showShortToast(
-                        application.getString(
-                                R.string.working_on_project_format, Utils.getProjectDisplayName(selectedProject)));
+                utils.showShortToast(application.getString(
+                        R.string.working_on_project_format, Utils.getProjectDisplayName(selectedProject)));
             }
         });
 

diff --git a/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B23708487.output b/...tir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/B23708487.output
@@ -2,9 +2,8 @@ class B23708487 {
     static {
         TASK_AS_OWNER = com.google.security.acl.proto2api.ACL.Entry.newBuilder()
                 .setRole(com.google.security.acl.proto2api.ACL.Entry.ROLE.OWNER.getNumber())
-                .setScope(
-                        com.google.security.acl.proto2api.ACL.Scope.newBuilder()
-                                .setMdbUser(BorgTaskInfo.getMdbUser().getMdbUserName()))
+                .setScope(com.google.security.acl.proto2api.ACL.Scope.newBuilder()
+                        .setMdbUser(BorgTaskInfo.getMdbUser().getMdbUserName()))
                 .build();
     }
 }