Fix bug in subtree merging cost calculation. (#1377)

An important optimization during solving is hoisting out a separable
Piece subtree, formatting in its own isolated Solver, and then merging
the resulting Solution back into the parent Solution.

When we merge, we add in all of the data from the subtree to the main
one. It's important that doing so yields the exact same result that we
would get if we had solved that subtree directly inline in the parent
Solution.

Unfortunately, the previous code didn't do that. In some cases, pieces
that are bound by the subtree Solution would already be bound in the
parent one too. In that case, merging would double-count the cost of
those pieces.

This fixes that by only counting the cost of pieces that weren't already
bound in the parent.

I ran into this bug while running the large benchmark and it was
surprisingly hard to hoist out a small separable test case. The issue
wasn't really specific to any particular Piece or syntax. It's really a
regression test.

The regression tests aren't migrated for the new formatter yet, so I
made a new directory "regression_tall" and put it there. I'm not sure if
that's ultimately the naming convention we'll want.

Also, while I was investigating this, I noticed that ListPiece could
theoretically try to format a child Piece separately when doing so
wasn't valid because there might be code from a sibling or parent piece
on its line. (In practice, that can't happen because the only
SequencePiece that doesn't have surrounding brackets is the one
top-level sequence for the compilation unit. So there definitely won't
be anything before or after that. But if we ever use SequencePiece in
other places without brackets, this avoids that becoming a bug.)

Author: munificent

lib/src/back_end/solution.dart

@@ -168,8 +168,15 @@ class Solution implements Comparable<Solution> {
   /// the resulting solution is being merged with this one.
   void mergeSubtree(Solution subtreeSolution) {
     _overflow += subtreeSolution._overflow;
-    _cost += subtreeSolution.cost;
-    _pieceStates.addAll(subtreeSolution._pieceStates);
+
+    // Add the subtree's bound pieces to this one. Make sure to not double
+    // count costs for pieces that are already bound in this one.
+    subtreeSolution._pieceStates.forEach((piece, state) {
+      _pieceStates.putIfAbsent(piece, () {
+        _cost += piece.stateCost(state);
+        return state;
+      });
+    });
   }
 
   /// Sets [selectionStart] to be [start] code units into the output.

lib/src/piece/sequence.dart

@@ -40,9 +40,14 @@ class SequencePiece extends Piece {
     for (var i = 0; i < _elements.length; i++) {
       var element = _elements[i];
 
-      // If the sequence is split, then every element is on its own line and
-      // can be formatted separately.
-      writer.format(element, separate: state == State.split);
+      // We can format an element separately if the element is on its own line.
+      // This happens when the sequence is split and there is something before
+      // and after the element, either brackets or other items.
+      var separate = state == State.split &&
+          (i > 0 || _leftBracket != null) &&
+          (i < _elements.length - 1 || _rightBracket != null);
+
+      writer.format(element, separate: separate);
 
       if (i < _elements.length - 1) {
         writer.newline(

test/regression_tall/other/misc.unit

@@ -0,0 +1,22 @@
+>>> Incorrect cost calculation when merging subtrees led to wrong solution.
+SomeVeryLongReturnType
+someFunctionName(SomeType_parameter1 AnotherType_parameter2) {
+  {
+    deps.then(() {
+          ;
+        })
+        .then(________traverseDeps_depender__deps_);
+  }
+}
+<<<
+SomeVeryLongReturnType someFunctionName(
+  SomeType_parameter1 AnotherType_parameter2,
+) {
+  {
+    deps
+        .then(() {
+          ;
+        })
+        .then(________traverseDeps_depender__deps_);
+  }
+}

test/tall_format_test.dart

@@ -21,6 +21,7 @@ void main() async {
   await testDirectory('top_level', tall: true);
   await testDirectory('type', tall: true);
   await testDirectory('variable', tall: true);
+  await testDirectory('regression_tall', tall: true);
 
   test('throws a FormatterException on failed parse', () {
     var formatter = DartFormatter();