From 6a53e14521ae17758560089d4a19529bf71212d3 Mon Sep 17 00:00:00 2001
From: Robert Nystrom <rnystrom@google.com>
Date: Wed, 2 Apr 2025 20:58:55 -0700
Subject: [PATCH] Make the eager argument list splitting heuristic more
 conservative.

Some folks on the Flutter team pointed out that the previous rule is too aggressive and splits even simple common code like:

```dart
Text('Item 1', style: TextStyle(color: Colors.white));
```

That's probably simple enough to stay on one line. So this tweaks the heuristic to allow that to remain unsplit. Where the old heuristic split any argument list with a named argument that contained a nested named argument, this requires there to be at least *three* named arguments.

It's sort of an arbitrary cutoff, but it seems to do a good job when run on a large corpus.
---
 CHANGELOG.md                               |  25 +--
 lib/src/front_end/expression_contents.dart |  86 ++++++---
 lib/src/front_end/piece_factory.dart       |   2 +-
 test/tall/invocation/nested.stmt           | 207 ++++++++++++++-------
 test/tall/regression/0300/0394.stmt        |   4 +-
 test/tall/regression/1500/1527.unit        |   4 +-
 test/tall/regression/other/flutter.unit    |  41 ++++
 7 files changed, 255 insertions(+), 114 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c759a96e..60645b4f 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -151,26 +151,27 @@
   to read spread across multiple lines even if they would otherwise fit on a
   single line (#1660). The rules are basically:
 
-  * If an argument list contains any named arguments and contains any other
-    calls whose argument lists contain any named arguments (directly or
-    indirectly), then split the outer one. We make an exception where a named
-    argument whose expression is a simple number, Boolean, or null literal
-    doesn't count as a named argument.
+  * If an argument list contains at least three named arguments, at least one
+    of which must be directly in the argument list and at least one of which
+    must be nested in an inner argument list, then force the outer one to split.
+    We make an exception where a named argument whose expression is a simple
+    number, Boolean, or null literal doesn't count as a named argument.
 
   * If a list, set, or map literal is the immediate expression in a named
-    argument and one of the above kinds of argument lists (directly or
-    indirectly), then force the collection to split.
+    argument and contains any argument lists with a named argument, then force
+    the collection to split.
 
   ```dart
   // Before:
-  Stack(children: [result, Semantics(label: indexLabel)]);
+  TabBar(tabs: [Tab(text: 'A'), Tab(text: 'B')], labelColor: Colors.white70);
 
   // After:
-  Stack(
-    children: [
-      result,
-      Semantics(label: indexLabel),
+  TabBar(
+    tabs: [
+      Tab(text: 'A'),
+      Tab(text: 'B'),
     ],
+    labelColor: Colors.white70,
   );
   ```
 
diff --git a/lib/src/front_end/expression_contents.dart b/lib/src/front_end/expression_contents.dart
index ecd8b145..60a19380 100644
--- a/lib/src/front_end/expression_contents.dart
+++ b/lib/src/front_end/expression_contents.dart
@@ -51,40 +51,49 @@ class ExpressionContents {
   /// Begins tracking an argument list.
   void beginCall(List<AstNode> arguments) {
     var type = _Type.otherCall;
-    var hasNontrivialNamedArgument = false;
+
+    // Count the non-trivial named arguments in this call.
+    var namedArguments = 0;
     for (var argument in arguments) {
       if (argument is NamedExpression) {
         type = _Type.callWithNamedArgument;
-        if (!_isTrivial(argument.expression)) {
-          hasNontrivialNamedArgument = true;
-          break;
-        }
+        if (!_isTrivial(argument.expression)) namedArguments++;
       }
     }
 
-    if (hasNontrivialNamedArgument) _stack.last.nontrivialCalls++;
-
-    _stack.add(_Contents(type));
+    _stack.add(_Contents(type, namedArguments: namedArguments));
   }
 
   /// Ends the most recently begun call and returns `true` if its argument list
   /// should eagerly split.
-  bool endCall() {
+  bool endCall(List<Expression> arguments) {
     var contents = _end();
 
-    // If this argument list has any named arguments and it contains another
-    // call with any non-trivial named arguments, then split it. The idea here
-    // is that when scanning a line of code, it's hard to tell which calls own
-    // which named arguments. If there are named arguments at different levels
-    // of nesting in an expression tree, it's better to split the outer one to
-    // make that clearer.
+    // If there are "too many" named arguments in this call and the calls it
+    // contains, then split it.
+    //
+    // The basic idea is that when scanning a line of code, it's hard to tell
+    // which calls own which named arguments if there are named arguments at
+    // multiple levels in the call tree. Splitting makes that clearer. At the
+    // same time, it's annoying it the formatter is too aggressive about
+    // splitting an expression that feels simple enough to the reader to fit on
+    // one line. (Especially because if the formatter does eagerly split it,
+    // there's nothing they can do to *prevent* that.)
+    //
+    // The heuristic here tries to strike a "Goldilocks" balance between not
+    // splitting too aggressively or too conservatively. The rule is that the
+    // entire call tree must contain at least three named arguments, at least
+    // one must be in the outermost call being split, and at least one must
+    // *not* be in the outermost call.
+    //
+    // It would be simpler to split any call that has named arguments at
+    // different nesting levels, but that's a little too aggressive and forces
+    // common code like this to split:
     //
-    // With this rule, any two named arguments in the middle of the same line
-    // will always be owned by the same call. There may be a named argument at
-    // the very beginning of the line owned by the surrounding call which is
-    // forced to split.
-    return (contents.type == _Type.callWithNamedArgument) &&
-        contents.nontrivialCalls > 0;
+    //       Text('Item 1', style: TextStyle(color: Colors.white));
+    return contents.totalNamedArguments > 2 &&
+        contents.namedArguments > 0 &&
+        contents.nestedNamedArguments > 0;
   }
 
   /// Begin tracking a collection literal and its contents.
@@ -102,9 +111,9 @@ class ExpressionContents {
     if (contents.collections > 0) return true;
 
     // If the collection is itself a named argument in a surrounding call that
-    // is going to be forced to eagerly split, then split the collection too.
-    // In this case, the collection is sort of like a vararg argument to the
-    // call. Prefers:
+    // may be be forced to eagerly split, then split the collection too. In
+    // that case, the collection is sort of like a vararg argument to the call.
+    // Prefers:
     //
     //     TabBar(
     //       tabs: <Widget>[
@@ -118,8 +127,14 @@ class ExpressionContents {
     //     TabBar(
     //       tabs: <Widget>[Tab(text: 'Tab 1'), Tab(text: 'Tab 2')],
     //     );
-    return contents.type == _Type.namedCollection &&
-        contents.nontrivialCalls > 0;
+    //
+    // Splitting a collection is also helpful, because it shows each element
+    // in parallel with each on its own line. But that's only true when there
+    // are multiple elements, so we don't eagerly split collections with just a
+    // single element.
+    return elements.length > 1 &&
+        contents.type == _Type.namedCollection &&
+        contents.totalNamedArguments > 0;
   }
 
   /// Ends the most recently begun operation and returns its contents.
@@ -129,7 +144,8 @@ class ExpressionContents {
     // Transitively include this operation's contents in the surrounding one.
     var parent = _stack.last;
     parent.collections += contents.collections;
-    parent.nontrivialCalls += contents.nontrivialCalls;
+    parent.nestedNamedArguments +=
+        contents.namedArguments + contents.nestedNamedArguments;
 
     return contents;
   }
@@ -169,11 +185,19 @@ class _Contents {
   /// this operation.
   int collections = 0;
 
-  /// The number of calls with non-trivial named arguments transitively inside
-  /// this operation.
-  int nontrivialCalls = 0;
+  /// The number of non-trivial named arguments in this call's own argument
+  /// list.
+  int namedArguments = 0;
+
+  /// The number of non-trivial named arguments transitively inside this
+  /// operation, but not including the call's own named arguments.
+  int nestedNamedArguments = 0;
+
+  _Contents(this.type, {this.namedArguments = 0});
 
-  _Contents(this.type);
+  /// The total number of non-trivial named arguments in this operation's own
+  /// argument list and all of transitive contents.
+  int get totalNamedArguments => namedArguments + nestedNamedArguments;
 }
 
 enum _Type {
diff --git a/lib/src/front_end/piece_factory.dart b/lib/src/front_end/piece_factory.dart
index d4cfd002..c9e5b380 100644
--- a/lib/src/front_end/piece_factory.dart
+++ b/lib/src/front_end/piece_factory.dart
@@ -144,7 +144,7 @@ mixin PieceFactory {
     );
 
     // If the call is complex enough, force it to split even if it would fit.
-    if (_contents.endCall()) {
+    if (_contents.endCall(arguments)) {
       // Don't force an argument list to fully split if it could block split.
       // TODO(rnystrom): Ideally, if the argument list has a block argument, we
       // would force it to either block split or fully split, but disallow it
diff --git a/test/tall/invocation/nested.stmt b/test/tall/invocation/nested.stmt
index 7edf7702..c4090179 100644
--- a/test/tall/invocation/nested.stmt
+++ b/test/tall/invocation/nested.stmt
@@ -1,138 +1,217 @@
 40 columns                              |
 ### Test how argument lists split eagerly when they contain other calls.
->>> Split outer call if both have named arguments.
-outer(name: inner(name: value));
+>>> Don't split if outer call has only positional arguments.
+a(b(c(d), e(f)), g(h), i(j));
+<<<
+a(b(c(d), e(f)), g(h), i(j));
+>>> Don't split even if outer call has many named arguments.
+outer(a: a(1), b: b(2), c: c(3), d: d);
+<<<
+outer(a: a(1), b: b(2), c: c(3), d: d);
+>>> Don't split if outer and inner each have only one named argument.
+outer(1, n: value, inner(n: value, 2));
+<<<
+outer(1, n: value, inner(n: value, 2));
+>>> Don't split if inner has many named arguments when outer has none.
+f(1, g(a: a, b: b, c: c, d: d));
+<<<
+f(1, g(a: a, b: b, c: c, d: d));
+>>> Split outer call if there are more than two outer and inner named arguments.
+outer(a: a, b: inner(c: c));
 <<<
 outer(
-  name: inner(name: value),
+  a: a,
+  b: inner(c: c),
 );
->>> Split outer call on indirect inner call.
-outer(name: !(inner(name: value)));
+>>> Split outer call if there are more than two outer and inner named arguments.
+outer(a: a, inner(b: b, c: c));
 <<<
 outer(
-  name: !(inner(name: value)),
+  a: a,
+  inner(b: b, c: c),
 );
->>> Don't split if inner call has only positional arguments.
-a(b(c(d), e(f)), g(h), i(j));
+>>> Split outer call if there are more than two outer and inner named arguments.
+outer(a: inner(b: b, c: c));
 <<<
-a(b(c(d), e(f)), g(h), i(j));
+outer(
+  a: inner(b: b, c: c),
+);
+>>> Count deeper nested calls.
+outer(a: inner(b: b, nest(c: c)));
+<<<
+outer(
+  a: inner(b: b, nest(c: c)),
+);
+>>> Split outer call on indirect inner call.
+outer(name: !(inner(x: x, y: y)));
+<<<
+outer(
+  name: !(inner(x: x, y: y)),
+);
 >>> Split outer `new` expression.
-new Outer(name: inner(name: value));
+new Outer(name: inner(x: x, y: y));
 <<<
 new Outer(
-  name: inner(name: value),
+  name: inner(x: x, y: y),
 );
 >>> Split outer `const` expression.
-const Outer(name: inner(name: value));
+const Outer(name: inner(x: x, y: y));
 <<<
 const Outer(
-  name: inner(name: value),
+  name: inner(x: x, y: y),
 );
 >>> Split on inner `new` expression.
-outer(name: new Inner(name: value));
+outer(name: new Inner(x: x, y: y));
 <<<
 outer(
-  name: new Inner(name: value),
+  name: new Inner(x: x, y: y),
 );
 >>> Split on inner `const` expression.
-outer(name: const Inner(name: value));
+outer(name: const Inner(x: x, y: y));
 <<<
 outer(
-  name: const Inner(name: value),
+  name: const Inner(x: x, y: y),
 );
->>> Don't split outer call if inner named argument is trivial expression.
+>>> Don't count named argument if it's a trivial expression.
 {
-  outer(name: inner(name: 123));
-  outer(name: inner(name: -123));
-  outer(name: inner(name: 12.3));
-  outer(name: inner(name: -12.3));
-  outer(name: inner(name: null));
-  outer(name: inner(name: true));
-  outer(name: inner(name: false));
+  outer(name: inner(x: x, y: 123));
+  outer(name: inner(x: x, y: -123));
+  outer(name: inner(x: x, y: 12.3));
+  outer(name: inner(x: x, y: -12.3));
+  outer(name: inner(x: x, y: null));
+  outer(name: inner(x: x, y: true));
+  outer(name: inner(x: x, y: false));
 }
 <<<
 {
-  outer(name: inner(name: 123));
-  outer(name: inner(name: -123));
-  outer(name: inner(name: 12.3));
-  outer(name: inner(name: -12.3));
-  outer(name: inner(name: null));
-  outer(name: inner(name: true));
-  outer(name: inner(name: false));
+  outer(name: inner(x: x, y: 123));
+  outer(name: inner(x: x, y: -123));
+  outer(name: inner(x: x, y: 12.3));
+  outer(name: inner(x: x, y: -12.3));
+  outer(name: inner(x: x, y: null));
+  outer(name: inner(x: x, y: true));
+  outer(name: inner(x: x, y: false));
 }
 >>> Split on non-trivial expressions.
 ### Edge cases of simple expressions that aren't considered trivial.
 {
-  outer(name: inner(name: 'string'));
-  outer(name: inner(name: (123)));
-  outer(name: inner(name: this));
-  outer(name: inner(name: -(1)));
-  outer(name: inner(name: 1+2));
+  outer(name: inner(x: x, y: 'string'));
+  outer(name: inner(x: x, y: (123)));
+  outer(name: inner(x: x, y: this));
+  outer(name: inner(x: x, y: -(1)));
+  outer(name: inner(x: x, y: 1+2));
 }
 <<<
 {
   outer(
-    name: inner(name: 'string'),
+    name: inner(x: x, y: 'string'),
   );
   outer(
-    name: inner(name: (123)),
+    name: inner(x: x, y: (123)),
   );
   outer(
-    name: inner(name: this),
+    name: inner(x: x, y: this),
   );
   outer(
-    name: inner(name: -(1)),
+    name: inner(x: x, y: -(1)),
   );
   outer(
-    name: inner(name: 1 + 2),
+    name: inner(x: x, y: 1 + 2),
   );
 }
->>> Split named argument collection if arguments split.
+>>> Split named list argument with multiple elements and any named arguments.
 {
-  f(name: [inner(name: value)]);
-  f(name: {inner(name: value)});
-  f(name: {k: inner(name: value)});
-  f(name: (r: inner(name: value)));
-  f(name: (inner(name: value),));
-  f(name: (inner(name: value), 2));
+  // Only one element.
+  f(name: [inner(x: x, y: y)]);
+  // No named arguments.
+  f(name: [a, inner(x, y)]);
+  // Multiple elements and a named argument.
+  f(name: [a, inner(x: x)]);
 }
 <<<
 {
+  // Only one element.
+  f(
+    name: [inner(x: x, y: y)],
+  );
+  // No named arguments.
+  f(name: [a, inner(x, y)]);
+  // Multiple elements and a named argument.
   f(
     name: [
-      inner(name: value),
+      a,
+      inner(x: x),
     ],
   );
+}
+>>> Split named map argument with multiple elements and any named arguments.
+{
+  // Only one element.
+  f(name: {a: inner(x: x, y: y)});
+  // No named arguments.
+  f(name: {a: a, b: inner(x, y)});
+  // Multiple elements and a named argument.
+  f(name: {a: a, b: inner(x: x)});
+}
+<<<
+{
+  // Only one element.
   f(
-    name: {
-      inner(name: value),
-    },
+    name: {a: inner(x: x, y: y)},
   );
+  // No named arguments.
+  f(name: {a: a, b: inner(x, y)});
+  // Multiple elements and a named argument.
   f(
     name: {
-      k: inner(name: value),
+      a: a,
+      b: inner(x: x),
     },
   );
+}
+>>> Split named set argument with multiple elements and any named arguments.
+{
+  // Only one element.
+  f(name: {inner(x: x, y: y)});
+  // No named arguments.
+  f(name: {a, inner(x, y)});
+  // Multiple elements and a named argument.
+  f(name: {a, inner(x: x)});
+}
+<<<
+{
+  // Only one element.
   f(
-    name: (r: inner(name: value)),
-  );
-  f(
-    name: (inner(name: value),),
+    name: {inner(x: x, y: y)},
   );
+  // No named arguments.
+  f(name: {a, inner(x, y)});
+  // Multiple elements and a named argument.
   f(
-    name: (inner(name: value), 2),
+    name: {
+      a,
+      inner(x: x),
+    },
   );
 }
+>>> Don't eagerly split named record arg regardless of contents.
+f(name: (a, inner(x: x, y: y), b: b));
+<<<
+### The outer call splits, but not the record.
+f(
+  name: (a, inner(x: x, y: y), b: b),
+);
 >>> Split when inner call isn't itself named argument.
-outer(inner(name: x), name: y);
+outer(x: x, inner(y: y), z: z);
 <<<
 outer(
-  inner(name: x),
-  name: y,
+  x: x,
+  inner(y: y),
+  z: z,
 );
 >>> Don't force split if the outer call can be block formatted.
-outer(() {;}, name: inner(name: x));
+outer(() {;}, name: inner(x: x, y: y));
 <<<
 outer(() {
   ;
-}, name: inner(name: x));
\ No newline at end of file
+}, name: inner(x: x, y: y));
diff --git a/test/tall/regression/0300/0394.stmt b/test/tall/regression/0300/0394.stmt
index b7afab0f..d7843ffc 100644
--- a/test/tall/regression/0300/0394.stmt
+++ b/test/tall/regression/0300/0394.stmt
@@ -36,9 +36,7 @@ return $.Div(
     ),
 
     $.Footer(
-      inner: [
-        $.P(id: "notes", inner: "${seeds} seeds"),
-      ],
+      inner: [$.P(id: "notes", inner: "${seeds} seeds")],
     ),
   ],
 );
\ No newline at end of file
diff --git a/test/tall/regression/1500/1527.unit b/test/tall/regression/1500/1527.unit
index b181868e..24040046 100644
--- a/test/tall/regression/1500/1527.unit
+++ b/test/tall/regression/1500/1527.unit
@@ -74,9 +74,7 @@ main() {
 <<<
   main() {
     return Scaffold(
-      body: Center(
-        child: AnimatedDigit(value: value % 10),
-      ),
+      body: Center(child: AnimatedDigit(value: value % 10)),
       floatingActionButton: FloatingActionButton(
         onPressed: () {
           setState(() {
diff --git a/test/tall/regression/other/flutter.unit b/test/tall/regression/other/flutter.unit
index e766ba6f..79b51260 100644
--- a/test/tall/regression/other/flutter.unit
+++ b/test/tall/regression/other/flutter.unit
@@ -49,4 +49,45 @@ class C {
     final double trackCenter = offset.dy + size.height / 2.0;
     final double trackLeft = offset.dx + _trackLeft;
   }
+}
+>>> Eager argument list splitting shouldn't be too eager.
+main() {
+  Text('Item 1', style: TextStyle(color: Colors.white));
+}
+<<<
+main() {
+  Text('Item 1', style: TextStyle(color: Colors.white));
+}
+>>> Eager argument list splitting shouldn't be too eager.
+main() {
+  return Container(
+    height: 70,
+    child: Center(child: contents),
+  );
+}
+<<<
+main() {
+  return Container(height: 70, child: Center(child: contents));
+}
+>>> Eager argument list splitting shouldn't be too eager.
+main() {
+  Stack(
+    children: <Matcher>[
+      matchesSemantics(label: 'inner'),
+    ],
+  );
+}
+<<<
+main() {
+  Stack(children: <Matcher>[matchesSemantics(label: 'inner')]);
+}
+>>> Eager argument list splitting shouldn't be too eager.
+main() {
+  Padding(
+    padding: EdgeInsets.only(top: 20.zR),
+  );
+}
+<<<
+main() {
+  Padding(padding: EdgeInsets.only(top: 20.zR));
 }
\ No newline at end of file