Change the block format heuristics.

This produces output that better matches what users expected in the associated issues. I think it also looks better in the affected regression tests. And I ran this on a large corpus and looking at the diffs, these rules seem to be much better across the board.

The new rules are basically:

* A block formatted argument list can't end up with named arguments on multiple lines.

* Only one trailing argument is allowed after the block argument.

The existing rules are also kept:

* Prefer a function block argument over a collection one.

* Only allow a single block argument.

* Allow the first argument to be adjacent strings with a later function block argument (to handle stuff like test() and group()).

Fix #1527.
Fix #1528.
Fix #1543.

Author: munificent

lib/src/ast_extensions.dart

@@ -7,6 +7,16 @@ import 'package:analyzer/dart/ast/token.dart';
 import 'piece/list.dart';
 
 extension AstNodeExtensions on AstNode {
+  /// When this node is in an argument list, what kind of block formatting
+  /// category it belongs to.
+  BlockFormat get blockFormatType => switch (this) {
+        AdjacentStrings(indentStrings: true) =>
+          BlockFormat.indentedAdjacentStrings,
+        AdjacentStrings() => BlockFormat.unindentedAdjacentStrings,
+        Expression(:var blockFormatType) => blockFormatType,
+        _ => BlockFormat.none,
+      };
+
   /// The first token at the beginning of this AST node, not including any
   /// tokens for leading doc comments.
   ///

lib/src/front_end/delimited_list_builder.dart

@@ -441,97 +441,111 @@ class DelimitedListBuilder {
   /// Stores the result of this calculation by setting flags on the
   /// [ListElement]s.
   void _setBlockArgument(List<AstNode> arguments) {
-    var firstIsUnindentedAdjacentStrings = false;
-    var functions = <int>[];
-    var collections = <int>[];
-    var adjacentStrings = <int>[];
-
-    for (var i = 0; i < arguments.length; i++) {
-      var argument = arguments[i];
-      // See if it's an expression that supports block formatting.
-      var format = switch (argument) {
-        AdjacentStrings(indentStrings: true) =>
-          BlockFormat.indentedAdjacentStrings,
-        AdjacentStrings() => BlockFormat.unindentedAdjacentStrings,
-        Expression() => argument.blockFormatType,
-        DartPattern() when argument.canBlockSplit => BlockFormat.collection,
-        _ => BlockFormat.none,
-      };
-
-      if (i == 0 && format == BlockFormat.unindentedAdjacentStrings) {
-        firstIsUnindentedAdjacentStrings = true;
+    var candidateIndex = _candidateBlockArgument(arguments);
+    if (candidateIndex == -1) return;
+
+    // Only allow up to one trailing argument after the block argument. This
+    // handles the common `tags` and `timeout` named arguments in `test()` and
+    // `group()` while still mostly having the block argument be at the end of
+    // the argument list.
+    if (candidateIndex < arguments.length - 2) return;
+
+    // If there are multiple named arguments, they should never end up on
+    // separate lines (unless the whole argument list fully splits). Otherwise,
+    // it's too easy for an argument name to get buried in the middle of a line.
+    // So we look for named arguments before, on, and after the candidate
+    // argument. If more than one of those sections of arguments has a named
+    // argument, then we don't allow the block argument.
+    var namedSections = 0;
+    bool hasNamedArgument(int from, int to) {
+      for (var i = from; i < to; i++) {
+        if (arguments[i] is NamedExpression) return true;
       }
 
-      switch (format) {
-        case BlockFormat.function:
-          functions.add(i);
-        case BlockFormat.collection:
-          collections.add(i);
-        case BlockFormat.invocation:
-          // We don't allow function calls as block elements partially for style
-          // and partially for performance. It often doesn't look great to let
-          // nested function calls pack arbitrarily deeply as block arguments:
-          //
-          //     ScaffoldMessenger.of(context).showSnackBar(SnackBar(
-          //         content: Text(
-          //       localizations.demoSnackbarsAction,
-          //     )));
-          //
-          // This is better when expanded like:
-          //
-          //     ScaffoldMessenger.of(context).showSnackBar(
-          //       SnackBar(
-          //         content: Text(
-          //           localizations.demoSnackbarsAction,
-          //         ),
-          //       ),
-          //     );
-          //
-          // Also, when invocations can be block arguments, which themselves
-          // may contain block arguments, it's easy to run into combinatorial
-          // performance in the solver as it tries to determine which of the
-          // nested calls should and shouldn't be block formatted.
-          break;
-        case BlockFormat.indentedAdjacentStrings:
-        case BlockFormat.unindentedAdjacentStrings:
-          adjacentStrings.add(i);
-        case BlockFormat.none:
-          break; // Not a block element.
-      }
+      return false;
     }
 
-    // TODO(tall): These heuristics will probably need some iteration.
-    switch ((functions, collections, 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]):
+    if (hasNamedArgument(0, candidateIndex)) namedSections++;
+    if (hasNamedArgument(candidateIndex, candidateIndex + 1)) namedSections++;
+    if (hasNamedArgument(candidateIndex + 1, arguments.length)) namedSections++;
+
+    if (namedSections > 1) return;
+
+    // Edge case: If the first argument is adjacent strings and the second
+    // argument is a function literal, with optionally a third non-block
+    // argument, then treat the function as the block argument.
+    //
+    // 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, as in:
+    //
+    //     test('Some long test description '
+    //         'that splits into multiple lines.', () {
+    //       expect(1 + 2, 3);
+    //     });
+    if (candidateIndex == 1 &&
+        arguments[0] is! NamedExpression &&
+        arguments[1] is! NamedExpression) {
+      if ((arguments[0].blockFormatType, arguments[1].blockFormatType)
+          case (
+            BlockFormat.unindentedAdjacentStrings ||
+                BlockFormat.indentedAdjacentStrings,
+            BlockFormat.function
+          )) {
         // The adjacent strings.
         _elements[0].allowNewlinesWhenUnsplit = true;
-        if (firstIsUnindentedAdjacentStrings) {
+        if (arguments[0].blockFormatType ==
+            BlockFormat.unindentedAdjacentStrings) {
           _elements[0].indentWhenBlockFormatted = true;
         }
 
         // The block-formattable function.
         _elements[1].allowNewlinesWhenUnsplit = true;
+        return;
+      }
+    }
 
-      // A function expression takes precedence over other block arguments.
-      case ([var element], _, _):
-        _elements[element].allowNewlinesWhenUnsplit = true;
+    // If we get here, we have a block argument.
+    _elements[candidateIndex].allowNewlinesWhenUnsplit = true;
+  }
+
+  /// If an argument in [arguments] is a candidate to be block formatted,
+  /// returns its index.
+  ///
+  /// Otherwise, returns `-1`.
+  int _candidateBlockArgument(List<AstNode> arguments) {
+    var functionIndex = -1;
+    var collectionIndex = -1;
+    // var stringIndex = -1;
+
+    for (var i = 0; i < arguments.length; i++) {
+      // See if it's an expression that supports block formatting.
+      switch (arguments[i].blockFormatType) {
+        case BlockFormat.function:
+          if (functionIndex >= 0) {
+            functionIndex = -2;
+          } else {
+            functionIndex = i;
+          }
+
+        case BlockFormat.collection:
+          if (collectionIndex >= 0) {
+            collectionIndex = -2;
+          } else {
+            collectionIndex = i;
+          }
 
-      // A single collection literal can be block formatted even if there are
-      // other arguments.
-      case ([], [var element], _):
-        _elements[element].allowNewlinesWhenUnsplit = true;
+        case BlockFormat.invocation:
+        case BlockFormat.indentedAdjacentStrings:
+        case BlockFormat.unindentedAdjacentStrings:
+        case BlockFormat.none:
+          break; // Normal argument.
+      }
     }
+
+    if (functionIndex >= 0) return functionIndex;
+    if (collectionIndex >= 0) return collectionIndex;
+
+    return -1;
   }
 }

lib/src/piece/list.dart

@@ -398,9 +398,10 @@ enum BlockFormat {
   /// elements.
   function,
 
-  /// The element is a collection literal.
+  /// The element is a collection literal or multiline string literal.
   ///
-  /// These can be block formatted even when there are other arguments.
+  /// If there is only one of these and no [BlockFormat.function] elements, then
+  /// it can be block formatted.
   collection,
 
   /// A function or method invocation.
@@ -410,7 +411,7 @@ enum BlockFormat {
 
   /// 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).
+  /// string literals in the list).
   indentedAdjacentStrings,
 
   /// The element is an adjacent strings expression that's in an list that

test/tall/invocation/block_argument_description.stmt

@@ -0,0 +1,104 @@
+40 columns                              |
+### Test how block formatting functions like test(), group(), that start with
+### leading adjacent strings behaves.
+>>> Adjacent strings preceding a function expression doesn't prevent block formatting.
+test('First adjacent string' 'second adjacent string'
+'third adjacent string', () async {
+  ;
+});
+<<<
+test('First adjacent string'
+    'second adjacent string'
+    'third adjacent string', () async {
+  ;
+});
+>>> Don't block format a function with a preceding adjacent string if it doesn't fit.
+test('First adjacent string' 'second long adjacent string', () async {
+  ;
+});
+<<<
+test(
+  'First adjacent string'
+  'second long adjacent string',
+  () async {
+    ;
+  },
+);
+>>> Don't block format adjacent strings preceding a non-function block argument.
+test('First adjacent string'
+    'second adjacent string'
+    'third adjacent string', [
+  element1,
+  element2,
+  element3,
+  element4,
+]);
+<<<
+test(
+  'First adjacent string'
+  'second adjacent string'
+  'third adjacent string',
+  [
+    element1,
+    element2,
+    element3,
+    element4,
+  ],
+);
+>>> Another string argument doesn't prevent block formatting.
+test('First string line 1' 'first string line 2', () {
+  ;
+}, 'Another simple string');
+<<<
+test('First string line 1'
+    'first string line 2', () {
+  ;
+}, 'Another simple string');
+>>> Multiple trailing arguments prevent block formatting.
+test('First string line 1' 'first string line 2', () {
+  ;
+}, trailing, another);
+<<<
+test(
+  'First string line 1'
+  'first string line 2',
+  () {
+    ;
+  },
+  trailing,
+  another,
+);
+>>> Don't block format if the leading string is named.
+test(named: 'First string line 1' 'first string line 2', () {
+  ;
+});
+<<<
+test(
+  named:
+      'First string line 1'
+      'first string line 2',
+  () {
+    ;
+  },
+);
+>>> Don't block format if the leading function is named.
+test('First string line 1' 'first string line 2', named: () {
+  ;
+});
+<<<
+test(
+  'First string line 1'
+  'first string line 2',
+  named: () {
+    ;
+  },
+);
+>>> Allow block formatting when the trailing argument is named.
+test('First string line 1' 'first string line 2', () {
+  ;
+}, named: arg);
+<<<
+test('First string line 1'
+    'first string line 2', () {
+  ;
+}, named: arg);