Skip to content

Commit 15f032a

Browse files
authored
Format cascades. (#1365)
Format cascades. There were a couple of tricky corners, but this wasn't as bad as I'd feared. Most of ChainBuilder worked great for it. In the process of doing this and migrating the tests, I noticed another kind of expression that should allow block formatting in an argument list: function expression invocations. That's a `(...)` where the thing on the left is some kind of expression and not simply a name, like: `(foo + bar)(args)`. Added support for that and added some tests of block formatting for those too.
1 parent 633b01c commit 15f032a

File tree

9 files changed

+498
-41
lines changed

9 files changed

+498
-41
lines changed

lib/src/ast_extensions.dart

Lines changed: 31 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,10 @@ extension ExpressionExtensions on Expression {
163163
// body;
164164
// }());
165165
return switch (expression) {
166+
// Allow the target of a single-section cascade to be block formatted.
167+
CascadeExpression(:var target, :var cascadeSections) =>
168+
cascadeSections.length == 1 && target.canBlockSplit,
169+
166170
// A function expression can use either a non-empty parameter list or a
167171
// non-empty block body for block formatting.
168172
FunctionExpression(:var parameters?, :var body) =>
@@ -184,6 +188,12 @@ extension ExpressionExtensions on Expression {
184188
MethodInvocation(:var argumentList) =>
185189
argumentList.arguments.canSplit(argumentList.rightParenthesis),
186190

191+
// Note: Using a separate case instead of `||` for this type because
192+
// Dart 3.0 reports an error that [argumentList] has a different type
193+
// here than in the previous two clauses.
194+
FunctionExpressionInvocation(:var argumentList) =>
195+
argumentList.arguments.canSplit(argumentList.rightParenthesis),
196+
187197
// Multi-line strings can.
188198
StringInterpolation(isMultiline: true) => true,
189199
SimpleStringLiteral(isMultiline: true) => true,
@@ -292,26 +302,27 @@ extension ExpressionExtensions on Expression {
292302
}
293303

294304
extension CascadeExpressionExtensions on CascadeExpression {
295-
/// Whether a cascade should be allowed to be inline as opposed to moving the
296-
/// section to the next line.
297-
bool get allowInline {
298-
// Cascades with multiple sections are handled elsewhere and are never
299-
// inline.
300-
assert(cascadeSections.length == 1);
301-
302-
// If the receiver is an expression that makes the cascade's very low
303-
// precedence confusing, force it to split. For example:
304-
//
305-
// a ? b : c..d();
306-
//
307-
// Here, the cascade is applied to the result of the conditional, not "c".
308-
if (target is ConditionalExpression) return false;
309-
if (target is BinaryExpression) return false;
310-
if (target is PrefixExpression) return false;
311-
if (target is AwaitExpression) return false;
312-
313-
return true;
314-
}
305+
/// Whether a cascade should be allowed to be inline with the target as
306+
/// opposed to moving the sections to the next line.
307+
bool get allowInline => switch (target) {
308+
// Cascades with multiple sections always split.
309+
_ when cascadeSections.length > 1 => false,
310+
311+
// If the receiver is an expression that makes the cascade's very low
312+
// precedence confusing, force it to split. For example:
313+
//
314+
// a ? b : c..d();
315+
//
316+
// Here, the cascade is applied to the result of the conditional, not
317+
// just "c".
318+
ConditionalExpression() => false,
319+
BinaryExpression() => false,
320+
PrefixExpression() => false,
321+
AwaitExpression() => false,
322+
323+
// Otherwise, the target doesn't force a split.
324+
_ => true,
325+
};
315326
}
316327

317328
extension AdjacentStringsExtensions on AdjacentStrings {

lib/src/front_end/ast_node_visitor.dart

Lines changed: 9 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -218,7 +218,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
218218

219219
@override
220220
Piece visitCascadeExpression(CascadeExpression node) {
221-
throw UnimplementedError();
221+
return ChainBuilder(this, node).build();
222222
}
223223

224224
@override
@@ -1399,6 +1399,14 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
13991399

14001400
@override
14011401
Piece visitPropertyAccess(PropertyAccess node) {
1402+
// If there's no target, this is a section in a cascade.
1403+
if (node.target == null) {
1404+
return buildPiece((b) {
1405+
b.token(node.operator);
1406+
b.visit(node.propertyName);
1407+
});
1408+
}
1409+
14021410
return ChainBuilder(this, node).build();
14031411
}
14041412

lib/src/front_end/chain_builder.dart

Lines changed: 99 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import 'package:analyzer/dart/ast/ast.dart';
66
import 'package:analyzer/dart/ast/token.dart';
77

88
import '../ast_extensions.dart';
9+
import '../constants.dart';
910
import '../piece/chain.dart';
1011
import '../piece/piece.dart';
1112
import 'piece_factory.dart';
@@ -42,6 +43,13 @@ import 'piece_factory.dart';
4243
class ChainBuilder {
4344
final PieceFactory _visitor;
4445

46+
/// The outermost expression being converted to a chain.
47+
///
48+
/// If it's a [CascadeExpression], then the chain is the cascade sections.
49+
/// Otherwise, it's some kind of method call or property access and the chain
50+
/// is the nested series of selector subexpressions.
51+
final Expression _root;
52+
4553
/// The left-most target of the chain.
4654
late Piece _target;
4755

@@ -60,11 +68,38 @@ class ChainBuilder {
6068
/// The dotted property accesses and method calls following the target.
6169
final List<ChainCall> _calls = [];
6270

63-
ChainBuilder(this._visitor, Expression expression) {
64-
_unwrapCall(expression);
71+
ChainBuilder(this._visitor, this._root) {
72+
if (_root case CascadeExpression cascade) {
73+
_visitTarget(cascade.target);
74+
75+
for (var section in cascade.cascadeSections) {
76+
_unwrapCall(section);
77+
}
78+
} else {
79+
_unwrapCall(_root);
80+
}
6581
}
6682

6783
Piece build() {
84+
if (_root case CascadeExpression cascade) {
85+
// If there is only a single section and it can block split, allow it:
86+
//
87+
// target..cascade(
88+
// argument,
89+
// );
90+
var blockCallIndex =
91+
_calls.length == 1 && _calls.single.canSplit ? 0 : -1;
92+
93+
var chain = ChainPiece(_target, _calls,
94+
indent: Indent.cascade,
95+
blockCallIndex: blockCallIndex,
96+
allowSplitInTarget: _allowSplitInTarget);
97+
98+
if (!cascade.allowInline) chain.pin(State.split);
99+
100+
return chain;
101+
}
102+
68103
// If there are no calls, there's no chain.
69104
if (_calls.isEmpty) return _target;
70105

@@ -94,14 +129,43 @@ class ChainBuilder {
94129
_ => -1,
95130
};
96131

97-
return ChainPiece(_target, _calls, leadingProperties, blockCallIndex,
132+
// If a method chain appears as the target of a cascade, then we only
133+
// indent the method chain +2. That way, with the cascade's own +2, the
134+
// result is a total of +4. This looks more natural than indenting the
135+
// method chain +4 relative to the cascade's +2:
136+
//
137+
// // Bad:
138+
// object
139+
// .method()
140+
// .method()
141+
// ..x = 1
142+
// ..y = 2;
143+
//
144+
// // Better:
145+
// object
146+
// .method()
147+
// .method()
148+
// ..x = 1
149+
// ..y = 2;
150+
var indent =
151+
_root.parent is CascadeExpression ? Indent.cascade : Indent.expression;
152+
153+
return ChainPiece(_target, _calls,
154+
indent: indent,
155+
leadingProperties: leadingProperties,
156+
blockCallIndex: blockCallIndex,
98157
allowSplitInTarget: _allowSplitInTarget);
99158
}
100159

101-
/// Given [expression], which is the outermost expression for some call chain,
102-
/// recursively traverses the selectors to fill in the list of [_calls].
160+
/// Given [expression], which is the expression for some call chain, traverses
161+
/// the selectors to fill in the list of [_calls].
162+
///
163+
/// If [_root] is a [CascadeSection], then this is called once for each
164+
/// section in the cascade.
103165
///
104-
/// Initializes [_target] with the innermost subexpression that isn't a part
166+
/// Otherwise, it's a method chain, and this recursively calls itself for the
167+
/// targets to unzip and flatten the nested selector expressions. Then it
168+
/// initializes [_target] with the innermost subexpression that isn't a part
105169
/// of the call chain. For example, given:
106170
///
107171
/// foo.bar()!.baz[0][1].bang()
@@ -112,16 +176,23 @@ class ChainBuilder {
112176
/// .baz[0][1]
113177
/// .bang()
114178
void _unwrapCall(Expression expression) {
179+
var isCascade = _root is CascadeExpression;
180+
115181
switch (expression) {
116-
case Expression(looksLikeStaticCall: true):
182+
case Expression(looksLikeStaticCall: true) when !isCascade:
117183
// Don't include things that look like static method or constructor
118184
// calls in the call chain because that tends to split up named
119185
// constructors from their class.
120186
_visitTarget(expression);
121187

122188
// Selectors.
123-
case MethodInvocation(:var target?):
124-
_unwrapCall(target);
189+
case AssignmentExpression():
190+
var piece = _visitor.createAssignment(expression.leftHandSide,
191+
expression.operator, expression.rightHandSide);
192+
_calls.add(ChainCall(piece, CallType.property));
193+
194+
case MethodInvocation(:var target) when isCascade || target != null:
195+
if (target != null) _unwrapCall(target);
125196

126197
var callPiece = _visitor.buildPiece((b) {
127198
b.token(expression.operator);
@@ -135,8 +206,8 @@ class ChainBuilder {
135206
_calls.add(ChainCall(callPiece,
136207
canSplit ? CallType.splittableCall : CallType.unsplittableCall));
137208

138-
case PropertyAccess(:var target?):
139-
_unwrapCall(target);
209+
case PropertyAccess(:var target):
210+
if (target != null) _unwrapCall(target);
140211

141212
var piece = _visitor.buildPiece((b) {
142213
b.token(expression.operator);
@@ -146,7 +217,7 @@ class ChainBuilder {
146217
_calls.add(ChainCall(piece, CallType.property));
147218

148219
case PrefixedIdentifier(:var prefix):
149-
_unwrapCall(prefix);
220+
if (!isCascade) _unwrapCall(prefix);
150221

151222
var piece = _visitor.buildPiece((b) {
152223
b.token(expression.period);
@@ -165,6 +236,19 @@ class ChainBuilder {
165236
});
166237
});
167238

239+
case IndexExpression() when isCascade && _calls.isEmpty:
240+
// An index expression as the first cascade section should be part of
241+
// the cascade chain and not part of the target, as in:
242+
//
243+
// foo
244+
// ..[index]
245+
// ..another();
246+
//
247+
// For non-cascade method chains, we keep leave the index as part of
248+
// the target since the method chain doesn't begin until the first `.`.
249+
var piece = _visitor.createIndexExpression(null, expression);
250+
_calls.add(ChainCall(piece, CallType.property));
251+
168252
case IndexExpression():
169253
_unwrapPostfix(expression.target!, (target) {
170254
return _visitor.createIndexExpression(target, expression);
@@ -180,7 +264,7 @@ class ChainBuilder {
180264

181265
default:
182266
// Otherwise, it isn't a selector so we've reached the target.
183-
_visitTarget(expression);
267+
if (!isCascade) _visitTarget(expression);
184268
}
185269
}
186270

@@ -194,8 +278,9 @@ class ChainBuilder {
194278
void _unwrapPostfix(
195279
Expression operand, Piece Function(Piece target) createPostfix) {
196280
_unwrapCall(operand);
281+
197282
// If we don't have a preceding call to hang the postfix expression off of,
198-
// wrap it around the target expression. For example:
283+
// make it part of the target expression. For example:
199284
//
200285
// (list + another)!
201286
if (_calls.isEmpty) {

lib/src/front_end/piece_factory.dart

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -599,6 +599,7 @@ mixin PieceFactory {
599599
return buildPiece((b) {
600600
if (target != null) b.add(target);
601601
b.token(index.question);
602+
b.token(index.period);
602603
b.token(index.leftBracket);
603604
b.visit(index.index);
604605
b.token(index.rightBracket);

lib/src/piece/chain.dart

Lines changed: 17 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -108,13 +108,24 @@ class ChainPiece extends Piece {
108108
/// .method();
109109
final bool _allowSplitInTarget;
110110

111+
/// How much to indent the chain when it splits.
112+
///
113+
/// This is [Indent.expression] for regular chains and [Indent.cascade] for
114+
/// cascades.
115+
final int _indent;
116+
111117
/// Creates a new ChainPiece.
112118
///
113119
/// Instead of calling this directly, prefer using [ChainBuilder].
114-
ChainPiece(
115-
this._target, this._calls, this._leadingProperties, this._blockCallIndex,
116-
{required bool allowSplitInTarget})
117-
: _allowSplitInTarget = allowSplitInTarget,
120+
ChainPiece(this._target, this._calls,
121+
{int leadingProperties = 0,
122+
int blockCallIndex = -1,
123+
int indent = Indent.expression,
124+
required bool allowSplitInTarget})
125+
: _leadingProperties = leadingProperties,
126+
_blockCallIndex = blockCallIndex,
127+
_indent = indent,
128+
_allowSplitInTarget = allowSplitInTarget,
118129
// If there are no calls, we shouldn't have created a chain.
119130
assert(_calls.isNotEmpty);
120131

@@ -137,12 +148,12 @@ class ChainPiece extends Piece {
137148
case State.unsplit:
138149
writer.setAllowNewlines(_allowSplitInTarget);
139150
case _splitAfterProperties:
140-
writer.setIndent(Indent.expression);
151+
writer.setIndent(_indent);
141152
writer.setAllowNewlines(_allowSplitInTarget);
142153
case _blockFormatTrailingCall:
143154
writer.setAllowNewlines(_allowSplitInTarget);
144155
case State.split:
145-
writer.setIndent(Indent.expression);
156+
writer.setIndent(_indent);
146157
}
147158

148159
writer.format(_target);

test/invocation/block_argument_kind.stmt

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -165,6 +165,31 @@ function________________(new SomeClass());
165165
function________________(
166166
new SomeClass(),
167167
);
168+
>>> Function expression call.
169+
function((expression)(veryLongArgumentExpression));
170+
<<<
171+
function((expression)(
172+
veryLongArgumentExpression,
173+
));
174+
>>> Zero-argument function expression call with block comment.
175+
function((expression)(/* long comment */));
176+
<<<
177+
function((expression)(
178+
/* long comment */
179+
));
180+
>>> Zero-argument function expression call with line comment.
181+
function((expression)(// comment
182+
));
183+
<<<
184+
function((expression)(
185+
// comment
186+
));
187+
>>> A zero-argument function expression call is not a block argument.
188+
function_______________________((expr)());
189+
<<<
190+
function_______________________(
191+
(expr)(),
192+
);
168193
>>> Parenthesized expression where inner expression is a block argument.
169194
function((innerFunction(veryLongArgumentExpression)));
170195
<<<

0 commit comments

Comments
 (0)