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 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 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: [ @@ -118,8 +127,14 @@ class ExpressionContents { // TabBar( // tabs: [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: [ + matchesSemantics(label: 'inner'), + ], + ); +} +<<< +main() { + Stack(children: [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