Skip to content

Commit 71421b3

Browse files
munificentmunificent
authored
Allow shape to propagate through AssignPieces. (#1694)
Consider: ```dart function(name: (param) => another(argument1, argument2, argument3)); ``` Here we have a ListPiece for the `another(...)` call wrapped in an AssignPiece for the `=>` wrapped in an AssignPiece for the `name:`. When the inner ListPiece splits, it will be block-shaped. That in turn allows the surrounding AssignPiece to block format, giving: ```dart (param) => another( argument1, argument2, argument3, ) ``` (If the ListPiece wasn't block-shaped, then the formatter would split after `=>`.) An interesting question is then "what is the shape of that AssignPiece?" Prior to this PR, the answer was always "other". Regardless of the shape of the AssignPiece's RHS, a split AssignPiece always had shape other. That meant that the entire original example would end up like: ```dart function( name: (param) => another( argument1, argument2, argument3, ), ); ``` Because the `(param) => ...` piece had shape "other", the AssignPiece for the named argument couldn't block format it, so it would split after the `:`. That's not ideal. In practice with `=` and espectially with named arguments, users really want code on the same line as the argument name (and less indented) if at all possible. This PR propagates the shape of the AssignPiece's contents out to its surrounding context. This means that a headline-shaped RHS causes the assignment to be headline-shaped, and likewise for block-shaped. That lets the above example be formatted as: ```dart function( name: (param) => another( argument1, argument2, argument3, ), ); ``` Since `=`, `:`, and `=>` all use AssignPiece, this theoretically means that arbitrarily complex chains of those propagate the shape out. For example, chained assignments like this now work: ```dart outer = inner = [ element, element, ]; ``` In practice, 99% of the time this comes into play, it's a named argument whose expression is a `=>` function. But those are very common for callbacks in Flutter code, so this makes a big difference. Fix #1536. Fix #1545. Fix #1668. Fix #1679.
1 parent 8a17214 commit 71421b3

File tree

25 files changed

+629
-144
lines changed

25 files changed

+629
-144
lines changed

‎CHANGELOG.md‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,41 @@
22

33
* Format null-aware elements.
44

5+
* Allow more code on the same line as a named argument or `=>` (#1536, #1545,
6+
#1668, #1679).
7+
8+
```dart
9+
// Before:
10+
function(
11+
name:
12+
(param, another) =>
13+
veryLongBody,
14+
);
15+
16+
function(
17+
name:
18+
(param) => another(
19+
argument1,
20+
argument2,
21+
argument3,
22+
),
23+
);
24+
25+
// After:
26+
function(
27+
name: (param, another) =>
28+
veryLongBody,
29+
);
30+
31+
function(
32+
name: (param) => another(
33+
argument1,
34+
argument2,
35+
argument3,
36+
),
37+
);
38+
```
39+
540
* Avoid splitting chains containing only properties.
641

742
```dart

‎lib/src/piece/assign.dart‎

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -124,23 +124,29 @@ final class AssignPiece extends Piece {
124124

125125
@override
126126
void format(CodeWriter writer, State state) {
127-
writer.setShapeMode(ShapeMode.other);
128-
129-
// When splitting at the operator, both operands may split or not and
130-
// will be indented if they do.
131-
if (state == State.split) writer.pushIndent(Indent.expression);
132-
133-
writer.format(_left);
134-
writer.splitIf(state == State.split);
135-
136127
if (state == State.split) {
128+
// When splitting at the operator, indent the operands.
129+
writer.pushIndent(Indent.expression);
130+
131+
// Treat a split `=` as potentially headline-shaped if the LHS doesn't
132+
// split. Allows:
133+
//
134+
// variable = another =
135+
// 'split at second "="';
136+
writer.setShapeMode(ShapeMode.beforeHeadline);
137+
writer.format(_left);
138+
writer.setShapeMode(ShapeMode.afterHeadline);
139+
140+
writer.newline();
137141
writer.popIndent();
138142
writer.pushIndent(Indent.assignment);
143+
writer.format(_right);
144+
writer.popIndent();
145+
} else {
146+
writer.format(_left);
147+
writer.space();
148+
writer.format(_right);
139149
}
140-
141-
writer.format(_right);
142-
143-
if (state == State.split) writer.popIndent();
144150
}
145151

146152
@override

‎lib/src/piece/chain.dart‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,22 @@ final class ChainPiece extends Piece {
202202
writer.popIndent();
203203

204204
case _blockFormatTrailingCall:
205+
// Don't treat a cascade as block-shaped in the surrounding context
206+
// even if it block splits. Prefer:
207+
//
208+
// variable = target
209+
// ..cascade(argument);
210+
//
211+
// Over:
212+
//
213+
// variable = target..cascade(
214+
// argument,
215+
// );
216+
//
217+
// Note how the former makes it clearer that `variable` will be assigned
218+
// the value `target` and that the cascade is a secondary side-effect.
219+
if (_isCascade) writer.setShapeMode(ShapeMode.other);
220+
205221
writer.format(_target);
206222

207223
for (var i = 0; i < _calls.length; i++) {

‎test/tall/expression/assignment.stmt‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -79,16 +79,15 @@ target
7979
.another
8080
.lastOneReallyLong =
8181
reallyLongValue;
82-
>>> Don't block format through nested assignments.
82+
>>> Allow block formatting through nested assignments.
8383
outer = inner = [element1, element2, element3, element4];
8484
<<<
85-
outer =
86-
inner = [
87-
element1,
88-
element2,
89-
element3,
90-
element4,
91-
];
85+
outer = inner = [
86+
element1,
87+
element2,
88+
element3,
89+
element4,
90+
];
9291
>>> Headline format unsplit target of call chain.
9392
variable = (tar + get).method().another().third();
9493
<<<

‎test/tall/invocation/cascade_mixed.stmt‎

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,4 +40,18 @@ object
4040
>>> Chain with index target.
4141
object..[index].method();
4242
<<<
43-
object..[index].method();
43+
object..[index].method();
44+
>>> Don't treat cascade as block-shaped in assignment.
45+
variable = target..method(argument1, argument2);
46+
<<<
47+
variable = target
48+
..method(argument1, argument2);
49+
>>> Don't treat cascade as block-shaped even if the argument list splits.
50+
variable = target..method(argument1, argument2, argument3);
51+
<<<
52+
variable = target
53+
..method(
54+
argument1,
55+
argument2,
56+
argument3,
57+
);

‎test/tall/invocation/named_argument.stmt‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -73,3 +73,20 @@ function(
7373
body;
7474
},
7575
);
76+
>>> Headline formatting for a split `=>` function.
77+
function(name: (param, another) => veryLongBody);
78+
<<<
79+
function(
80+
name: (param, another) =>
81+
veryLongBody,
82+
);
83+
>>> Block-like formatting of a `=>` body containing a function call.
84+
function(name: (param) => another(argument1, argument2, argument3));
85+
<<<
86+
function(
87+
name: (param) => another(
88+
argument1,
89+
argument2,
90+
argument3,
91+
),
92+
);

‎test/tall/regression/0000/0040.stmt‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,8 +14,9 @@
1414
"e"
1515
]);
1616
<<<
17-
var s = new Serialization()..addRuleFor(
18-
Various,
19-
constructor: "Foo",
20-
constructorFields: ["d", "e"],
21-
);
17+
var s = new Serialization()
18+
..addRuleFor(
19+
Various,
20+
constructor: "Foo",
21+
constructorFields: ["d", "e"],
22+
);

‎test/tall/regression/0000/0056.stmt‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
>>> (indent 8)
22
_sources[uri] = src = new _MockSdkSource(uri, 'library dart.${uri.path};');
33
<<<
4-
_sources[uri] =
5-
src = new _MockSdkSource(uri, 'library dart.${uri.path};');
4+
_sources[uri] = src = new _MockSdkSource(
5+
uri,
6+
'library dart.${uri.path};',
7+
);

‎test/tall/regression/0100/0158.unit‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,8 +10,10 @@ void foo() {
1010
void foo() {
1111
if (bar) {
1212
if (baz) {
13-
_sources[uri] =
14-
src = new _MockSdkSource(uri, 'library dart.${uri.path};');
13+
_sources[uri] = src = new _MockSdkSource(
14+
uri,
15+
'library dart.${uri.path};',
16+
);
1517
}
1618
}
1719
}

‎test/tall/regression/0200/0221.unit‎

Lines changed: 3 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,9 @@ class Foo {
1313
static Column column(Handler onSelection) =>
1414
(Column.defaultBuilder(videoMsg())
1515
..id = 'VIDEO'
16-
..segment =
17-
((row) => row.segmentedStats
18-
.map((s) => s.get(Stats.SEGMENTATION))
19-
.toList())
16+
..segment = ((row) => row.segmentedStats
17+
.map((s) => s.get(Stats.SEGMENTATION))
18+
.toList())
2019
..cell = new Cell(onSelection))
2120
.build();
2221
}

0 commit comments

Comments
 (0)