Skip to content

Commit c9e5191

Browse files
munificentmunificent
authored
Better style for inline case bodies. (#1176)
In the previous PR, any case body that fit on one line was allowed to even if other cases in the same switch didn't. I tested it on a corpus and I found that led to confusing switches where it wasn't always clear where the case body starts. I think you really want it all or nothing: either every single case fits on the same line in which case you can make the whole switch compact, or every case should be on its own line, even the ones that would fit. Unfortunately, it's a little tricky to have formatter rules that span code containing hard splits, so getting that working took some doing. It also regressed performance pretty badly. But I figured out some optimizations in ChunkBuilder and it's basically back to the same performance it had before. Also, this incidentally fixes a bug where parameter metadata in trailing comma parameter lists was also supposed to have that same all-or-nothing splitting logic but didn't. I've tried this on a corpus and I'm pretty happy with the results. Right now, relatively few switches benefit because the mandatory breaks mean a lot of switches have at least two statements (which always causes the case to split). But as those breaks are removed, I think we'll see more compact switches. Even today, this code does improve some switches where every case is just a short return statement.
1 parent dd8b295 commit c9e5191

File tree

11 files changed

+399
-69
lines changed

11 files changed

+399
-69
lines changed

‎CHANGELOG.md

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
# 2.2.5-dev
22

33
* Format patterns and related features.
4-
* Allow switch statement case bodies on the same line as the case.
4+
* Allow switch statements where all case bodies are on the same line as the
5+
case when they all fit.
6+
* Fix bug where parameter metadata wouldn't always split when it should.
57
* Don't split after `<` in collection literals.
68
* Format record expressions and record type annotations.
79
* Better indentation of multiline function types inside type argument lists.

‎benchmark/after.dart.txt

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,114 @@ class BacktrackingSolver {
171171
: type = type,
172172
sources = sources,
173173
cache = new PubspecCache(type, sources) {
174+
// A fairly large switch statement.
175+
switch (region) {
176+
case Region.everywhere:
177+
return 0.45;
178+
case Region.n:
179+
return lerpDouble(pos.y, 0, height, min, max);
180+
case Region.ne:
181+
var distance = math.max(width - pos.x - 1, pos.y);
182+
var range = math.min(width, height);
183+
return lerpDouble(distance, 0, range, min, max);
184+
case Region.e:
185+
return lerpDouble(pos.x, 0, width, min, max);
186+
case Region.se:
187+
var distance = math.max(width - pos.x - 1, height - pos.y - 1);
188+
var range = math.min(width, height);
189+
return lerpDouble(distance, 0, range, min, max);
190+
case Region.s:
191+
return lerpDouble(pos.y, 0, height, max, min);
192+
case Region.sw:
193+
var distance = math.max(pos.x, height - pos.y - 1);
194+
var range = math.min(width, height);
195+
return lerpDouble(distance, 0, range, min, max);
196+
case Region.w:
197+
return lerpDouble(pos.x, 0, width, max, min);
198+
case Region.nw:
199+
var distance = math.max(pos.x, pos.y);
200+
var range = math.min(width, height);
201+
return lerpDouble(distance, 0, range, min, max);
202+
case Region.everywhere:
203+
return 0.45;
204+
case Region.n:
205+
return lerpDouble(pos.y, 0, height, min, max);
206+
case Region.ne:
207+
var distance = math.max(width - pos.x - 1, pos.y);
208+
var range = math.min(width, height);
209+
return lerpDouble(distance, 0, range, min, max);
210+
case Region.e:
211+
return lerpDouble(pos.x, 0, width, min, max);
212+
case Region.se:
213+
var distance = math.max(width - pos.x - 1, height - pos.y - 1);
214+
var range = math.min(width, height);
215+
return lerpDouble(distance, 0, range, min, max);
216+
case Region.s:
217+
return lerpDouble(pos.y, 0, height, max, min);
218+
case Region.sw:
219+
var distance = math.max(pos.x, height - pos.y - 1);
220+
var range = math.min(width, height);
221+
return lerpDouble(distance, 0, range, min, max);
222+
case Region.w:
223+
return lerpDouble(pos.x, 0, width, max, min);
224+
case Region.nw:
225+
var distance = math.max(pos.x, pos.y);
226+
var range = math.min(width, height);
227+
return lerpDouble(distance, 0, range, min, max);
228+
case Region.everywhere:
229+
return 0.45;
230+
case Region.n:
231+
return lerpDouble(pos.y, 0, height, min, max);
232+
case Region.ne:
233+
var distance = math.max(width - pos.x - 1, pos.y);
234+
var range = math.min(width, height);
235+
return lerpDouble(distance, 0, range, min, max);
236+
case Region.e:
237+
return lerpDouble(pos.x, 0, width, min, max);
238+
case Region.se:
239+
var distance = math.max(width - pos.x - 1, height - pos.y - 1);
240+
var range = math.min(width, height);
241+
return lerpDouble(distance, 0, range, min, max);
242+
case Region.s:
243+
return lerpDouble(pos.y, 0, height, max, min);
244+
case Region.sw:
245+
var distance = math.max(pos.x, height - pos.y - 1);
246+
var range = math.min(width, height);
247+
return lerpDouble(distance, 0, range, min, max);
248+
case Region.w:
249+
return lerpDouble(pos.x, 0, width, max, min);
250+
case Region.nw:
251+
var distance = math.max(pos.x, pos.y);
252+
var range = math.min(width, height);
253+
return lerpDouble(distance, 0, range, min, max);
254+
case Region.everywhere:
255+
return 0.45;
256+
case Region.n:
257+
return lerpDouble(pos.y, 0, height, min, max);
258+
case Region.ne:
259+
var distance = math.max(width - pos.x - 1, pos.y);
260+
var range = math.min(width, height);
261+
return lerpDouble(distance, 0, range, min, max);
262+
case Region.e:
263+
return lerpDouble(pos.x, 0, width, min, max);
264+
case Region.se:
265+
var distance = math.max(width - pos.x - 1, height - pos.y - 1);
266+
var range = math.min(width, height);
267+
return lerpDouble(distance, 0, range, min, max);
268+
case Region.s:
269+
return lerpDouble(pos.y, 0, height, max, min);
270+
case Region.sw:
271+
var distance = math.max(pos.x, height - pos.y - 1);
272+
var range = math.min(width, height);
273+
return lerpDouble(distance, 0, range, min, max);
274+
case Region.w:
275+
return lerpDouble(pos.x, 0, width, max, min);
276+
case Region.nw:
277+
var distance = math.max(pos.x, pos.y);
278+
var range = math.min(width, height);
279+
return lerpDouble(distance, 0, range, min, max);
280+
}
281+
174282
for (var package in useLatest) {
175283
_forceLatest.add(package);
176284
}

‎benchmark/before.dart.txt

Lines changed: 108 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,114 @@ class BacktrackingSolver {
171171
: type = type,
172172
sources = sources,
173173
cache = new PubspecCache(type, sources) {
174+
// A fairly large switch statement.
175+
switch (region) {
176+
case Region.everywhere:
177+
return 0.45;
178+
case Region.n:
179+
return lerpDouble(pos.y, 0, height, min, max);
180+
case Region.ne:
181+
var distance = math.max(width - pos.x - 1, pos.y);
182+
var range = math.min(width, height);
183+
return lerpDouble(distance, 0, range, min, max);
184+
case Region.e:
185+
return lerpDouble(pos.x, 0, width, min, max);
186+
case Region.se:
187+
var distance = math.max(width - pos.x - 1, height - pos.y - 1);
188+
var range = math.min(width, height);
189+
return lerpDouble(distance, 0, range, min, max);
190+
case Region.s:
191+
return lerpDouble(pos.y, 0, height, max, min);
192+
case Region.sw:
193+
var distance = math.max(pos.x, height - pos.y - 1);
194+
var range = math.min(width, height);
195+
return lerpDouble(distance, 0, range, min, max);
196+
case Region.w:
197+
return lerpDouble(pos.x, 0, width, max, min);
198+
case Region.nw:
199+
var distance = math.max(pos.x, pos.y);
200+
var range = math.min(width, height);
201+
return lerpDouble(distance, 0, range, min, max);
202+
case Region.everywhere:
203+
return 0.45;
204+
case Region.n:
205+
return lerpDouble(pos.y, 0, height, min, max);
206+
case Region.ne:
207+
var distance = math.max(width - pos.x - 1, pos.y);
208+
var range = math.min(width, height);
209+
return lerpDouble(distance, 0, range, min, max);
210+
case Region.e:
211+
return lerpDouble(pos.x, 0, width, min, max);
212+
case Region.se:
213+
var distance = math.max(width - pos.x - 1, height - pos.y - 1);
214+
var range = math.min(width, height);
215+
return lerpDouble(distance, 0, range, min, max);
216+
case Region.s:
217+
return lerpDouble(pos.y, 0, height, max, min);
218+
case Region.sw:
219+
var distance = math.max(pos.x, height - pos.y - 1);
220+
var range = math.min(width, height);
221+
return lerpDouble(distance, 0, range, min, max);
222+
case Region.w:
223+
return lerpDouble(pos.x, 0, width, max, min);
224+
case Region.nw:
225+
var distance = math.max(pos.x, pos.y);
226+
var range = math.min(width, height);
227+
return lerpDouble(distance, 0, range, min, max);
228+
case Region.everywhere:
229+
return 0.45;
230+
case Region.n:
231+
return lerpDouble(pos.y, 0, height, min, max);
232+
case Region.ne:
233+
var distance = math.max(width - pos.x - 1, pos.y);
234+
var range = math.min(width, height);
235+
return lerpDouble(distance, 0, range, min, max);
236+
case Region.e:
237+
return lerpDouble(pos.x, 0, width, min, max);
238+
case Region.se:
239+
var distance = math.max(width - pos.x - 1, height - pos.y - 1);
240+
var range = math.min(width, height);
241+
return lerpDouble(distance, 0, range, min, max);
242+
case Region.s:
243+
return lerpDouble(pos.y, 0, height, max, min);
244+
case Region.sw:
245+
var distance = math.max(pos.x, height - pos.y - 1);
246+
var range = math.min(width, height);
247+
return lerpDouble(distance, 0, range, min, max);
248+
case Region.w:
249+
return lerpDouble(pos.x, 0, width, max, min);
250+
case Region.nw:
251+
var distance = math.max(pos.x, pos.y);
252+
var range = math.min(width, height);
253+
return lerpDouble(distance, 0, range, min, max);
254+
case Region.everywhere:
255+
return 0.45;
256+
case Region.n:
257+
return lerpDouble(pos.y, 0, height, min, max);
258+
case Region.ne:
259+
var distance = math.max(width - pos.x - 1, pos.y);
260+
var range = math.min(width, height);
261+
return lerpDouble(distance, 0, range, min, max);
262+
case Region.e:
263+
return lerpDouble(pos.x, 0, width, min, max);
264+
case Region.se:
265+
var distance = math.max(width - pos.x - 1, height - pos.y - 1);
266+
var range = math.min(width, height);
267+
return lerpDouble(distance, 0, range, min, max);
268+
case Region.s:
269+
return lerpDouble(pos.y, 0, height, max, min);
270+
case Region.sw:
271+
var distance = math.max(pos.x, height - pos.y - 1);
272+
var range = math.min(width, height);
273+
return lerpDouble(distance, 0, range, min, max);
274+
case Region.w:
275+
return lerpDouble(pos.x, 0, width, max, min);
276+
case Region.nw:
277+
var distance = math.max(pos.x, pos.y);
278+
var range = math.min(width, height);
279+
return lerpDouble(distance, 0, range, min, max);
280+
}
281+
174282
for (var package in useLatest) {
175283
_forceLatest.add(package);
176284
}

‎lib/src/chunk_builder.dart

Lines changed: 94 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -891,38 +891,117 @@ class ChunkBuilder {
891891
return chunk;
892892
}
893893

894+
/// Pre-processes the chunks after they are done being written by the visitor
895+
/// but before they are run through the line splitter.
896+
///
897+
/// Marks ranges of chunks that can be line split independently to keep the
898+
/// batches we send to [LineSplitter] small.
899+
void _divideChunks() {
900+
// Harden all of the rules that we know get forced by containing hard
901+
// splits, along with all of the other rules they constrain.
902+
_hardenRules();
903+
904+
var ruleRanges = _calculateRuleRanges();
905+
906+
for (var i = 0; i < _chunks.length; i++) {
907+
var chunk = _chunks[i];
908+
var canDivide = _canDivide(chunk);
909+
910+
if (canDivide) {
911+
// Don't divide in the middle of a rule.
912+
//
913+
// This rarely comes into play since few rules can have hard splits
914+
// between their chunks while not also being hardened themselves. But
915+
// rules for paramater metadata and switch case bodies can.
916+
for (var range in ruleRanges) {
917+
// Example:
918+
// Chunk index: 0 1 2 3 4 5
919+
// Rule: a a [2, 4]
920+
// Divide: | | |
921+
// 1 2 5
922+
if (i > range[0] && i < range[1]) {
923+
canDivide = false;
924+
break;
925+
}
926+
}
927+
}
928+
929+
chunk.markDivide(canDivide);
930+
}
931+
}
932+
894933
/// Returns true if we can divide the chunks at [index] and line split the
895934
/// ones before and after that separately.
896-
bool _canDivideAt(int i) {
935+
bool _canDivide(Chunk chunk) {
897936
// Don't divide at the first chunk.
898-
if (i == 0) return false;
937+
if (chunk == _chunks.first) return false;
899938

900-
var chunk = _chunks[i];
939+
// Can't divide soft rules.
901940
if (!chunk.rule.isHardened) return false;
941+
942+
// Can't divide in the middle of expression nesting.
902943
if (chunk.nesting.isNested) return false;
903944

904945
// If the chunk is the ending delimiter of a block, then don't separate it
905946
// and its children from the preceding beginning of the block.
906-
if (_chunks[i] is BlockChunk) return false;
947+
if (chunk is BlockChunk) return false;
907948

908949
return true;
909950
}
910951

911-
/// Pre-processes the chunks after they are done being written by the visitor
912-
/// but before they are run through the line splitter.
952+
/// For each non-hardened rule, find the range of chunks that share it.
913953
///
914-
/// Marks ranges of chunks that can be line split independently to keep the
915-
/// batches we send to [LineSplitter] small.
916-
void _divideChunks() {
917-
// Harden all of the rules that we know get forced by containing hard
918-
// splits, along with all of the other rules they constrain.
919-
_hardenRules();
954+
/// We must ensure that those chunks are all split together so that they
955+
/// don't try to pick different values for the same rule.
956+
///
957+
/// Returns a list of `[low, high]` ranges.
958+
///
959+
/// Very few rules actually allow a hard split in between chunks sharing that
960+
/// rule (as of today just switch case bodies and parameter metadata). To
961+
/// avoid returning a huge list of mostly useless ranges, we only return
962+
/// ranges for rules that contain at least one hard split between their first
963+
/// and last chunks.
964+
List<List<int>> _calculateRuleRanges() {
965+
var ruleStarts = <Rule, int>{};
966+
var ruleRanges = <Rule, List<int>>{};
967+
var lastHardSplit = -1;
968+
var usefulRanges = <Rule, List<int>>{};
920969

921-
// Now that we know where all of the divided chunk sections are, mark the
922-
// chunks.
923970
for (var i = 0; i < _chunks.length; i++) {
924-
_chunks[i].markDivide(_canDivideAt(i));
971+
var rule = _chunks[i].rule;
972+
if (!rule.isHardened) {
973+
var start = ruleStarts[rule];
974+
if (start == null) {
975+
// This is the first chunk using this rule. Don't create a range yet
976+
// until we see it used by another chunk, since most rules are only
977+
// used for a single chunk.
978+
ruleStarts[rule] = i;
979+
} else {
980+
// This rule now spans at least two chunks, so create a range. The
981+
// low end is the first index using this rule rule.
982+
var range = ruleRanges.putIfAbsent(rule, () => [start, 0]);
983+
984+
// Keep updating the high end of the range as long we continue to find
985+
// the rule.
986+
range[1] = i;
987+
988+
// If we encounter a hard split within this rule's range, keep the
989+
// range.
990+
if (lastHardSplit > range[0] && lastHardSplit < range[1]) {
991+
usefulRanges[rule] = range;
992+
}
993+
}
994+
} else {
995+
lastHardSplit = i;
996+
}
925997
}
998+
999+
// TODO(rnystrom): There is probably something more efficient we can do.
1000+
// We could merge overlapping ranges in the list into larger ones. Or maybe
1001+
// during chunk building, when a Rule sets `splitsOnInnerRules` to `false`
1002+
// and we write a hard chunk, we can mark that hard chunk as not splittable.
1003+
1004+
return usefulRanges.values.toList();
9261005
}
9271006

9281007
/// Hardens the active rules when a hard split occurs within them.

0 commit comments

Comments
 (0)