Don't allow a block argument to be named. (#1561)

There are three ways an argument list can be formatted:

```dart
// 1. All inline:
function(argument, another);

// 2. Block formatted:
function(argument, [
  element,
  element,
]);

// 3. Tall style:
function(
  argument,
  another,
);
```

One of the trickiest parts of the formatter is deciding the heuristics to choose between 2 and 3.

Prior to this PR, there is a fairly subtle rule: Arguments before the block argument can be named or not, the block argument can be named or not, and the argument after the block argument can be named or not, but only one of those three sections can have a named argument.

The intent of that rule is to allow named block arguments and named non-block arguments, while avoiding multiple argument names on the same line when block formatted. Also, it allows an argument list containing only a named argument to be block formatted:

```dart
function(name: [
  element,
]);
```

From looking at how contributors to Flutter have hand-formatted their code, it doesn't look like this level of complexity is well motivated. And allowing a single named argument to be block formatted but not if there are other named arguments means that adding a second named argument to a function can cause the entire argument list to be reformatted and indented differently. That can be a lot of churn if the block argument is itself a large nested widget tree.

This PR tweaks that rule to something simpler:

1. The block argument must be positional.

2. Other non-block arguments have no restrictions on whether they can be named or not.

This reduces diff churn and means that widget trees (which almost always use named arguments) have a more consistent (but taller) style across the entire tree.

From talking to Michael Goderbauer, this seems like a reasonable trade-off to me.

Author: munificent

Diff for: benchmark/case/flutter_scrollbar_test.expect

@@ -19,23 +19,25 @@ void main() {
                   child: SizedBox(
                     height: 1000.0,
                     width: double.infinity,
-                    child: Column(children: <Widget>[
-                      Scrollbar(
-                        key: key1,
-                        child: SizedBox(
-                          height: 300.0,
-                          width: double.infinity,
-                          child: SingleChildScrollView(
-                            key: innerKey,
-                            child: const SizedBox(
-                              key: Key('Inner scrollable'),
-                              height: 1000.0,
-                              width: double.infinity,
+                    child: Column(
+                      children: <Widget>[
+                        Scrollbar(
+                          key: key1,
+                          child: SizedBox(
+                            height: 300.0,
+                            width: double.infinity,
+                            child: SingleChildScrollView(
+                              key: innerKey,
+                              child: const SizedBox(
+                                key: Key('Inner scrollable'),
+                                height: 1000.0,
+                                width: double.infinity,
+                              ),
                             ),
                           ),
                         ),
-                      ),
-                    ]),
+                      ],
+                    ),
                   ),
                 ),
               ),

Diff for: lib/src/front_end/delimited_list_builder.dart

@@ -444,33 +444,15 @@ class DelimitedListBuilder {
     var candidateIndex = _candidateBlockArgument(arguments);
     if (candidateIndex == -1) return;
 
+    // The block argument must be positional.
+    if (arguments[candidateIndex] is NamedExpression) 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;
-      }
-
-      return false;
-    }
-
-    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.
@@ -484,18 +466,15 @@ class DelimitedListBuilder {
     //       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
-          )) {
+        arguments[1].blockFormatType == BlockFormat.function &&
+        arguments[0] is! NamedExpression) {
+      var firstArgumentFormatType = arguments[0].blockFormatType;
+      if (firstArgumentFormatType
+          case BlockFormat.unindentedAdjacentStrings ||
+              BlockFormat.indentedAdjacentStrings) {
         // The adjacent strings.
         _elements[0].allowNewlinesWhenUnsplit = true;
-        if (arguments[0].blockFormatType ==
-            BlockFormat.unindentedAdjacentStrings) {
+        if (firstArgumentFormatType == BlockFormat.unindentedAdjacentStrings) {
           _elements[0].indentWhenBlockFormatted = true;
         }
 
@@ -512,11 +491,18 @@ class DelimitedListBuilder {
   /// If an argument in [arguments] is a candidate to be block formatted,
   /// returns its index.
   ///
-  /// Otherwise, returns `-1`.
+  /// If there is a single non-empty block bodied function expression in
+  /// [arguments], returns its index. Otherwise, if there is a single non-empty
+  /// collection literal in [arguments], returns its index. Otherwise, returns
+  /// `-1`.
   int _candidateBlockArgument(List<AstNode> arguments) {
+    // The index of the function expression argument, or -1 if none has been
+    // found or -2 if there are multiple.
     var functionIndex = -1;
+
+    // The index of the collection literal argument, or -1 if none has been
+    // found or -2 if there are multiple.
     var collectionIndex = -1;
-    // var stringIndex = -1;
 
     for (var i = 0; i < arguments.length; i++) {
       // See if it's an expression that supports block formatting.

Diff for: test/tall/invocation/block_argument_named.stmt

@@ -1,66 +1,52 @@
 40 columns                              |
 ### Test how named arguments interact with block formatting.
->>> Block format all positional.
-function(be, fore, [element, element], after);
-<<<
-function(be, fore, [
-  element,
-  element,
-], after);
->>> Block format named only leading.
-function(a: be, b: fore, [element, element], after);
-<<<
-function(a: be, b: fore, [
-  element,
-  element,
-], after);
->>> Block format named only block argument.
-function(be, fore, a: [element, element], after);
-<<<
-function(be, fore, a: [
-  element,
-  element,
-], after);
->>> Block format named only trailing.
-function(be, fore, [element, element], a: after);
-<<<
-function(be, fore, [
-  element,
-  element,
-], a: after);
->>> Don't block format named leading and block argument.
-function(a: be, fore, b: [element, element], after);
+>>> A function block argument can't be named.
+function(name: () {;});
 <<<
 function(
-  a: be,
-  fore,
-  b: [element, element],
-  after,
+  name: () {
+    ;
+  },
 );
->>> Don't block format named leading and trailing.
-function(a: be, fore, [element, element], b: after);
+>>> A collection block argument can't be named.
+function(name: [element, element, element, element]);
 <<<
 function(
-  a: be,
-  fore,
-  [element, element],
-  b: after,
+  name: [
+    element,
+    element,
+    element,
+    element,
+  ],
 );
->>> Don't block format named block argument and trailing.
-function(be, fore, a: [element, element], b: after);
+>>> If there are multiple functions, don't block format, even if only one is positional.
+function(a: () {;}, () {;});
 <<<
 function(
-  be,
-  fore,
-  a: [element, element],
-  b: after,
+  a: () {
+    ;
+  },
+  () {
+    ;
+  },
 );
->>> Don't block format all named.
-function(a: be, b: fore, c: [element, element], d: after);
+>>> If there are multiple collections, don't block format, even if only one is positional.
+function(a: [element], [element, element, element, element, element]);
 <<<
 function(
-  a: be,
-  b: fore,
-  c: [element, element],
-  d: after,
-);
+  a: [element],
+  [
+    element,
+    element,
+    element,
+    element,
+    element,
+  ],
+);
+>>> Other non-block arguments can be named or not.
+function(1, a: 2, 3, b: 4, [element, element], c: 5);
+<<<
+function(1, a: 2, 3, b: 4, [
+  element,
+  element,
+], c: 5);

Diff for: test/tall/invocation/block_argument_other.stmt

@@ -34,20 +34,6 @@ function(
     element,
   ],
 );
->>> A function block argument can be named.
-function(name: () { body; });
-<<<
-function(name: () {
-  body;
-});
->>> A non-function block argument can be named.
-function(name: [element, element, element]);
-<<<
-function(name: [
-  element,
-  element,
-  element,
-]);
 >>> A long argument name can prevent block formatting.
 veryLongFunctionName(veryLongArgumentName: [element]);
 <<<

Diff for: test/tall/regression/0100/0198.stmt

@@ -24,38 +24,38 @@
     });
   });
 <<<
