Format adjacent strings. (#1364)

Format adjacent strings.

Most of this was fairly straightforward, but the two tricky bits are:

### 1. Deciding whether or not to indent

There are some rules around whether subsequent strings in the adjacent
strings get indented or not. The answer is yes in some cases to avoid
confusion:

```
var list = function(
  'string 1',
  'adjacent'
      'string 2',
  'string 3',
];
```

But not in others since it looks nicer to line them up when possible:

```
var description =
    'some text '
    'more text';
```

### 2. Handling `test()` and `group()`

It's really important that test functions don't end up fully split
because doing so would lead to the inner function expression getting
indented +2:

```
test('this looks good', () {
  body;
});

test(
  'this looks bad',
  () {
    body;
  },
);
```

Test descriptions often span multiple lines using adjacent strings:

```
test('this is a very long test description '
    'spanning multiple lines, () {
  body;
});
```

Normally, the newline inside the adjacent strings would cause the entire
argument list to split. The old style handles that (I think) by allowing
multiple block-formatted arguments and then treating both the adjacent
strings and the function expressions as block arguments.

The new style currently only allows a single block argument (because in
almost all of the Flutter code I found using block formatting, one
argument was sufficient). So I chose a more narrowly targeted rule here
where we allow adjacent strings to not prevent block formatting only if
the adjacent strings are the first argument and the block argument is a
function expression as the next argument.

I left a TODO to see if we want to iterate on that rule, but I think it
works pretty well.

### Other stuff

Unlike the old style, I chose to always split between adjacent strings.
The old style will preserve newlines there but if a user chooses to
deliberately put multiple adjacent strings on the same line and they
fit, it will honor it. That didn't seem useful to me, so now they just
always split. I don't think adjacent strings ever look good on the same
line.

I ended up moving the state to track which elements in a ListPiece out
of ListPiece and into the ListElements themselves. I think it's clearer
this way and will be easier to evolve if we end up supporting multiple
block formatted elements in a single list.

Author: munificent

lib/src/ast_extensions.dart

@@ -314,6 +314,63 @@ extension CascadeExpressionExtensions on CascadeExpression {
   }
 }
 
