Refactor how child chunks are stored. (#1117)

* Refactor how child chunks are stored.

Given a piece of code like:

    main() {
      a();
      b();
    }

There are top level chunks for `main() }` and `}`, and the chunks for
`a()` and `b()` are children.

Previously, the code stored those child chunks in the preceding parent
chunk (so `main() {` here). But it's the subsequent chunk (`}`) that
determines whether the block contents are actually split, so that
doesn't make a lot of sense and leads to weird `+ 1` and `- 1` when
working with nested chunks.

This refactors the code so that child chunks are stored on the same
chunk that determines whether or not they split. This means that chunks
are now written in a post-order traversal: a block chunk's children come
before its own text.

Since we now know that a chunk will have children at the moment that
it's created, removed the old ChunkBlock class and replaced it with a
BlockChunk subclass of Chunk.

Also added a bunch of tests around trailing whitespace in blocks. When I
was testing this change on a corpus of code, I thought it inadvertently
changed some behavior, but it turns out that it was the previous
refactoring (which did deliberately change block formatting) and not
this one. These tests help pin that behavior down.

This commit here has zero changes on the formatter's visible behavior.

* Apply review feedback.

Author: munificent

lib/src/chunk.dart

@@ -89,16 +89,6 @@ class Chunk extends Selection {
   ///             argument));
   final NestingLevel nesting;
 
-  /// If this chunk marks the beginning of a block, this contains the child
-  /// chunks and other data about that nested block.
-  ///
-  /// This should only be accessed when [isBlock] is `true`.
-  ChunkBlock get block => _block!;
-  ChunkBlock? _block;
-
-  /// Whether this chunk has a [block].
-  bool get isBlock => _block != null;
-
   /// The [Rule] that controls when a split should occur before this chunk.
   ///
   /// Multiple splits may share a [Rule].
@@ -136,16 +126,7 @@ class Chunk extends Selection {
   ///
   /// Does not include this chunk's own length, just the length of its child
   /// block chunks (recursively).
-  int get unsplitBlockLength {
-    if (!isBlock) return 0;
-
-    var length = 0;
-    for (var chunk in block.chunks) {
-      length += chunk.length + chunk.unsplitBlockLength;
-    }
-
-    return length;
-  }
+  int get unsplitBlockLength => 0;
 
   /// The [Span]s that contain this chunk.
   final spans = <Span>[];
@@ -189,29 +170,12 @@ class Chunk extends Selection {
     if (space != null) _spaceWhenUnsplit = space;
   }
 
-  /// Turns this chunk into one that can contain a block of child chunks.
-  void makeBlock(Chunk? blockArgument) {
-    assert(_block == null);
-    _block = ChunkBlock(blockArgument);
-  }
-
-  /// Returns `true` if the block body owned by this chunk should be expression
-  /// indented given a set of rule values provided by [getValue].
-  bool indentBlock(int Function(Rule) getValue) {
-    if (!isBlock) return false;
-
-    var argument = block.argument;
-    if (argument == null) return false;
-
-    var rule = argument.rule;
-
-    // There may be no rule if the block occurs inside a string interpolation.
-    // In that case, it's not clear if anything will look particularly nice, but
-    // expression nesting is probably marginally better.
-    if (rule == Rule.dummy) return true;
-
-    return rule.isSplit(getValue(rule), argument);
-  }
+  /// Returns `true` if this chunk is a block whose children should be
+  /// expression indented given a set of rule values provided by [getValue].
+  ///
+  /// [getValue] takes a [Rule] and returns the chosen split state value for
+  /// that [Rule].
+  bool indentBlock(int Function(Rule) getValue) => false;
 
   // Mark whether this chunk can divide the range of chunks.
   void markDivide(bool canDivide) {
@@ -234,9 +198,33 @@ class Chunk extends Selection {
   }
 }
 
-/// The child chunks owned by a chunk that begins a "block" -- an actual block
-/// statement, function expression, or collection literal.
-class ChunkBlock {
+/// A [Chunk] containing a list of nested "child" chunks that are formatted
+/// independently of the surrounding chunks.
+///
+/// This is used for blocks, function expressions, collection literals, etc.
+/// Basically, anywhere we have a delimited body of code whose formatting
+/// doesn't depend on how the surrounding code is formatted except to determine
+/// indentation.
+///
+/// This chunk's own text is the closing delimiter of the block, so its
+/// children come before itself. For example, given this code:
+///
+///     main() {
+///       var list = [
+///         element,
+///       ];
+///     }
+///
+/// It is organized into a tree of chunks like so:
+///
+///    - Chunk           "main() {"
+///    - BlockChunk
+///      |- Chunk          "var list = ["
+///      |- BlockChunk
+///      |  |- Chunk         "element,"
+///      |  '- (text)      "];"
+///      '- (text)       "}"
+class BlockChunk extends Chunk {
   /// If this block is for a collection literal in an argument list, this will
   /// be the chunk preceding this literal argument.
   ///
@@ -245,9 +233,39 @@ class ChunkBlock {
   final Chunk? argument;
 
   /// The child chunks in this block.
-  final List<Chunk> chunks = [];
+  final List<Chunk> children = [];
+
+  BlockChunk(this.argument, super.rule, super.indent, super.nesting,
+      {required super.space, required super.flushLeft})
+      : super(isDouble: false);
 
-  ChunkBlock(this.argument);
+  /// The unsplit length of all of this chunk's block contents.
+  ///
+  /// Does not include this chunk's own length, just the length of its child
+  /// block chunks (recursively).
+  @override
+  int get unsplitBlockLength {
+    var length = 0;
+    for (var chunk in children) {
+      length += chunk.length + chunk.unsplitBlockLength;
+    }
+
+    return length;
+  }
+
+  @override
+  bool indentBlock(int Function(Rule) getValue) {
+    var argument = this.argument;
+    if (argument == null) return false;
+
+    // There may be no rule if the block occurs inside a string interpolation.
+    // In that case, it's not clear if anything will look particularly nice, but
+    // expression nesting is probably marginally better.
+    var rule = argument.rule;
+    if (rule == Rule.dummy) return true;
+
+    return rule.isSplit(getValue(rule), argument);
+  }
 }
 
 /// The in-progress state for a [Span] that has been started but has not yet

lib/src/chunk_builder.dart

@@ -567,28 +567,43 @@ class ChunkBuilder {
     _blockArgumentNesting.removeLast();
   }
 
-  /// Starts a new block as a child of the current chunk.
+  /// Starts a new block chunk and returns the [ChunkBuilder] for it.
   ///
   /// Nested blocks are handled using their own independent [LineWriter].
-  ChunkBuilder startBlock([Chunk? argumentChunk]) {
-    var chunk = _chunks.last;
-    chunk.makeBlock(argumentChunk);
+  ChunkBuilder startBlock(
+      {Chunk? argumentChunk, bool indent = true, bool space = false}) {
+    // Start a block chunk for the block. It will contain the chunks for the
+    // contents of the block, and its own text will be the closing block
+    // delimiter.
+    var chunk = BlockChunk(argumentChunk, _rules.last, _nesting.indentation,
+        _blockArgumentNesting.last,
+        space: space, flushLeft: _pendingFlushLeft);
+    _chunks.add(chunk);
+    _pendingFlushLeft = false;
+
+    var builder = ChunkBuilder._(this, _formatter, _source, chunk.children);
 
-    return ChunkBuilder._(this, _formatter, _source, chunk.block.chunks);
+    if (indent) builder.indent();
+
+    // Create a hard split for the contents. The rule on the parent BlockChunk
+    // determines whether the body is split or not. This hard rule is only when
+    // the block's contents are split.
+    var rule = Rule.hard();
+    builder.startRule(rule);
+    builder.split(nest: false, space: space);
+
+    return builder;
   }
 
   /// Ends this [ChunkBuilder], which must have been created by [startBlock()].
   ///
   /// Forces the chunk that owns the block to split if it can tell that the
   /// block contents will always split. It does that by looking for hard splits
-  /// in the block. If [bodyRule] is given, that rule will be ignored when
-  /// determining if a block contains a hard split. If [space] is `true`, the
-  /// split at the end of the block will get a space when unsplit. If
+  /// in the block that aren't for top level elements in the block. If
   /// [forceSplit] is `true`, the block always splits.
   ///
   /// Returns the previous writer for the surrounding block.
-  ChunkBuilder endBlock(
-      {Rule? bodyRule, bool space = false, bool forceSplit = true}) {
+  ChunkBuilder endBlock({bool forceSplit = true}) {
     _divideChunks();
 
     // If the last chunk ends with a comment that wants a newline after it,
@@ -606,37 +621,21 @@ class ChunkBuilder {
           break;
         }
 
-        // If there are any hardened splits in the chunks (aside from the first
-        // one which is always a hard split since it is the beginning of the
-        // code), then force the collection to split.
-        if (chunk != _chunks.first &&
-            chunk.rule.isHardened &&
-            chunk.rule != bodyRule) {
+        // If there are any hardened splits in the chunks (aside from ones
+        // using the initial hard rule created by [startBlock()] which are for
+        // the top level elements in the block), then force the block to split.
+        if (chunk.rule.isHardened && chunk.rule != _rules.first) {
           forceSplit = true;
           break;
         }
       }
     }
 
-    if (forceSplit) forceRules();
-
-    var parent = _parent!;
-    parent._endChildBlock(space: space, forceSplit: forceSplit);
-    return parent;
-  }
-
-  /// Finishes off the last chunk in a child block of this parent.
-  void _endChildBlock({required bool space, required bool forceSplit}) {
     // If there is a hard newline within the block, force the surrounding rule
     // for it so that we apply that constraint.
-    if (forceSplit) forceRules();
-
-    // Start a new chunk for the code after the block contents. The split at
-    // the beginning of this chunk also determines whether the preceding block
-    // splits and, if so, how it is indented.
-    _startChunk(_blockArgumentNesting.last, isHard: false, space: space);
-
-    if (rule.isHardened) _handleHardSplit();
+    var parent = _parent!;
+    if (forceSplit) parent.forceRules();
+    return parent;
   }
 
   /// Finishes writing and returns a [SourceCode] containing the final output
@@ -902,9 +901,9 @@ class ChunkBuilder {
     if (!chunk.rule.isHardened) return false;
     if (chunk.nesting.isNested) return false;
 
-    // If the previous chunk is a block, then this chunk determines whether the
-    // block contents split, so don't separate it from the block.
-    if (i > 0 && _chunks[i - 1].isBlock) 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;
 
     return true;
   }

lib/src/debug.dart

@@ -70,7 +70,7 @@ void dumpChunks(int start, List<Chunk> chunks) {
     for (var chunk in chunks) {
       spanSet.addAll(chunk.spans);
 
-      if (chunk.isBlock) addSpans(chunk.block.chunks);
+      if (chunk is BlockChunk) addSpans(chunk.children);
     }
   }
 
@@ -82,11 +82,17 @@ void dumpChunks(int start, List<Chunk> chunks) {
   var rows = <List<String>>[];
 
   void addChunk(List<Chunk> chunks, String prefix, int index) {
+    var chunk = chunks[index];
+
+    if (chunk is BlockChunk) {
+      for (var j = 0; j < chunk.children.length; j++) {
+        addChunk(chunk.children, '$prefix$index.', j);
+      }
+    }
+
     var row = <String>[];
     row.add('$prefix$index:');
 
-    var chunk = chunks[index];
-
     void writeIf(predicate, String Function() callback) {
       if (predicate) {
         row.add(callback());
@@ -148,12 +154,6 @@ void dumpChunks(int start, List<Chunk> chunks) {
     }
 
     rows.add(row);
-
-    if (chunk.isBlock) {
-      for (var j = 0; j < chunk.block.chunks.length; j++) {
-        addChunk(chunk.block.chunks, '$prefix$index.', j);
-      }
-    }
   }
 
   for (var i = start; i < chunks.length; i++) {
@@ -222,10 +222,11 @@ void dumpLines(List<Chunk> chunks, SplitSet splits) {
   void writeChunksUnsplit(List<Chunk> chunks) {
     for (var chunk in chunks) {
       if (chunk.spaceWhenUnsplit) buffer.write(' ');
-      buffer.write(chunk.text);
 
       // Recurse into the block.
-      if (chunk.isBlock) writeChunksUnsplit(chunk.block.chunks);
+      if (chunk is BlockChunk) writeChunksUnsplit(chunk.children);
+
+      buffer.write(chunk.text);
     }
   }
 
@@ -241,11 +242,11 @@ void dumpLines(List<Chunk> chunks, SplitSet splits) {
       buffer.write(' ');
     }
 
-    buffer.write(chunk.text);
-
-    if (chunk.isBlock && !splits.shouldSplitAt(i)) {
-      writeChunksUnsplit(chunk.block.chunks);
+    if (chunk is BlockChunk && !splits.shouldSplitAt(i)) {
+      writeChunksUnsplit(chunk.children);
     }
+
+    buffer.write(chunk.text);
   }
 
   log(buffer);

lib/src/line_splitting/solve_state.dart

@@ -297,7 +297,7 @@ class SolveState {
           // And any expression nesting.
           indent += chunk.nesting.totalUsedIndent;
 
-          if (i > 0 && _splitter.chunks[i - 1].indentBlock(getValue)) {
+          if (_splitter.chunks[i].indentBlock(getValue)) {
             indent += Indent.expression;
           }
         }
@@ -389,19 +389,18 @@ class SolveState {
         if (chunk.spaceWhenUnsplit) length++;
       }
 
-      length += chunk.text.length;
-
-      if (chunk.isBlock) {
-        if (_splits.shouldSplitAt(i + 1)) {
+      if (chunk is BlockChunk) {
+        if (_splits.shouldSplitAt(i)) {
           // Include the cost of the nested block.
-          cost += _splitter.writer
-              .formatBlock(chunk, _splits.getColumn(i + 1))
-              .cost;
+          cost +=
+              _splitter.writer.formatBlock(chunk, _splits.getColumn(i)).cost;
         } else {
           // Include the nested block inline, if any.
           length += chunk.unsplitBlockLength;
         }
       }
+
+      length += chunk.text.length;
     }
 
     // Add the costs for the rules that have any splits.