Skip to content

Commit b7d2c98

Browse files
crogozcrogoz
committed
use maxDepth to explore both breaking & not breaking
. spotless .
1 parent 9d6b8f4 commit b7d2c98

4 files changed

Lines changed: 237 additions & 15 deletions

File tree

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

Lines changed: 68 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -52,10 +52,9 @@
5252
/** A {@code Level} inside a {@link Doc}. */
5353
public final class Level extends Doc {
5454
/**
55-
* How many branches we are allowed to take (i.e how many times we can consider breaking vs not breaking the current
56-
* level) before we stop branching and always break, which is the google-java-format default behaviour.
55+
* The depth of nested levels in the current level tree from which we explore both breaking vs not breaking the current level.
5756
*/
58-
private static final int MAX_BRANCHING_COEFFICIENT = 20;
57+
private static final int LAST_LEVELS_TO_EXPLORE = 7;
5958

6059
private static final Collector<Level, ?, Optional<Level>> GET_LAST_COLLECTOR = Collectors.reducing((u, v) -> v);
6160

@@ -64,6 +63,10 @@ public final class Level extends Doc {
6463
@SuppressWarnings("Immutable") // Effectively immutable
6564
private final ImmutableSupplier<SplitsBreaks> memoizedSplitsBreaks =
6665
Suppliers.memoize(() -> splitByBreaks(docs))::get;
66+
67+
@SuppressWarnings("Immutable") // Effectively immutable
68+
private final ImmutableSupplier<Integer> memoizedMaxDepth = Suppliers.memoize(() -> computeMaxDepth(docs))::get;
69+
6770
/** The immutable characteristics of this level determined before the level contents are available. */
6871
private final OpenOp openOp;
6972

@@ -130,7 +133,7 @@ public State computeBreaks(CommentsHelper commentsHelper, int maxWidth, State st
130133
* into account the level's {@link #getColumnLimitBeforeLastBreak()}.
131134
*
132135
* @return the width after fitting it onto one line, if it was possible. This is guaranteed to be less than
133-
* {@code maxWidth}
136+
* {@code maxWidth}
134137
*/
135138
private Optional<Integer> tryToFitOnOneLine(int maxWidth, State state, Iterable<Doc> docs) {
136139
int column = state.column();
@@ -189,16 +192,15 @@ public State breakThisLevel() {
189192

190193
@Override
191194
public State preferBreakingLastInnerLevel(boolean _keepIndentWhenInlined) {
192-
// Try both breaking and not breaking. Choose the better one based on LOC, preferring
193-
// breaks if the outcome is the same.
194-
State state = this.state.withNewBranch();
195-
196-
Obs.Exploration broken = breakNormally(state);
197-
198-
if (state.branchingCoefficient() < MAX_BRANCHING_COEFFICIENT) {
195+
int maxDepth = getMaxDepth();
196+
// Explore both breaking and not breaking if we're in the last LAST_LEVELS_TO_EXPLORE levels. Choose the
197+
// better one based on LOC, preferring breaks if the outcome is the same.
198+
if (maxDepth <= LAST_LEVELS_TO_EXPLORE) {
199+
State state = this.state.withNewBranch();
200+
Obs.Exploration broken = breakNormally(state);
199201
State state1 = state.withNoIndent();
200202
Optional<Obs.Exploration> lastLevelBroken = levelNode.maybeExplore(
201-
"tryBreakLastLevel",
203+
"tryBreakLastLevel" + maxDepth,
202204
state1,
203205
explorationNode -> tryBreakLastLevel(commentsHelper, maxWidth, state1, explorationNode, true));
204206

@@ -208,8 +210,20 @@ public State preferBreakingLastInnerLevel(boolean _keepIndentWhenInlined) {
208210
return lastLevelBroken.get().markAccepted();
209211
}
210212
}
213+
return broken.markAccepted();
214+
} else {
215+
State state1 = state.withNoIndent();
216+
Optional<Obs.Exploration> lastLevelBroken = levelNode.maybeExplore(
217+
"tryBreakLastLevel only" + maxDepth,
218+
state1,
219+
explorationNode -> tryBreakLastLevel(commentsHelper, maxWidth, state1, explorationNode, true));
220+
221+
if (lastLevelBroken.isPresent()) {
222+
return lastLevelBroken.get().markAccepted();
223+
}
224+
// falling back to breaking normally
211225
}
212-
return broken.markAccepted();
226+
return breakNormally(state).markAccepted();
213227
}
214228

215229
@Override
@@ -242,7 +256,7 @@ public State inlineSuffix() {
242256
private Exploration breakNormally(State state, LevelNode levelNode, CommentsHelper commentsHelper, int maxWidth) {
243257
State stateForBroken = state.withIndentIncrementedBy(getPlusIndent());
244258
return levelNode.explore(
245-
"breaking normally",
259+
"breaking normally " + getMaxDepth(),
246260
stateForBroken,
247261
explorationNode -> computeBroken(commentsHelper, maxWidth, stateForBroken, explorationNode));
248262
}
@@ -258,7 +272,7 @@ private Exploration breakNormally(State state, LevelNode levelNode, CommentsHelp
258272
* </ul>
259273
*
260274
* @param brokenState is expected to be the state resulting from {@link Level#breakNormally}, but is passed in
261-
* for efficiency reasons
275+
* for efficiency reasons
262276
*/
263277
private Optional<State> handle_breakOnlyIfInnerLevelsThenFitOnOneLine(
264278
CommentsHelper commentsHelper,
@@ -736,6 +750,44 @@ public String representation(State state) {
736750
return new LevelDelimitedFlatValueDocVisitor(state).visit(this);
737751
}
738752

753+
/**
754+
* Get the approximation of the maximum depth of nested levels in this level tree (memoized).
755+
*/
756+
private int getMaxDepth() {
757+
return memoizedMaxDepth.get();
758+
}
759+
760+
/**
761+
* Calculate an approximation of the maximum depth of nested levels in this level tree, counting method call nesting.
762+
* This counts levels that represent method invocations (including lambdas and other callable constructs),
763+
* which provides a better measure of structural complexity than counting all levels.
764+
* This computation is expensive, so it's memoized via {@link #getMaxDepth()}.
765+
*/
766+
private int computeMaxDepth(Iterable<Doc> docs) {
767+
int maxChildDepth = 0;
768+
for (Doc doc : docs) {
769+
if (doc instanceof Level) {
770+
Level childLevel = (Level) doc;
771+
maxChildDepth = Math.max(maxChildDepth, childLevel.getMaxDepth());
772+
}
773+
}
774+
775+
// Count this level if it represents a method call or similar construct
776+
// A level is considered a "method call level" if it has breaks (can span multiple lines)
777+
// and is not just a simple wrapper. We use the presence of breaks as a proxy for
778+
// meaningful structural nesting.
779+
boolean hasBreaks = false;
780+
for (Doc doc : docs) {
781+
if (doc instanceof Break) {
782+
hasBreaks = true;
783+
break;
784+
}
785+
}
786+
787+
// Only count levels that have breaks (i.e., can actually be formatted across multiple lines)
788+
return hasBreaks ? 1 + maxChildDepth : maxChildDepth;
789+
}
790+
739791
/**
740792
* Get the width of a sequence of {@link Doc}s.
741793
*
@@ -774,6 +826,7 @@ public String toString() {
774826
interface SplitsBreaks {
775827
/** Groups of {@link Doc}s that are children of the current {@link Level}, separated by {@link Break}s. */
776828
ImmutableList<ImmutableList<Doc>> splits();
829+
777830
/** {@link Break}s between {@link Doc}s in the current {@link Level}. */
778831
ImmutableList<Break> breaks();
779832
}

‎palantir-java-format/src/main/java/com/palantir/javaformat/doc/State.java‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,12 +39,14 @@
3939
public abstract class State {
4040
/** Last indent that was actually taken. */
4141
public abstract int lastIndent();
42+
4243
/** Next indent, if the level is about to be broken. */
4344
public abstract int indent();
4445

4546
public abstract int column();
4647

4748
public abstract boolean mustBreak();
49+
4850
/** Counts how many lines a particular formatting took. */
4951
public abstract int numLines();
5052

‎palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-deeply-nested-calls.input‎

Lines changed: 89 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,4 +12,93 @@ class PalantirDeeplyNestedCalls {
1212
.build()))
1313
.build()))
1414
.build();
15+
16+
17+
void testMethod() {
18+
assertThat(ConfigServiceV1.getAllItems(item1, item2, item3, item4))
19+
.containsExactlyInAnyOrderElementsOf(Set.of(
20+
new ItemWithCustomLocator(
21+
ItemConfigV1.DEFAULT_CONFIG.toCustomString(),
22+
ItemConfigV1.createCustomConfig(
23+
ItemConfigV1Locator.of(TEST_ITEM_A))),
24+
new ItemWithCustomLocator(
25+
ItemConfigV1.builder()
26+
.id(getId(TEST_ITEM_A))
27+
.name(ItemName.of(TEST_API_NAME.get()))
28+
.apiName(TEST_API_NAME)
29+
.spec(ItemConfigV1Spec.native_(
30+
NativeItemConfigV1Spec.model(TestModel.of())))
31+
.category(TestCatalog.TEST_CATEGORY_ALPHA)
32+
.creator(TestCatalog.TEST_CREATOR_BETA)
33+
.status(ItemLifecycleStatus.EXPERIMENTAL)
34+
.statusV2(ItemLifecycleStatusV2.experimental(
35+
LifecycleStatusExperimental.of()))
36+
.inputSpec(ItemInputSpec.unknownInputType(UnknownInputSpec.of()))
37+
.creationDate(
38+
ItemMetadataInfo.builder().build())
39+
.performanceAttributes(
40+
ItemAttributes.builder().build())
41+
.categories(TestCatalog.TEST_CATEGORY_ALPHA)
42+
.build(),
43+
CustomItemLocator.hosted(
44+
HostedItemLocator.of(TEST_ITEM_B))),
45+
new ItemWithCustomLocator(
46+
ItemConfigV1.builder()
47+
.id(getId(TestModelCatalog.MODEL_XYZ_123.apiName()))
48+
.name(ItemName.of(TestModelCatalog.MODEL_XYZ_123
49+
.apiName()
50+
.get()))
51+
.apiName(TestModelCatalog.MODEL_XYZ_123.apiName())
52+
.spec(ItemConfigV1Spec.native_(NativeItemConfigV1Spec.hubModel(
53+
HubModelSpec.of())))
54+
.category(TestCatalog.TEST_CATEGORY_ALPHA)
55+
.creator(TestCatalog.TEST_CREATOR_BETA)
56+
.status(ItemLifecycleStatus.EXPERIMENTAL)
57+
.statusV2(ItemLifecycleStatusV2.experimental(
58+
LifecycleStatusExperimental.of()))
59+
.inputSpec(ItemInputSpec.unknownInputType(UnknownInputSpec.of()))
60+
.creationDate(
61+
ItemMetadataInfo.builder().build())
62+
.performanceAttributes(
63+
ItemAttributes.builder().build())
64+
.categories(TestCatalog.TEST_CATEGORY_ALPHA)
65+
.build(),
66+
CustomItemLocator.hosted(
67+
HostedItemLocator.of(TEST_ITEM_C))),
68+
// Discovered items
69+
new ItemWithCustomLocator(
70+
TestModelCatalog.MODEL_ABC_456.toItemApi(),
71+
CustomItemLocator.discovered(DiscoveredItemLocator.of())),
72+
new ItemWithCustomLocator(
73+
ItemConfigV1.builder()
74+
.id(getId(TEST_API_NAME))
75+
.name(ItemName.of(TEST_API_NAME.get()))
76+
.apiName(TEST_API_NAME)
77+
.spec(ItemConfigV1Spec.native_(NativeItemConfigV1Spec.hubModel(
78+
HubModelSpec.of())))
79+
.category(TestCatalog.TEST_CATEGORY_ALPHA)
80+
.creator(TestCatalog.TEST_CREATOR_BETA)
81+
.status(ItemLifecycleStatus.EXPERIMENTAL)
82+
.statusV2(ItemLifecycleStatusV2.experimental(
83+
LifecycleStatusExperimental.of()))
84+
.inputSpec(ItemInputSpec.unknownInputType(UnknownInputSpec.of()))
85+
.creationDate(
86+
ItemMetadataInfo.builder().build())
87+
.performanceAttributes(
88+
ItemAttributes.builder().build())
89+
.categories(TestCatalog.TEST_CATEGORY_ALPHA)
90+
.build(),
91+
CustomItemLocator.discovered(DiscoveredItemLocator.of()))));
92+
}
93+
94+
public Map<FooBarRid, BazQuxMetrics> otherMethod(
95+
FooAuthHeader authHeader, Set<FooBarRid> thingRids, WidgetTimeRange timeRange) {
96+
return myCustomService
97+
.get()
98+
.fetchData(authHeader, FooMyCustomClass.of(thingRids, timeRange))
99+
.entrySet()
100+
.stream()
101+
.collect(Collectors.toMap(
102+
Map.Entry::getKey, entry -> entry.getValue().getMetrics()));
103+
}
15104
}

‎palantir-java-format/src/test/resources/com/palantir/javaformat/java/testdata/palantir-deeply-nested-calls.output‎

Lines changed: 78 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,4 +12,82 @@ class PalantirDeeplyNestedCalls {
1212
.build()))
1313
.build()))
1414
.build();
15+
16+
void testMethod() {
17+
assertThat(ConfigServiceV1.getAllItems(item1, item2, item3, item4))
18+
.containsExactlyInAnyOrderElementsOf(Set.of(
19+
new ItemWithCustomLocator(
20+
ItemConfigV1.DEFAULT_CONFIG.toCustomString(),
21+
ItemConfigV1.createCustomConfig(ItemConfigV1Locator.of(TEST_ITEM_A))),
22+
new ItemWithCustomLocator(
23+
ItemConfigV1.builder()
24+
.id(getId(TEST_ITEM_A))
25+
.name(ItemName.of(TEST_API_NAME.get()))
26+
.apiName(TEST_API_NAME)
27+
.spec(ItemConfigV1Spec.native_(NativeItemConfigV1Spec.model(TestModel.of())))
28+
.category(TestCatalog.TEST_CATEGORY_ALPHA)
29+
.creator(TestCatalog.TEST_CREATOR_BETA)
30+
.status(ItemLifecycleStatus.EXPERIMENTAL)
31+
.statusV2(ItemLifecycleStatusV2.experimental(LifecycleStatusExperimental.of()))
32+
.inputSpec(ItemInputSpec.unknownInputType(UnknownInputSpec.of()))
33+
.creationDate(ItemMetadataInfo.builder().build())
34+
.performanceAttributes(
35+
ItemAttributes.builder().build())
36+
.categories(TestCatalog.TEST_CATEGORY_ALPHA)
37+
.build(),
38+
CustomItemLocator.hosted(HostedItemLocator.of(TEST_ITEM_B))),
39+
new ItemWithCustomLocator(
40+
ItemConfigV1.builder()
41+
.id(getId(TestModelCatalog.MODEL_XYZ_123.apiName()))
42+
.name(ItemName.of(TestModelCatalog.MODEL_XYZ_123
43+
.apiName()
44+
.get()))
45+
.apiName(TestModelCatalog.MODEL_XYZ_123.apiName())
46+
.spec(ItemConfigV1Spec.native_(
47+
NativeItemConfigV1Spec.hubModel(HubModelSpec.of())))
48+
.category(TestCatalog.TEST_CATEGORY_ALPHA)
49+
.creator(TestCatalog.TEST_CREATOR_BETA)
50+
.status(ItemLifecycleStatus.EXPERIMENTAL)
51+
.statusV2(ItemLifecycleStatusV2.experimental(LifecycleStatusExperimental.of()))
52+
.inputSpec(ItemInputSpec.unknownInputType(UnknownInputSpec.of()))
53+
.creationDate(ItemMetadataInfo.builder().build())
54+
.performanceAttributes(
55+
ItemAttributes.builder().build())
56+
.categories(TestCatalog.TEST_CATEGORY_ALPHA)
57+
.build(),
58+
CustomItemLocator.hosted(HostedItemLocator.of(TEST_ITEM_C))),
59+
// Discovered items
60+
new ItemWithCustomLocator(
61+
TestModelCatalog.MODEL_ABC_456.toItemApi(),
62+
CustomItemLocator.discovered(DiscoveredItemLocator.of())),
63+
new ItemWithCustomLocator(
64+
ItemConfigV1.builder()
65+
.id(getId(TEST_API_NAME))
66+
.name(ItemName.of(TEST_API_NAME.get()))
67+
.apiName(TEST_API_NAME)
68+
.spec(ItemConfigV1Spec.native_(
69+
NativeItemConfigV1Spec.hubModel(HubModelSpec.of())))
70+
.category(TestCatalog.TEST_CATEGORY_ALPHA)
71+
.creator(TestCatalog.TEST_CREATOR_BETA)
72+
.status(ItemLifecycleStatus.EXPERIMENTAL)
73+
.statusV2(ItemLifecycleStatusV2.experimental(LifecycleStatusExperimental.of()))
74+
.inputSpec(ItemInputSpec.unknownInputType(UnknownInputSpec.of()))
75+
.creationDate(ItemMetadataInfo.builder().build())
76+
.performanceAttributes(
77+
ItemAttributes.builder().build())
78+
.categories(TestCatalog.TEST_CATEGORY_ALPHA)
79+
.build(),
80+
CustomItemLocator.discovered(DiscoveredItemLocator.of()))));
81+
}
82+
83+
public Map<FooBarRid, BazQuxMetrics> otherMethod(
84+
FooAuthHeader authHeader, Set<FooBarRid> thingRids, WidgetTimeRange timeRange) {
85+
return myCustomService
86+
.get()
87+
.fetchData(authHeader, FooMyCustomClass.of(thingRids, timeRange))
88+
.entrySet()
89+
.stream()
90+
.collect(Collectors.toMap(
91+
Map.Entry::getKey, entry -> entry.getValue().getMetrics()));
92+
}
1593
}

0 commit comments

Comments
 (0)