+extension AdjacentStringsExtensions on AdjacentStrings {
+  /// Whether subsequent strings should be indented relative to the first
+  /// string.
+  ///
+  /// We generally want to indent adjacent strings because it can be confusing
+  /// otherwise when they appear in a list of expressions, like:
+  ///
+  ///     [
+  ///       "one",
+  ///       "two"
+  ///       "three",
+  ///       "four"
+  ///     ]
+  ///
+  /// Especially when these strings are longer, it can be hard to tell that
+  /// "three" is a continuation of the previous element.
+  ///
+  /// However, the indentation is distracting in places that don't suffer from
+  /// this ambiguity:
+  ///
+  ///     var description =
+  ///         "A very long description..."
+  ///             "this extra indentation is unnecessary.");
+  ///
+  /// To balance these, we omit the indentation when an adjacent string
+  /// expression is in a context where it's unlikely to be confusing.
+  bool get indentStrings {
+    bool hasOtherStringArgument(List<Expression> arguments) => arguments
+        .any((argument) => argument != this && argument is StringLiteral);
+
+    return switch (parent) {
+      ArgumentList(:var arguments) => hasOtherStringArgument(arguments),
+
+      // Treat asserts like argument lists.
+      Assertion(:var condition, :var message) =>
+        hasOtherStringArgument([condition, if (message != null) message]),
+
+      // Don't add extra indentation in a variable initializer or assignment:
+      //
+      //     var variable =
+      //         "no extra"
+      //         "indent";
+      VariableDeclaration() => false,
+      AssignmentExpression(:var rightHandSide) when rightHandSide == this =>
+        false,
+
+      // Don't indent when following `:`.
+      MapLiteralEntry(:var value) when value == this => false,
+      NamedExpression() => false,
+
+      // Don't indent when the body of a `=>` function.
+      ExpressionFunctionBody() => false,
+      _ => true,
+    };
+  }
+}
+
 extension PatternExtensions on DartPattern {
   /// Whether this expression is a non-empty delimited container for inner
   /// expressions that allows "block-like" formatting in some contexts.

lib/src/front_end/ast_node_visitor.dart

@@ -10,6 +10,7 @@ import '../ast_extensions.dart';
 import '../constants.dart';
 import '../dart_formatter.dart';
 import '../piece/adjacent.dart';
+import '../piece/adjacent_strings.dart';
 import '../piece/assign.dart';
 import '../piece/block.dart';
 import '../piece/constructor.dart';
@@ -110,7 +111,8 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
 
   @override
   Piece visitAdjacentStrings(AdjacentStrings node) {
-    throw UnimplementedError();
+    return AdjacentStringsPiece(node.strings.map(nodePiece).toList(),
+        indent: node.indentStrings);
   }
 
   @override

lib/src/front_end/delimited_list_builder.dart

@@ -52,11 +52,10 @@ class DelimitedListBuilder {
   /// Creates the final [ListPiece] out of the added brackets, delimiters,
   /// elements, and style.
   ListPiece build() {
-    var blockElement = -1;
-    if (_style.allowBlockElement) blockElement = _findBlockElement();
+    _setBlockElementFormatting();
 
-    var piece = ListPiece(_leftBracket, _elements, _blanksAfter, _rightBracket,
-        _style, blockElement);
+    var piece =
+        ListPiece(_leftBracket, _elements, _blanksAfter, _rightBracket, _style);
     if (_mustSplit) piece.pin(State.split);
     return piece;
   }
@@ -155,6 +154,9 @@ class DelimitedListBuilder {
 
     // See if it's an expression that supports block formatting.
     var format = switch (element) {
+      AdjacentStrings(indentStrings: true) =>
+        BlockFormat.indentedAdjacentStrings,
+      AdjacentStrings() => BlockFormat.unindentedAdjacentStrings,
       FunctionExpression() when element.canBlockSplit => BlockFormat.function,
       Expression() when element.canBlockSplit => BlockFormat.block,
       DartPattern() when element.canBlockSplit => BlockFormat.block,
@@ -388,32 +390,86 @@ class DelimitedListBuilder {
     );
   }
 
-  /// If [_blockCandidates] contains a single expression that can receive
-  /// block formatting, then returns its index. Otherwise returns `-1`.
-  int _findBlockElement() {
+  /// Looks at the [BlockFormat] types of all of the elements to determine if
+  /// one of them should be block formatted.
+  ///
+  /// Also, if an argument list has an adjacent strings expression followed by a
+  /// block formattable function expression, we allow the adjacent strings to
+  /// split without forcing the list to split so that it can continue to have
+  /// block formatting. This is pretty special-cased, but it makes calls to
+  /// `test()` and `group()` look better and those are so common that it's
+  /// worth massaging them some. It allows:
+  ///
+  ///     test('some long description'
+  ///         'split across multiple lines', () {
+  ///       expect(1, 1);
+  ///     });
+  ///
+  /// Without this special rule, the newline in the adjacent strings would
+  /// prevent block formatting and lead to the entire test body to be indented:
+  ///
+  ///     test(
+  ///       'some long description'
+  ///       'split across multiple lines',
+  ///       () {
+  ///         expect(1, 1);
+  ///       },
+  ///     );
+  ///
+  /// Stores the result of this calculation by setting flags on the
+  /// [ListElement]s.
+  void _setBlockElementFormatting() {
     // TODO(tall): These heuristics will probably need some iteration.
     var functions = <int>[];
     var others = <int>[];
+    var adjacentStrings = <int>[];
 
     for (var i = 0; i < _elements.length; i++) {
       switch (_elements[i].blockFormat) {
         case BlockFormat.function:
           functions.add(i);
         case BlockFormat.block:
           others.add(i);
+        case BlockFormat.indentedAdjacentStrings:
+        case BlockFormat.unindentedAdjacentStrings:
+          adjacentStrings.add(i);
         case BlockFormat.none:
           break; // Not a block element.
       }
     }
 
-    // A function expression takes precedence over other block arguments.
-    if (functions.length == 1) return functions.first;
+    switch ((functions, others, adjacentStrings)) {
+      // Only allow block formatting in an argument list containing adjacent
+      // strings when:
+      //
+      // 1. The block argument is a function expression.
+      // 2. It is the second argument, following an adjacent strings expression.
+      // 3. There are no other adjacent strings in the argument list.
+      //
+      // This matches the `test()` and `group()` and other similar APIs where
+      // you have a message string followed by a block-like function expression
+      // but little else.
+      // TODO(tall): We may want to iterate on these heuristics. For now,
+      // starting with something very narrowly targeted.
+      case ([1], _, [0]):
+        // The adjacent strings.
+        _elements[0].allowNewlines = true;
+        if (_elements[0].blockFormat == BlockFormat.unindentedAdjacentStrings) {
+          _elements[0].indentWhenBlockFormatted = true;
+        }
+
+        // The block-formattable function.
+        _elements[1].allowNewlines = true;
 
-    // Otherwise, if there is single block argument, it can be block formatted.
-    if (functions.isEmpty && others.length == 1) return others.first;
+      // A function expression takes precedence over other block arguments.
+      case ([var blockArgument], _, _):
+      // Otherwise, if there one block argument, it can be block formatted.
+      case ([], [var blockArgument], _):
+        _elements[blockArgument].allowNewlines = true;
+    }
 
-    // There are no block arguments, or it's ambiguous as to which one should
-    // be it.
+    // If we get here, there are no block arguments, or it's ambiguous as to
+    // which one should be it so none are.
     // TODO(tall): The old formatter allows multiple block arguments, like:
     //
     //     function(() {
@@ -426,6 +482,5 @@ class DelimitedListBuilder {
     // sometimes. We'll probably want to experiment to see if it's worth
     // supporting multiple block arguments. If so, we should at least require
     // them to be contiguous with no non-block arguments in the middle.
-    return -1;
   }
 }

lib/src/piece/adjacent_strings.dart

@@ -0,0 +1,35 @@
+// Copyright (c) 2024, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+import '../back_end/code_writer.dart';
+import '../constants.dart';
+import 'piece.dart';
+
+/// Piece for a series of adjacent strings, like:
+///
+///     var message =
+///         'This is a long message '
+///         'split into multiple strings';
+class AdjacentStringsPiece extends Piece {
+  final List<Piece> _strings;
+
+  /// Whether strings after the first should be indented.
+  final bool _indent;
+
+  AdjacentStringsPiece(this._strings, {bool indent = true}) : _indent = indent;
+
+  @override
+  void format(CodeWriter writer, State state) {
+    if (_indent) writer.setIndent(Indent.expression);
+
+    for (var i = 0; i < _strings.length; i++) {
+      if (i > 0) writer.newline();
+      writer.format(_strings[i]);
+    }
+  }
+
+  @override
+  void forEachChild(void Function(Piece piece) callback) {
+    _strings.forEach(callback);
+  }
+}

lib/src/piece/list.dart

@@ -55,12 +55,8 @@ class ListPiece extends Piece {
   /// The details of how this particular list should be formatted.
   final ListStyle _style;
 
-  /// If this list has an element that can receive block formatting, this is
-  /// the elements's index. Otherwise `-1`.
-  final int _blockElement;
-
   ListPiece(this._before, this._elements, this._blanksAfter, this._after,
-      this._style, this._blockElement);
+      this._style);
 
   @override
   List<State> get additionalStates => [if (_elements.isNotEmpty) State.split];
@@ -102,16 +98,27 @@ class ListPiece extends Piece {
         Commas.none => false,
       };
 
-      // Only allow newlines in the block element or in all elements if we're
-      // fully split.
-      writer.setAllowNewlines(i == _blockElement || state == State.split);
-
       var element = _elements[i];
+
+      // Only some elements (usually a single block element) allow newlines
+      // when the list itself isn't split.
+      writer.setAllowNewlines(element.allowNewlines || state == State.split);
+
+      // If this element allows newlines when the list isn't split, add
+      // indentation if it requires it.
+      if (state == State.unsplit && element.indentWhenBlockFormatted) {
+        writer.setIndent(Indent.expression);
+      }
+
       element.format(writer,
           appendComma: appendComma,
           // Only allow newlines in comments if we're fully split.
           allowNewlinesInComments: state == State.split);
 
+      if (state == State.unsplit && element.indentWhenBlockFormatted) {
+        writer.setIndent(Indent.none);
+      }
+
       // Write a space or newline between elements.
       if (!isLast) {
         writer.splitIf(state != State.unsplit,
@@ -172,6 +179,35 @@ final class ListElement {
   /// What kind of block formatting can be applied to this element.
   final BlockFormat blockFormat;
 
+  /// Whether newlines are allowed in this element when this list is unsplit.
+  ///
+  /// This is generally only true for a single "block" element, as in:
+  ///
+  ///     function(argument, [
+  ///       block,
+  ///       element,
+  ///     ], another);
+  bool allowNewlines = false;
+
+  /// Whether we should increase indentation when formatting this element when
+  /// the list isn't split.
+  ///
+  /// This only comes into play for unsplit lists and is only relevant when the
+  /// element contains newlines, which means that this is only ever useful when
+  /// [allowNewlines] is also true.
+  ///
+  /// This is used for adjacent strings expression at the beginning of an
+  /// argument list followed by a function expression, like in a `test()` call.
+  /// Since the adjacent strings may not require indentation when the list is
+  /// fully split, this ensures that they are indented properly when the list
+  /// isn't split. Avoids:
+  //
+  //     test('long description'
+  //     'that should be indented', () {
+  //       body;
+  //     });
+  bool indentWhenBlockFormatted = false;
+
   /// If this piece has an opening delimiter after the comma, this is its
   /// lexeme, otherwise an empty string.
   ///
@@ -286,6 +322,16 @@ enum BlockFormat {
   /// can be block formatted.
   block,
 
+  /// The element is an adjacent strings expression that's in an list that
+  /// requires its subsequent lines to be indented (because there are other
+  /// string literal in the list).
+  indentedAdjacentStrings,
+
+  /// The element is an adjacent strings expression that's in an list that
+  /// doesn't require its subsequent lines to be indented (because there
+  /// are no other string literals in the list).
+  unindentedAdjacentStrings,
+
   /// The element can't be block formatted.
   none,
 }