Skip to content

Commit 5a93630

Browse files
munificentmunificent
authored
Tweak the heuristics on which expressions allow block formatting. (#1408)
Tweak the heuristics on which expressions allow block formatting. I was trying out the new formatting on Flutter and noticed a few issues: 1. Immediately-invoked functions in asserts are common. Prior to this change, these were not block format candidates, giving you: ```dart assert( () { // Some slow computation... }(), 'Message.', ); ``` With this change, they're treated like (uninvoked) function expressions: ```dart assert(() { // Some slow computation... }(), 'Message.'); ``` 2. Formatting large deep widget trees looks weird. Prior to this change, a function call inside an argument list can be treated like a block argument even if there are other arguments. That tends to pack things in strangely in widget code, like: ```dart 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)), ))], )); ``` With this change, a function or method call inside an argument list is only block formatted if there are no other arguments. In other words, you can block format a function call if the outer call is a pure wrapper around the call, but not otherwise. That leads to: ```dart 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, ), ), ), )]), ); ``` I think that looks a lot nicer. 3. Deep, complex argument lists are combinatorially slow. We have an optimization to eagerly force an argument list to split if its contents clearly won't fit. But that optimization ignores the size of a potential block argument since the contents of that can be wrapped across multiple lines without forcing the outer argument list to split. A consequence of that is that big deeply nested function calls end up slow if it happens that each one only has a single block-formattable call. That turns out to be very common in Flutter code where you have a deep chain of nested widgets along with a bunch of other arguments. The above change to disallow block formatting function calls if there are any other arguments conveniently fixes that performance issue in most cases. (Though I expect we will want to do more work here to handle pathological cases.)
1 parent 6b2b31e commit 5a93630

12 files changed

+405
-111
lines changed

‎benchmark/case/flutter_scrollbar_test.expect

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,45 @@
1+
// This test is from a particularly slow to format part of:
2+
//
3+
// flutter/packages/test/material/scrollbar_test.dart.
4+
void main() {
5+
testWidgets(
6+
"Scrollbar doesn't show when scroll the inner scrollable widget",
7+
(WidgetTester tester) async {
8+
await tester.pumpWidget(Directionality(
9+
textDirection: TextDirection.ltr,
10+
child: MediaQuery(
11+
data: const MediaQueryData(),
12+
child: ScrollConfiguration(
13+
behavior: const NoScrollbarBehavior(),
14+
child: Scrollbar(
15+
key: key2,
16+
child: SingleChildScrollView(
17+
key: outerKey,
18+
child: SizedBox(
19+
height: 1000.0,
20+
width: double.infinity,
21+
child: Column(children: <Widget>[Scrollbar(
22+
key: key1,
23+
child: SizedBox(
24+
height: 300.0,
25+
width: double.infinity,
26+
child: SingleChildScrollView(
27+
key: innerKey,
28+
child: const SizedBox(
29+
key: Key('Inner scrollable'),
30+
height: 1000.0,
31+
width: double.infinity,
32+
),
33+
),
34+
),
35+
)]),
36+
),
37+
),
38+
),
39+
),
40+
),
41+
));
42+
},
43+
variant: TargetPlatformVariant.all(),
44+
);
45+
}

‎benchmark/case/flutter_scrollbar_test.expect_short

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
// This test is from a particularly slow to format part of:
2+
//
3+
// flutter/packages/test/material/scrollbar_test.dart.
4+
void main() {
5+
testWidgets("Scrollbar doesn't show when scroll the inner scrollable widget",
6+
(WidgetTester tester) async {
7+
await tester.pumpWidget(
8+
Directionality(
9+
textDirection: TextDirection.ltr,
10+
child: MediaQuery(
11+
data: const MediaQueryData(),
12+
child: ScrollConfiguration(
13+
behavior: const NoScrollbarBehavior(),
14+
child: Scrollbar(
15+
key: key2,
16+
child: SingleChildScrollView(
17+
key: outerKey,
18+
child: SizedBox(
19+
height: 1000.0,
20+
width: double.infinity,
21+
child: Column(
22+
children: <Widget>[
23+
Scrollbar(
24+
key: key1,
25+
child: SizedBox(
26+
height: 300.0,
27+
width: double.infinity,
28+
child: SingleChildScrollView(
29+
key: innerKey,
30+
child: const SizedBox(
31+
key: Key('Inner scrollable'),
32+
height: 1000.0,
33+
width: double.infinity,
34+
),
35+
),
36+
),
37+
),
38+
],
39+
),
40+
),
41+
),
42+
),
43+
),
44+
),
45+
),
46+
);
47+
}, variant: TargetPlatformVariant.all());
48+
}

‎benchmark/case/flutter_scrollbar_test.unit

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
// This test is from a particularly slow to format part of:
2+
//
3+
// flutter/packages/test/material/scrollbar_test.dart.
4+
void main() {
5+
testWidgets("Scrollbar doesn't show when scroll the inner scrollable widget", (WidgetTester tester) async {
6+
await tester.pumpWidget(
7+
Directionality(textDirection: TextDirection.ltr,
8+
child: MediaQuery(
9+
data: const MediaQueryData(),
10+
child: ScrollConfiguration(
11+
behavior: const NoScrollbarBehavior(),
12+
child: Scrollbar(
13+
key: key2,
14+
child: SingleChildScrollView(
15+
key: outerKey,
16+
child: SizedBox(
17+
height: 1000.0,
18+
width: double.infinity,
19+
child: Column(
20+
children: <Widget>[
21+
Scrollbar(
22+
key: key1,
23+
child: SizedBox(
24+
height: 300.0,
25+
width: double.infinity,
26+
child: SingleChildScrollView(
27+
key: innerKey,
28+
child: const SizedBox(
29+
key: Key('Inner scrollable'),
30+
height: 1000.0,
31+
width: double.infinity,
32+
),
33+
),
34+
),
35+
),
36+
],
37+
),
38+
),
39+
),
40+
),
41+
),
42+
),
43+
),
44+
);
45+
}, variant: TargetPlatformVariant.all());
46+
}

‎benchmark/case/large.expect

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -325,10 +325,10 @@ class BacktrackingSolver {
325325
javaBooleanAnd(
326326
javaBooleanAnd(
327327
javaBooleanAnd(
328-
javaBooleanAnd(javaBooleanAnd(), _isEqualTokens(
329-
node.period,
330-
toNode.period,
331-
)),
328+
javaBooleanAnd(
329+
javaBooleanAnd(),
330+
_isEqualTokens(node.period, toNode.period),
331+
),
332332
_isEqualNodes(node.name, toNode.name),
333333
),
334334
_isEqualNodes(node.parameters, toNode.parameters),

‎lib/src/ast_extensions.dart

Lines changed: 44 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,8 @@
44
import 'package:analyzer/dart/ast/ast.dart';
55
import 'package:analyzer/dart/ast/token.dart';
66

7+
import 'piece/list.dart';
8+
79
extension AstNodeExtensions on AstNode {
810
/// The first token at the beginning of this AST node, not including any
911
/// tokens for leading doc comments.
@@ -149,58 +151,66 @@ extension ExpressionExtensions on Expression {
149151
///
150152
/// Completely empty delimited constructs like `[]` and `foo()` don't allow
151153
/// splitting inside them, so are not considered block-like.
152-
bool get canBlockSplit {
153-
// Unwrap named expressions to get the real expression inside.
154-
var expression = this;
155-
if (expression is NamedExpression) {
156-
expression = expression.expression;
157-
}
154+
bool get canBlockSplit => blockFormatType != BlockFormat.none;
155+
156+
/// When this expression is in an argument list, what kind of block formatting
157+
/// category it belongs to.
158+
BlockFormat get blockFormatType {
159+
return switch (this) {
160+
// Unwrap named expressions to get the real expression inside.
161+
NamedExpression(:var expression) => expression.blockFormatType,
158162

159-
// TODO(tall): Consider whether immediately-invoked function expressions
160-
// should be block argument candidates, like:
161-
//
162-
// function(() {
163-
// body;
164-
// }());
165-
return switch (expression) {
166163
// Allow the target of a single-section cascade to be block formatted.
167-
CascadeExpression(:var target, :var cascadeSections) =>
168-
cascadeSections.length == 1 && target.canBlockSplit,
164+
CascadeExpression(:var target, :var cascadeSections)
165+
when cascadeSections.length == 1 && target.canBlockSplit =>
166+
BlockFormat.invocation,
169167

170168
// A function expression can use either a non-empty parameter list or a
171169
// non-empty block body for block formatting.
172-
FunctionExpression(:var parameters?, :var body) =>
173-
parameters.parameters.canSplit(parameters.rightParenthesis) ||
174-
(body is BlockFunctionBody &&
175-
body.block.statements.canSplit(body.block.rightBracket)),
170+
FunctionExpression(:var parameters?, :var body)
171+
when parameters.parameters.canSplit(parameters.rightParenthesis) ||
172+
(body is BlockFunctionBody &&
173+
body.block.statements.canSplit(body.block.rightBracket)) =>
174+
BlockFormat.function,
175+
176+
// An immediately invoked function expression is formatted like a
177+
// function expression.
178+
FunctionExpressionInvocation(:FunctionExpression function)
179+
when function.blockFormatType == BlockFormat.function =>
180+
BlockFormat.function,
176181

177182
// Non-empty collection literals can block split.
178183
ListLiteral(:var elements, :var rightBracket) ||
179-
SetOrMapLiteral(:var elements, :var rightBracket) =>
180-
elements.canSplit(rightBracket),
181-
RecordLiteral(:var fields, :var rightParenthesis) =>
182-
fields.canSplit(rightParenthesis),
183-
SwitchExpression(:var cases, :var rightBracket) =>
184-
cases.canSplit(rightBracket),
184+
SetOrMapLiteral(:var elements, :var rightBracket)
185+
when elements.canSplit(rightBracket) =>
186+
BlockFormat.collection,
187+
RecordLiteral(:var fields, :var rightParenthesis)
188+
when fields.canSplit(rightParenthesis) =>
189+
BlockFormat.collection,
190+
SwitchExpression(:var cases, :var rightBracket)
191+
when cases.canSplit(rightBracket) =>
192+
BlockFormat.collection,
185193

186194
// Function calls can block split if their argument lists can.
187195
InstanceCreationExpression(:var argumentList) ||
188-
MethodInvocation(:var argumentList) =>
189-
argumentList.arguments.canSplit(argumentList.rightParenthesis),
196+
MethodInvocation(:var argumentList)
197+
when argumentList.arguments.canSplit(argumentList.rightParenthesis) =>
198+
BlockFormat.invocation,
190199

191200
// Note: Using a separate case instead of `||` for this type because
192201
// Dart 3.0 reports an error that [argumentList] has a different type
193202
// here than in the previous two clauses.
194-
FunctionExpressionInvocation(:var argumentList) =>
195-
argumentList.arguments.canSplit(argumentList.rightParenthesis),
203+
FunctionExpressionInvocation(:var argumentList)
204+
when argumentList.arguments.canSplit(argumentList.rightParenthesis) =>
205+
BlockFormat.invocation,
196206

197207
// Multi-line strings can.
198-
StringInterpolation(isMultiline: true) => true,
199-
SimpleStringLiteral(isMultiline: true) => true,
208+
StringInterpolation(isMultiline: true) => BlockFormat.collection,
209+
SimpleStringLiteral(isMultiline: true) => BlockFormat.collection,
200210

201-
// Parenthesized expressions can if the inner one can.
202-
ParenthesizedExpression(:var expression) => expression.canBlockSplit,
203-
_ => false,
211+
// Parenthesized expressions unwrap the inner expression.
212+
ParenthesizedExpression(:var expression) => expression.blockFormatType,
213+
_ => BlockFormat.none,
204214
};
205215
}
206216

‎lib/src/front_end/delimited_list_builder.dart

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -51,7 +51,17 @@ class DelimitedListBuilder {
5151

5252
/// Creates the final [ListPiece] out of the added brackets, delimiters,
5353
/// elements, and style.
54-
ListPiece build() {
54+
Piece build() {
55+
// To simplify the piece tree, if there are no elements, just return the
56+
// brackets concatenated together. We don't have to worry about comments
57+
// here since they would be in the [_elements] list if there were any.
58+
if (_elements.isEmpty) {
59+
return _visitor.buildPiece((b) {
60+
if (_leftBracket case var bracket?) b.add(bracket);
61+
if (_rightBracket case var bracket?) b.add(bracket);
62+
});
63+
}
64+
5565
_setBlockElementFormatting();
5666

5767
var piece =
@@ -147,9 +157,8 @@ class DelimitedListBuilder {
147157
AdjacentStrings(indentStrings: true) =>
148158
BlockFormat.indentedAdjacentStrings,
149159
AdjacentStrings() => BlockFormat.unindentedAdjacentStrings,
150-
FunctionExpression() when element.canBlockSplit => BlockFormat.function,
151-
Expression() when element.canBlockSplit => BlockFormat.block,
152-
DartPattern() when element.canBlockSplit => BlockFormat.block,
160+
Expression() => element.blockFormatType,
161+
DartPattern() when element.canBlockSplit => BlockFormat.collection,
153162
_ => BlockFormat.none,
154163
};
155164

@@ -411,15 +420,18 @@ class DelimitedListBuilder {
411420
void _setBlockElementFormatting() {
412421
// TODO(tall): These heuristics will probably need some iteration.
413422
var functions = <int>[];
414-
var others = <int>[];
423+
var collections = <int>[];
424+
var invocations = <int>[];
415425
var adjacentStrings = <int>[];
416426

417427
for (var i = 0; i < _elements.length; i++) {
418428
switch (_elements[i].blockFormat) {
419429
case BlockFormat.function:
420430
functions.add(i);
421-
case BlockFormat.block:
422-
others.add(i);
431+
case BlockFormat.collection:
432+
collections.add(i);
433+
case BlockFormat.invocation:
434+
invocations.add(i);
423435
case BlockFormat.indentedAdjacentStrings:
424436
case BlockFormat.unindentedAdjacentStrings:
425437
adjacentStrings.add(i);
@@ -428,7 +440,7 @@ class DelimitedListBuilder {
428440
}
429441
}
430442

431-
switch ((functions, others, adjacentStrings)) {
443+
switch ((functions, collections, invocations, adjacentStrings)) {
432444
// Only allow block formatting in an argument list containing adjacent
433445
// strings when:
434446
//
@@ -441,7 +453,7 @@ class DelimitedListBuilder {
441453
// but little else.
442454
// TODO(tall): We may want to iterate on these heuristics. For now,
443455
// starting with something very narrowly targeted.
444-
case ([1], _, [0]):
456+
case ([1], _, _, [0]):
445457
// The adjacent strings.
446458
_elements[0].allowNewlinesWhenUnsplit = true;
447459
if (_elements[0].blockFormat == BlockFormat.unindentedAdjacentStrings) {
@@ -452,9 +464,15 @@ class DelimitedListBuilder {
452464
_elements[1].allowNewlinesWhenUnsplit = true;
453465

454466
// A function expression takes precedence over other block arguments.
455-
case ([var blockArgument], _, _):
456-
// Otherwise, if there one block argument, it can be block formatted.
457-
case ([], [var blockArgument], _):
467+
case ([var blockArgument], _, _, _):
468+
469+
// A single collection literal can be block formatted even if there are
470+
// other arguments.
471+
case ([], [var blockArgument], _, _):
472+
473+
// A single invocation can be block formatted only when there are no
474+
// other arguments.
475+
case ([], [], [var blockArgument], _) when _elements.length == 1:
458476
_elements[blockArgument].allowNewlinesWhenUnsplit = true;
459477
}
460478

0 commit comments

Comments
 (0)