From c9e51918c47388ef53303bbdc3b30bc9e840a51b Mon Sep 17 00:00:00 2001 From: Bob Nystrom Date: Tue, 14 Feb 2023 17:06:06 -0800 Subject: [PATCH] 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. --- CHANGELOG.md | 4 +- benchmark/after.dart.txt | 108 +++++++++++++++++++++++++++++++++ benchmark/before.dart.txt | 108 +++++++++++++++++++++++++++++++++ lib/src/chunk_builder.dart | 109 +++++++++++++++++++++++++++++----- lib/src/rule/argument.dart | 24 +------- lib/src/rule/rule.dart | 25 ++++++++ lib/src/source_visitor.dart | 56 ++++++++++++----- test/comments/switch.stmt | 6 +- test/splitting/switch.stmt | 22 ++++--- test/whitespace/metadata.unit | 3 +- test/whitespace/patterns.stmt | 3 +- 11 files changed, 399 insertions(+), 69 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9c97d913..b633e4c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,7 +1,9 @@ # 2.2.5-dev * Format patterns and related features. -* Allow switch statement case bodies on the same line as the case. +* Allow switch statements where all case bodies are on the same line as the + case when they all fit. +* Fix bug where parameter metadata wouldn't always split when it should. * Don't split after `<` in collection literals. * Format record expressions and record type annotations. * Better indentation of multiline function types inside type argument lists. diff --git a/benchmark/after.dart.txt b/benchmark/after.dart.txt index 30a35b17..2d2ec6b7 100644 --- a/benchmark/after.dart.txt +++ b/benchmark/after.dart.txt @@ -171,6 +171,114 @@ class BacktrackingSolver { : type = type, sources = sources, cache = new PubspecCache(type, sources) { + // A fairly large switch statement. + switch (region) { + case Region.everywhere: + return 0.45; + case Region.n: + return lerpDouble(pos.y, 0, height, min, max); + case Region.ne: + var distance = math.max(width - pos.x - 1, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.e: + return lerpDouble(pos.x, 0, width, min, max); + case Region.se: + var distance = math.max(width - pos.x - 1, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.s: + return lerpDouble(pos.y, 0, height, max, min); + case Region.sw: + var distance = math.max(pos.x, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.w: + return lerpDouble(pos.x, 0, width, max, min); + case Region.nw: + var distance = math.max(pos.x, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.everywhere: + return 0.45; + case Region.n: + return lerpDouble(pos.y, 0, height, min, max); + case Region.ne: + var distance = math.max(width - pos.x - 1, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.e: + return lerpDouble(pos.x, 0, width, min, max); + case Region.se: + var distance = math.max(width - pos.x - 1, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.s: + return lerpDouble(pos.y, 0, height, max, min); + case Region.sw: + var distance = math.max(pos.x, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.w: + return lerpDouble(pos.x, 0, width, max, min); + case Region.nw: + var distance = math.max(pos.x, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.everywhere: + return 0.45; + case Region.n: + return lerpDouble(pos.y, 0, height, min, max); + case Region.ne: + var distance = math.max(width - pos.x - 1, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.e: + return lerpDouble(pos.x, 0, width, min, max); + case Region.se: + var distance = math.max(width - pos.x - 1, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.s: + return lerpDouble(pos.y, 0, height, max, min); + case Region.sw: + var distance = math.max(pos.x, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.w: + return lerpDouble(pos.x, 0, width, max, min); + case Region.nw: + var distance = math.max(pos.x, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.everywhere: + return 0.45; + case Region.n: + return lerpDouble(pos.y, 0, height, min, max); + case Region.ne: + var distance = math.max(width - pos.x - 1, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.e: + return lerpDouble(pos.x, 0, width, min, max); + case Region.se: + var distance = math.max(width - pos.x - 1, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.s: + return lerpDouble(pos.y, 0, height, max, min); + case Region.sw: + var distance = math.max(pos.x, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.w: + return lerpDouble(pos.x, 0, width, max, min); + case Region.nw: + var distance = math.max(pos.x, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + } + for (var package in useLatest) { _forceLatest.add(package); } diff --git a/benchmark/before.dart.txt b/benchmark/before.dart.txt index b0fb12ee..9f9867c7 100644 --- a/benchmark/before.dart.txt +++ b/benchmark/before.dart.txt @@ -171,6 +171,114 @@ class BacktrackingSolver { : type = type, sources = sources, cache = new PubspecCache(type, sources) { + // A fairly large switch statement. + switch (region) { + case Region.everywhere: + return 0.45; + case Region.n: + return lerpDouble(pos.y, 0, height, min, max); + case Region.ne: + var distance = math.max(width - pos.x - 1, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.e: + return lerpDouble(pos.x, 0, width, min, max); + case Region.se: + var distance = math.max(width - pos.x - 1, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.s: + return lerpDouble(pos.y, 0, height, max, min); + case Region.sw: + var distance = math.max(pos.x, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.w: + return lerpDouble(pos.x, 0, width, max, min); + case Region.nw: + var distance = math.max(pos.x, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.everywhere: + return 0.45; + case Region.n: + return lerpDouble(pos.y, 0, height, min, max); + case Region.ne: + var distance = math.max(width - pos.x - 1, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.e: + return lerpDouble(pos.x, 0, width, min, max); + case Region.se: + var distance = math.max(width - pos.x - 1, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.s: + return lerpDouble(pos.y, 0, height, max, min); + case Region.sw: + var distance = math.max(pos.x, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.w: + return lerpDouble(pos.x, 0, width, max, min); + case Region.nw: + var distance = math.max(pos.x, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.everywhere: + return 0.45; + case Region.n: + return lerpDouble(pos.y, 0, height, min, max); + case Region.ne: + var distance = math.max(width - pos.x - 1, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.e: + return lerpDouble(pos.x, 0, width, min, max); + case Region.se: + var distance = math.max(width - pos.x - 1, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.s: + return lerpDouble(pos.y, 0, height, max, min); + case Region.sw: + var distance = math.max(pos.x, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.w: + return lerpDouble(pos.x, 0, width, max, min); + case Region.nw: + var distance = math.max(pos.x, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.everywhere: + return 0.45; + case Region.n: + return lerpDouble(pos.y, 0, height, min, max); + case Region.ne: + var distance = math.max(width - pos.x - 1, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.e: + return lerpDouble(pos.x, 0, width, min, max); + case Region.se: + var distance = math.max(width - pos.x - 1, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.s: + return lerpDouble(pos.y, 0, height, max, min); + case Region.sw: + var distance = math.max(pos.x, height - pos.y - 1); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + case Region.w: + return lerpDouble(pos.x, 0, width, max, min); + case Region.nw: + var distance = math.max(pos.x, pos.y); + var range = math.min(width, height); + return lerpDouble(distance, 0, range, min, max); + } + for (var package in useLatest) { _forceLatest.add(package); } diff --git a/lib/src/chunk_builder.dart b/lib/src/chunk_builder.dart index 5b921fa0..c7165eb7 100644 --- a/lib/src/chunk_builder.dart +++ b/lib/src/chunk_builder.dart @@ -891,38 +891,117 @@ class ChunkBuilder { return chunk; } + /// Pre-processes the chunks after they are done being written by the visitor + /// but before they are run through the line splitter. + /// + /// Marks ranges of chunks that can be line split independently to keep the + /// batches we send to [LineSplitter] small. + void _divideChunks() { + // Harden all of the rules that we know get forced by containing hard + // splits, along with all of the other rules they constrain. + _hardenRules(); + + var ruleRanges = _calculateRuleRanges(); + + for (var i = 0; i < _chunks.length; i++) { + var chunk = _chunks[i]; + var canDivide = _canDivide(chunk); + + if (canDivide) { + // Don't divide in the middle of a rule. + // + // This rarely comes into play since few rules can have hard splits + // between their chunks while not also being hardened themselves. But + // rules for paramater metadata and switch case bodies can. + for (var range in ruleRanges) { + // Example: + // Chunk index: 0 1 2 3 4 5 + // Rule: a a [2, 4] + // Divide: | | | + // 1 2 5 + if (i > range[0] && i < range[1]) { + canDivide = false; + break; + } + } + } + + chunk.markDivide(canDivide); + } + } + /// Returns true if we can divide the chunks at [index] and line split the /// ones before and after that separately. - bool _canDivideAt(int i) { + bool _canDivide(Chunk chunk) { // Don't divide at the first chunk. - if (i == 0) return false; + if (chunk == _chunks.first) return false; - var chunk = _chunks[i]; + // Can't divide soft rules. if (!chunk.rule.isHardened) return false; + + // Can't divide in the middle of expression nesting. if (chunk.nesting.isNested) return false; // If the chunk is the ending delimiter of a block, then don't separate it // and its children from the preceding beginning of the block. - if (_chunks[i] is BlockChunk) return false; + if (chunk is BlockChunk) return false; return true; } - /// Pre-processes the chunks after they are done being written by the visitor - /// but before they are run through the line splitter. + /// For each non-hardened rule, find the range of chunks that share it. /// - /// Marks ranges of chunks that can be line split independently to keep the - /// batches we send to [LineSplitter] small. - void _divideChunks() { - // Harden all of the rules that we know get forced by containing hard - // splits, along with all of the other rules they constrain. - _hardenRules(); + /// We must ensure that those chunks are all split together so that they + /// don't try to pick different values for the same rule. + /// + /// Returns a list of `[low, high]` ranges. + /// + /// Very few rules actually allow a hard split in between chunks sharing that + /// rule (as of today just switch case bodies and parameter metadata). To + /// avoid returning a huge list of mostly useless ranges, we only return + /// ranges for rules that contain at least one hard split between their first + /// and last chunks. + List> _calculateRuleRanges() { + var ruleStarts = {}; + var ruleRanges = >{}; + var lastHardSplit = -1; + var usefulRanges = >{}; - // Now that we know where all of the divided chunk sections are, mark the - // chunks. for (var i = 0; i < _chunks.length; i++) { - _chunks[i].markDivide(_canDivideAt(i)); + var rule = _chunks[i].rule; + if (!rule.isHardened) { + var start = ruleStarts[rule]; + if (start == null) { + // This is the first chunk using this rule. Don't create a range yet + // until we see it used by another chunk, since most rules are only + // used for a single chunk. + ruleStarts[rule] = i; + } else { + // This rule now spans at least two chunks, so create a range. The + // low end is the first index using this rule rule. + var range = ruleRanges.putIfAbsent(rule, () => [start, 0]); + + // Keep updating the high end of the range as long we continue to find + // the rule. + range[1] = i; + + // If we encounter a hard split within this rule's range, keep the + // range. + if (lastHardSplit > range[0] && lastHardSplit < range[1]) { + usefulRanges[rule] = range; + } + } + } else { + lastHardSplit = i; + } } + + // TODO(rnystrom): There is probably something more efficient we can do. + // We could merge overlapping ranges in the list into larger ones. Or maybe + // during chunk building, when a Rule sets `splitsOnInnerRules` to `false` + // and we write a hard chunk, we can mark that hard chunk as not splittable. + + return usefulRanges.values.toList(); } /// Hardens the active rules when a hard split occurs within them. diff --git a/lib/src/rule/argument.dart b/lib/src/rule/argument.dart index e99504bc..0aa16485 100644 --- a/lib/src/rule/argument.dart +++ b/lib/src/rule/argument.dart @@ -5,37 +5,15 @@ import '../chunk.dart'; import 'rule.dart'; /// Base class for a rule that handles argument or parameter lists. -abstract class ArgumentRule extends Rule { +abstract class ArgumentRule extends SplitContainingRule { /// The chunks prior to each positional argument. final List _arguments = []; - /// If true, then inner rules that are written will force this rule to split. - /// - /// Temporarily disabled while writing collection arguments so that they can - /// be multi-line without forcing the whole argument list to split. - bool _trackInnerRules = true; - - /// Don't split when an inner collection rule splits. - @override - bool get splitsOnInnerRules => _trackInnerRules; - /// Remembers [chunk] as containing the split that occurs right before an /// argument in the list. void beforeArgument(Chunk? chunk) { _arguments.add(chunk); } - - /// Disables tracking inner rules while a collection argument is written. - void disableSplitOnInnerRules() { - assert(_trackInnerRules == true); - _trackInnerRules = false; - } - - /// Re-enables tracking inner rules. - void enableSplitOnInnerRules() { - assert(_trackInnerRules == false); - _trackInnerRules = true; - } } /// Rule for handling positional argument lists. diff --git a/lib/src/rule/rule.dart b/lib/src/rule/rule.dart index 2846afca..e396b5a0 100644 --- a/lib/src/rule/rule.dart +++ b/lib/src/rule/rule.dart @@ -213,6 +213,31 @@ class Rule extends FastHash { String toString() => '$id'; } +/// A rule that may contain splits which don't force it to split. +class SplitContainingRule extends Rule { + /// If true, then inner rules that are written will force this rule to split. + /// + /// Temporarily disabled while writing collection arguments so that they can + /// be multi-line without forcing the whole argument list to split. + bool _trackInnerRules = true; + + /// Don't split when an inner collection rule splits. + @override + bool get splitsOnInnerRules => _trackInnerRules; + + /// Disables tracking inner rules while a collection argument is written. + void disableSplitOnInnerRules() { + assert(_trackInnerRules == true); + _trackInnerRules = false; + } + + /// Re-enables tracking inner rules. + void enableSplitOnInnerRules() { + assert(_trackInnerRules == false); + _trackInnerRules = true; + } +} + /// Describes a value constraint that one [Rule] places on another rule's /// values. /// diff --git a/lib/src/source_visitor.dart b/lib/src/source_visitor.dart index 8839142a..5f8d9d76 100644 --- a/lib/src/source_visitor.dart +++ b/lib/src/source_visitor.dart @@ -2813,29 +2813,44 @@ class SourceVisitor extends ThrowingAstVisitor { node.rightParenthesis); _beginBody(node.leftBracket); - for (var member in node.members) { - // If the case pattern and body don't contain any splits, then we allow - // the entire case on one line: - // - // switch (obj) { - // case 1: - // case 2: print('one or two'); - // default: print('other'); - // } - // - // We wrap the rule for this around the entire case so that a split in - // the pattern or the case statement forces splitting after the ":" too. - builder.startLazyRule(); + // If all of the case bodies are small, it looks nice if they go on the same + // line as `case`, like: + // + // switch (obj) { + // case 1: print('one'); + // case 2: + // case 3: print('two or three'); + // } + // + // But it looks bad if some cases are inline and others split: + // + // switch (obj) { + // case 1: print('one'); + // case 2: + // print('two'); + // print('two again'); + // case 3: print('two or three'); + // } + // + // So we use a single rule for all cases. If any case splits, because it has + // multiple statements, or there is a split in the pattern or body, then + // they all split. + var caseRule = SplitContainingRule(); + caseRule.disableSplitOnInnerRules(); + builder.startLazyRule(caseRule); + for (var member in node.members) { _visitLabels(member.labels); token(member.keyword); + // We want a split in the pattern or bodies to force the cases to split. + caseRule.enableSplitOnInnerRules(); + if (member is SwitchCase) { space(); visit(member.expression); } else if (member is SwitchPatternCase) { space(); - var whenClause = member.guardedPattern.whenClause; if (whenClause == null) { builder.indent(); @@ -2854,6 +2869,9 @@ class SourceVisitor extends ThrowingAstVisitor { builder.unnest(); builder.endRule(); } + } else { + assert(member is SwitchDefault); + // Nothing to do. } token(member.colon); @@ -2863,15 +2881,21 @@ class SourceVisitor extends ThrowingAstVisitor { split(); visitNodes(member.statements, between: oneOrTwoNewlines); builder.unindent(); + + // We don't want the split between cases to force them to split. + caseRule.disableSplitOnInnerRules(); oneOrTwoNewlines(); } else { + // We don't want the split between cases to force them to split. + caseRule.disableSplitOnInnerRules(); + // Don't preserve blank lines between empty cases. newline(); } - - builder.endRule(); } + builder.endRule(); + newline(); _endBody(node.rightBracket, forceSplit: true); } diff --git a/test/comments/switch.stmt b/test/comments/switch.stmt index b6668279..350e7c21 100644 --- a/test/comments/switch.stmt +++ b/test/comments/switch.stmt @@ -46,15 +46,13 @@ switch (n) { >>> line comment at end of statement does not force split switch (n) { case 0: zero; // comment - case 1: one; one; // comment + case 1: one; // comment case 2: two; // comment } <<< switch (n) { case 0: zero; // comment - case 1: - one; - one; // comment + case 1: one; // comment case 2: two; // comment } >>> keeps one blank line around case comments in switch expression diff --git a/test/splitting/switch.stmt b/test/splitting/switch.stmt index 562708b7..9390c8e9 100644 --- a/test/splitting/switch.stmt +++ b/test/splitting/switch.stmt @@ -31,7 +31,7 @@ switch (obj) { case 1: case 2: a(); } ->>> single-statement case can stay on one line +>>> single-statement cases can stay on one line switch (obj) { case 1: a(); case 2: b(); @@ -61,7 +61,7 @@ switch (obj) { d(); e(); } ->>> can mix split and unsplit cases +>>> any split case forces all cases to split switch (obj) { case 1: a(); b(); case 2: c(); @@ -73,11 +73,13 @@ switch (obj) { case 1: a(); b(); - case 2: c(); + case 2: + c(); case 3: d(); e(); - default: f(); + default: + f(); } >>> long body statement splits after ":" switch (obj) { @@ -89,8 +91,10 @@ switch (obj) { switch (obj) { case 1: longExpression + thatMustSplit; - case 2: b(); - default: c(); + case 2: + b(); + default: + c(); } >>> split in body statement splits after ":" switch (obj) { @@ -103,8 +107,10 @@ switch (obj) { case 1: longExpression + thatMustSplitAndAgain; - case 2: b(); - default: c(); + case 2: + b(); + default: + c(); } >>> pattern and guard on same line switch (obj) { diff --git a/test/whitespace/metadata.unit b/test/whitespace/metadata.unit index a0ad0243..fe548729 100644 --- a/test/whitespace/metadata.unit +++ b/test/whitespace/metadata.unit @@ -250,7 +250,8 @@ function( function(@Annotation longParameter,@Annotation @Other @Third longParameter2,) {} <<< function( - @Annotation longParameter, + @Annotation + longParameter, @Annotation @Other @Third diff --git a/test/whitespace/patterns.stmt b/test/whitespace/patterns.stmt index aa11eac5..e3cc471f 100644 --- a/test/whitespace/patterns.stmt +++ b/test/whitespace/patterns.stmt @@ -154,7 +154,8 @@ switch (obj) { }: case {k: 1, m: 2}: case {...}: - case {k: _, ...}: ok; + case {k: _, ...}: + ok; } >>> record switch (obj) {