Skip to content

Commit 30dce1c

Browse files
munificentmunificent
authored
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.
1 parent ab9cbef commit 30dce1c

File tree

10 files changed

+199
-229
lines changed

10 files changed

+199
-229
lines changed

‎benchmark/case/flutter_scrollbar_test.expect

Lines changed: 16 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -19,23 +19,25 @@ void main() {
1919
child: SizedBox(
2020
height: 1000.0,
2121
width: double.infinity,
22-
child: Column(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,
22+
child: Column(
23+
children: <Widget>[
24+
Scrollbar(
25+
key: key1,
26+
child: SizedBox(
27+
height: 300.0,
28+
width: double.infinity,
29+
child: SingleChildScrollView(
30+
key: innerKey,
31+
child: const SizedBox(
32+
key: Key('Inner scrollable'),
33+
height: 1000.0,
34+
width: double.infinity,
35+
),
3436
),
3537
),
3638
),
37-
),
38-
]),
39+
],
40+
),
3941
),
4042
),
4143
),

‎lib/src/front_end/delimited_list_builder.dart

Lines changed: 19 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -444,33 +444,15 @@ class DelimitedListBuilder {
444444
var candidateIndex = _candidateBlockArgument(arguments);
445445
if (candidateIndex == -1) return;
446446

447+
// The block argument must be positional.
448+
if (arguments[candidateIndex] is NamedExpression) return;
449+
447450
// Only allow up to one trailing argument after the block argument. This
448451
// handles the common `tags` and `timeout` named arguments in `test()` and
449452
// `group()` while still mostly having the block argument be at the end of
450453
// the argument list.
451454
if (candidateIndex < arguments.length - 2) return;
452455

453-
// If there are multiple named arguments, they should never end up on
454-
// separate lines (unless the whole argument list fully splits). Otherwise,
455-
// it's too easy for an argument name to get buried in the middle of a line.
456-
// So we look for named arguments before, on, and after the candidate
457-
// argument. If more than one of those sections of arguments has a named
458-
// argument, then we don't allow the block argument.
459-
var namedSections = 0;
460-
bool hasNamedArgument(int from, int to) {
461-
for (var i = from; i < to; i++) {
462-
if (arguments[i] is NamedExpression) return true;
463-
}
464-
465-
return false;
466-
}
467-
468-
if (hasNamedArgument(0, candidateIndex)) namedSections++;
469-
if (hasNamedArgument(candidateIndex, candidateIndex + 1)) namedSections++;
470-
if (hasNamedArgument(candidateIndex + 1, arguments.length)) namedSections++;
471-
472-
if (namedSections > 1) return;
473-
474456
// Edge case: If the first argument is adjacent strings and the second
475457
// argument is a function literal, with optionally a third non-block
476458
// argument, then treat the function as the block argument.
@@ -484,18 +466,15 @@ class DelimitedListBuilder {
484466
// expect(1 + 2, 3);
485467
// });
486468
if (candidateIndex == 1 &&
487-
arguments[0] is! NamedExpression &&
488-
arguments[1] is! NamedExpression) {
489-
if ((arguments[0].blockFormatType, arguments[1].blockFormatType)
490-
case (
491-
BlockFormat.unindentedAdjacentStrings ||
492-
BlockFormat.indentedAdjacentStrings,
493-
BlockFormat.function
494-
)) {
469+
arguments[1].blockFormatType == BlockFormat.function &&
470+
arguments[0] is! NamedExpression) {
471+
var firstArgumentFormatType = arguments[0].blockFormatType;
472+
if (firstArgumentFormatType
473+
case BlockFormat.unindentedAdjacentStrings ||
474+
BlockFormat.indentedAdjacentStrings) {
495475
// The adjacent strings.
496476
_elements[0].allowNewlinesWhenUnsplit = true;
497-
if (arguments[0].blockFormatType ==
498-
BlockFormat.unindentedAdjacentStrings) {
477+
if (firstArgumentFormatType == BlockFormat.unindentedAdjacentStrings) {
499478
_elements[0].indentWhenBlockFormatted = true;
500479
}
501480

@@ -512,11 +491,18 @@ class DelimitedListBuilder {
512491
/// If an argument in [arguments] is a candidate to be block formatted,
513492
/// returns its index.
514493
///
515-
/// Otherwise, returns `-1`.
494+
/// If there is a single non-empty block bodied function expression in
495+
/// [arguments], returns its index. Otherwise, if there is a single non-empty
496+
/// collection literal in [arguments], returns its index. Otherwise, returns
497+
/// `-1`.
516498
int _candidateBlockArgument(List<AstNode> arguments) {
499+
// The index of the function expression argument, or -1 if none has been
500+
// found or -2 if there are multiple.
517501
var functionIndex = -1;
502+
503+
// The index of the collection literal argument, or -1 if none has been
504+
// found or -2 if there are multiple.
518505
var collectionIndex = -1;
519-
// var stringIndex = -1;
520506

521507
for (var i = 0; i < arguments.length; i++) {
522508
// See if it's an expression that supports block formatting.

‎test/tall/invocation/block_argument_named.stmt

Lines changed: 39 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1,66 +1,52 @@
11
40 columns |
22
### Test how named arguments interact with block formatting.
3-
>>> Block format all positional.
4-
function(be, fore, [element, element], after);
5-
<<<
6-
function(be, fore, [
7-
element,
8-
element,
9-
], after);
10-
>>> Block format named only leading.
11-
function(a: be, b: fore, [element, element], after);
12-
<<<
13-
function(a: be, b: fore, [
14-
element,
15-
element,
16-
], after);
17-
>>> Block format named only block argument.
18-
function(be, fore, a: [element, element], after);
19-
<<<
20-
function(be, fore, a: [
21-
element,
22-
element,
23-
], after);
24-
>>> Block format named only trailing.
25-
function(be, fore, [element, element], a: after);
26-
<<<
27-
function(be, fore, [
28-
element,
29-
element,
30-
], a: after);
31-
>>> Don't block format named leading and block argument.
32-
function(a: be, fore, b: [element, element], after);
3+
>>> A function block argument can't be named.
4+
function(name: () {;});
335
<<<
346
function(
35-
a: be,
36-
fore,
37-
b: [element, element],
38-
after,
7+
name: () {
8+
;
9+
},
3910
);
40-
>>> Don't block format named leading and trailing.
41-
function(a: be, fore, [element, element], b: after);
11+
>>> A collection block argument can't be named.
12+
function(name: [element, element, element, element]);
4213
<<<
4314
function(
44-
a: be,
45-
fore,
46-
[element, element],
47-
b: after,
15+
name: [
16+
element,
17+
element,
18+
element,
19+
element,
20+
],
4821
);
49-
>>> Don't block format named block argument and trailing.
50-
function(be, fore, a: [element, element], b: after);
22+
>>> If there are multiple functions, don't block format, even if only one is positional.
23+
function(a: () {;}, () {;});
5124
<<<
5225
function(
53-
be,
54-
fore,
55-
a: [element, element],
56-
b: after,
26+
a: () {
27+
;
28+
},
29+
() {
30+
;
31+
},
5732
);
58-
>>> Don't block format all named.
59-
function(a: be, b: fore, c: [element, element], d: after);
33+
>>> If there are multiple collections, don't block format, even if only one is positional.
34+
function(a: [element], [element, element, element, element, element]);
6035
<<<
6136
function(
62-
a: be,
63-
b: fore,
64-
c: [element, element],
65-
d: after,
66-
);
37+
a: [element],
38+
[
39+
element,
40+
element,
41+
element,
42+
element,
43+
element,
44+
],
45+
);
46+
>>> Other non-block arguments can be named or not.
47+
function(1, a: 2, 3, b: 4, [element, element], c: 5);
48+
<<<
49+
function(1, a: 2, 3, b: 4, [
50+
element,
51+
element,
52+
], c: 5);

‎test/tall/invocation/block_argument_other.stmt

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -34,20 +34,6 @@ function(
3434
element,
3535
],
3636
);
37-
>>> A function block argument can be named.
38-
function(name: () { body; });
39-
<<<
40-
function(name: () {
41-
body;
42-
});
43-
>>> A non-function block argument can be named.
44-
function(name: [element, element, element]);
45-
<<<
46-
function(name: [
47-
element,
48-
element,
49-
element,
50-
]);
5137
>>> A long argument name can prevent block formatting.
5238
veryLongFunctionName(veryLongArgumentName: [element]);
5339
<<<

‎test/tall/regression/0100/0198.stmt

Lines changed: 34 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -24,38 +24,38 @@
2424
});
2525
});
2626
<<<
27-
testThat('backward navigation is disabled when at end of stream', when: (
28-
TaskList taskList,
29-
TaskService taskService,
30-
) {
31-
var cursorPageNo = 0;
32-
final streamCtrl = initCustomTaskServiceMock(
33-
taskService,
34-
canMoveTo: (pageNo) => pageNo < 0 ? false : true,
35-
getCurrentPageNumber: () => cursorPageNo,
36-
);
27+
testThat(
28+
'backward navigation is disabled when at end of stream',
29+
when: (TaskList taskList, TaskService taskService) {
30+
var cursorPageNo = 0;
31+
final streamCtrl = initCustomTaskServiceMock(
32+
taskService,
33+
canMoveTo: (pageNo) => pageNo < 0 ? false : true,
34+
getCurrentPageNumber: () => cursorPageNo,
35+
);
3736

38-
first('attach tasklist', () {
39-
taskList.attach();
40-
addTasks(streamCtrl);
41-
})
42-
.thenExpect(
43-
'pager at page 1',
44-
() => {
45-
taskList.currentPageNo: 1,
46-
taskList.backwardPaginationDisabled: isFalse,
47-
},
48-
)
49-
.then('go to page 2', () {
50-
taskList.nextPage();
51-
addTasks(streamCtrl, count: 1);
52-
cursorPageNo = 1;
53-
})
54-
.thenExpect(
55-
'pager unchanged',
56-
() => {
57-
taskList.currentPageNo: 2,
58-
taskList.backwardPaginationDisabled: isTrue,
59-
},
60-
);
61-
});
37+
first('attach tasklist', () {
38+
taskList.attach();
39+
addTasks(streamCtrl);
40+
})
41+
.thenExpect(
42+
'pager at page 1',
43+
() => {
44+
taskList.currentPageNo: 1,
45+
taskList.backwardPaginationDisabled: isFalse,
46+
},
47+
)
48+
.then('go to page 2', () {
49+
taskList.nextPage();
50+
addTasks(streamCtrl, count: 1);
51+
cursorPageNo = 1;
52+
})
53+
.thenExpect(
54+
'pager unchanged',
55+
() => {
56+
taskList.currentPageNo: 2,
57+
taskList.backwardPaginationDisabled: isTrue,
58+
},
59+
);
60+
},
61+
);

‎test/tall/regression/0300/0394.stmt

Lines changed: 23 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -14,25 +14,27 @@ return $.Div(inner: [
1414
]),
1515
]);
1616
<<<
17-
return $.Div(inner: [
18-
$.Div(
19-
id: "container",
20-
inner: [
21-
$.Canvas(width: maxD, height: maxD, clazz: "center", ref: canvas),
22-
$.Form(
23-
clazz: "center",
24-
inner: [
25-
$.Input(
26-
type: "range",
27-
max: 1000,
28-
value: seeds,
29-
onChange: onSliderChange,
30-
),
31-
],
32-
),
33-
$.Img(src: "math.png", width: "350px", height: "42px", clazz: "center"),
34-
],
35-
),
17+
return $.Div(
18+
inner: [
19+
$.Div(
20+
id: "container",
21+
inner: [
22+
$.Canvas(width: maxD, height: maxD, clazz: "center", ref: canvas),
23+
$.Form(
24+
clazz: "center",
25+
inner: [
26+
$.Input(
27+
type: "range",
28+
max: 1000,
29+
value: seeds,
30+
onChange: onSliderChange,
31+
),
32+
],
33+
),
34+
$.Img(src: "math.png", width: "350px", height: "42px", clazz: "center"),
35+
],
36+
),
3637

37-
$.Footer(inner: [$.P(id: "notes", inner: "${seeds} seeds")]),
38-
]);
38+
$.Footer(inner: [$.P(id: "notes", inner: "${seeds} seeds")]),
39+
],
40+
);

0 commit comments

Comments
 (0)