-  testThat('backward navigation is disabled when at end of stream', when: (
-    TaskList taskList,
-    TaskService taskService,
-  ) {
-    var cursorPageNo = 0;
-    final streamCtrl = initCustomTaskServiceMock(
-      taskService,
-      canMoveTo: (pageNo) => pageNo < 0 ? false : true,
-      getCurrentPageNumber: () => cursorPageNo,
-    );
+  testThat(
+    'backward navigation is disabled when at end of stream',
+    when: (TaskList taskList, TaskService taskService) {
+      var cursorPageNo = 0;
+      final streamCtrl = initCustomTaskServiceMock(
+        taskService,
+        canMoveTo: (pageNo) => pageNo < 0 ? false : true,
+        getCurrentPageNumber: () => cursorPageNo,
+      );
 
-    first('attach tasklist', () {
-          taskList.attach();
-          addTasks(streamCtrl);
-        })
-        .thenExpect(
-          'pager at page 1',
-          () => {
-            taskList.currentPageNo: 1,
-            taskList.backwardPaginationDisabled: isFalse,
-          },
-        )
-        .then('go to page 2', () {
-          taskList.nextPage();
-          addTasks(streamCtrl, count: 1);
-          cursorPageNo = 1;
-        })
-        .thenExpect(
-          'pager unchanged',
-          () => {
-            taskList.currentPageNo: 2,
-            taskList.backwardPaginationDisabled: isTrue,
-          },
-        );
-  });
+      first('attach tasklist', () {
+            taskList.attach();
+            addTasks(streamCtrl);
+          })
+          .thenExpect(
+            'pager at page 1',
+            () => {
+              taskList.currentPageNo: 1,
+              taskList.backwardPaginationDisabled: isFalse,
+            },
+          )
+          .then('go to page 2', () {
+            taskList.nextPage();
+            addTasks(streamCtrl, count: 1);
+            cursorPageNo = 1;
+          })
+          .thenExpect(
+            'pager unchanged',
+            () => {
+              taskList.currentPageNo: 2,
+              taskList.backwardPaginationDisabled: isTrue,
+            },
+          );
+    },
+  );

Diff for: test/tall/regression/0300/0394.stmt

@@ -14,25 +14,27 @@ return $.Div(inner: [
     ]),
 ]);
 <<<
-return $.Div(inner: [
-  $.Div(
-    id: "container",
-    inner: [
-      $.Canvas(width: maxD, height: maxD, clazz: "center", ref: canvas),
-      $.Form(
-        clazz: "center",
-        inner: [
-          $.Input(
-            type: "range",
-            max: 1000,
-            value: seeds,
-            onChange: onSliderChange,
-          ),
-        ],
-      ),
-      $.Img(src: "math.png", width: "350px", height: "42px", clazz: "center"),
-    ],
-  ),
+return $.Div(
+  inner: [
+    $.Div(
+      id: "container",
+      inner: [
+        $.Canvas(width: maxD, height: maxD, clazz: "center", ref: canvas),
+        $.Form(
+          clazz: "center",
+          inner: [
+            $.Input(
+              type: "range",
+              max: 1000,
+              value: seeds,
+              onChange: onSliderChange,
+            ),
+          ],
+        ),
+        $.Img(src: "math.png", width: "350px", height: "42px", clazz: "center"),
+      ],
+    ),
 
-  $.Footer(inner: [$.P(id: "notes", inner: "${seeds} seeds")]),
-]);
+    $.Footer(inner: [$.P(id: "notes", inner: "${seeds} seeds")]),
+  ],
+);