When the RHS of => is a function call, prefer to split at the =>.

[munificent]

We use the same AssignPiece rule for =, =>, and :. And that rule uniformly handles all kinds of block-formattable right hand sides: collection literals, switch expressions, and function calls. For the most part, that works well and provides nice consistent formatting. Users generally like:

// Better:
variable = [
  long,
  list,
  literal,
];

// Worse:
variable =
    [long, list, literal];

And:

// Better:
SomeWidget(
  children: [
    long,
    list,
    literal,
  ],
);

// Worse:
SomeWidget(
  children:
      [long, list, literal],
);

And (with somewhat less consensus):

// Better:
variable = function(
  long,
  argument,
  list,
);

// Worse:
variable =
    function(long, argument, list);

Also, users have long requested and seem to like:

// Better:
class C {
  List<String> makeStuff() => [
    long,
    list,
    literal,
  ];
}

// Worse:
class C {
  List<String> makeStuff() =>
      [long, list, literal];
}

So based on all that, I just used uniform rules for all combinations of assignment constructs and delimited constructs. That means that this behavior falls out implicitly:

// Better:
class C {
  String doThing() => function(
    long,
    argument,
    list,
  );
}

// Worse:
class C {
  String doThing() =>
      function(long, argument, list);
}

But it turns out that that particular combination of => with a function call on the right doesn't get the same user sentiment. Instead, most (including me) prefer:

class C {
  String doThing() =>
      function(long, argument, list);
}

I think it's because this keeps the function name next to its arguments. With the other block-like forms: list literals, etc. there isn't really anything particularly interesting going on in the opening delimiter, so it makes sense to keep it on the same line as the => since it's pretty much just punctuation. But a function call is a single coherent operation including the function and its arguments.

So this PR tweaks the cost rule for AssignPiece. When the operator is => and the RHS is a function call, we prefer to split at the => if that lets the RHS stay unsplit.

[Levi-Lesches]

...if that lets the RHS stay unsplit.

I think this is definitely the key part. There's a big difference between:

  void veryLongFuncName1() => 
    veryLongFuncName2(param1, param2, param3);

and

void veryLongFuncName1() => veryLongFuncName(
  expression1 + expression2 * 3,
  expression3.toSomethingElse(),
  optionalParam: expression4 as MyType,
)

In other words, sometimes the function call is one cohesive unit, like a small function and a few simple arguments, and sometimes the function call is better thought of as a list of arguments that can grow over time, which would be very common in widget constructors.

[FMorschel]

I saw this sample:

// Better:
SomeWidget(
  children: [
    long,
    list,
    literal,
  ],
);

// Worse:
SomeWidget(
  children:
      [long, list, literal],
);

I'm not sure how to test this myself so, what about cases like:

SomeWidget(
  children: [
    for (final o in objects) o,
  ],
);

SomeWidget(children: [
  for (final o in objects) o,
]);

I personally like the second one a little better. I guess what I'm trying to ask is:

What about cases where the parameter starts at the first line and has multiple lines (not sure what to call them to differentiate from the first sample on this comment) like that or a () {} in tests for example? Will they all be extended like the above?

---

Edit

Also cases like:

Today:

final someMap = {...};
final mapList = []; // Say from json
final foo = [
  ...mapList
      .cast<Map<String, dynamic>()
      .map(
        (innerMap) => innerMap
          ..addAll({
            'something': someMap['something'],
            //...
          }),
      )
];

Personally I prefer:

final someMap = {...};
final mapList = []; // Say from json
final foo = [
  ...mapList
      .cast<Map<String, dynamic>()
      .map((innerMap) => innerMap..addAll({
        'something': someMap['something'],
        //...
      })),
];

[munificent]

I'm not sure how to test this myself so, what about cases like:

SomeWidget(
  children: [
    for (final o in objects) o,
  ],
);

SomeWidget(children: [
  for (final o in objects) o,
]);

I personally like the second one a little better.

Yes, it will give you the second one. The question of how to split within an argument list also has some subtlety and is pretty much orthogonal to this change. But in the example here, yes, the formatter does what it calls "block formatting" for the argument list.

Today:

final someMap = {...};
final mapList = []; // Say from json
final foo = [
  ...mapList
      .cast<Map<String, dynamic>()
      .map(
        (innerMap) => innerMap
          ..addAll({
            'something': someMap['something'],
            //...
          }),
      )
];

Personally I prefer:

final someMap = {...};
final mapList = []; // Say from json
final foo = [
  ...mapList
      .cast<Map<String, dynamic>()
      .map((innerMap) => innerMap..addAll({
        'something': someMap['something'],
        //...
      })),
];

That's definitely separate from this change. :)

You've got a collection literal containing a spread containing a method chain containing a function expression containing a cascade containing a map literal. That is quite a lot to pack into a single expression and I think the taller output is obviously a little more spread out but is easier to figure out what's going on. Also, the formatting rules that lead to the current output are much simpler than the rules that would be required to get your desired output and simpler rules have some value too.

[FMorschel]

That is quite a lot to pack into a single expression and I think the taller output is obviously a little more spread out but is easier to figure out what's going on.

Sorry if I was not clear enough For this change my question was for this specific part:

.map((innerMap) => innerMap..addAll({
  'something': someMap['something'],
  //...
}))

But I guess cascades were not in the scope of this change so my bad! Is there any specific issue/PR I should look for this? Or should I go to the main one (#1253)?

[munificent]

For this change my question was for this specific part:

.map((innerMap) => innerMap..addAll({
  'something': someMap['something'],
  //...
}))

But I guess cascades were not in the scope of this change so my bad! Is there any specific issue/PR I should look for this? Or should I go to the main one (#1253)?

Best would be to file a separate issue for that, thanks!