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

@@ -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. 

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);
     }

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);
     }

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<List<int>> _calculateRuleRanges() {
+    var ruleStarts = <Rule, int>{};
+    var ruleRanges = <Rule, List<int>>{};
+    var lastHardSplit = -1;
+    var usefulRanges = <Rule, List<int>>{};
 
-    // 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.