Skip to content

Commit f88d826

Browse files
munificentmunificent
authored
Format for elements. (#1350)
Format for elements. This mostly uses the existing code for formatting the `for (...)` part of for statements, which I moved into PieceFactory so that it can be reused. In the process of adding tests for this, I noticed that a newline in the initializer of a for-in loop forced the `in` to split, which looks weird, so I fixed that too, for both for statements and for elements.
1 parent 1ad10d2 commit f88d826

10 files changed

+656
-115
lines changed

‎lib/src/front_end/ast_node_visitor.dart

Lines changed: 23 additions & 104 deletions
Original file line numberDiff line numberDiff line change
@@ -383,7 +383,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
383383
if (node.redirectedConstructor case var constructor?) {
384384
redirect = AssignPiece(
385385
tokenPiece(node.separator!), nodePiece(constructor),
386-
isValueDelimited: false);
386+
allowInnerSplit: false);
387387
} else if (node.initializers.isNotEmpty) {
388388
initializerSeparator = tokenPiece(node.separator!);
389389
initializers = createList(node.initializers,
@@ -595,7 +595,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
595595
var expression = nodePiece(node.expression);
596596

597597
b.add(AssignPiece(operatorPiece, expression,
598-
isValueDelimited: node.expression.canBlockSplit));
598+
allowInnerSplit: node.expression.canBlockSplit));
599599
b.token(node.semicolon);
600600
});
601601
}
@@ -693,117 +693,36 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
693693

694694
@override
695695
Piece visitForElement(ForElement node) {
696-
throw UnimplementedError();
697-
}
698-
699-
@override
700-
Piece visitForStatement(ForStatement node) {
701696
var forKeyword = buildPiece((b) {
702697
b.modifier(node.awaitKeyword);
703698
b.token(node.forKeyword);
704699
});
705700

706-
Piece forPartsPiece;
707-
switch (node.forLoopParts) {
708-
// Edge case: A totally empty for loop is formatted just as `(;;)` with
709-
// no splits or spaces anywhere.
710-
case ForPartsWithExpression(
711-
initialization: null,
712-
leftSeparator: Token(precedingComments: null),
713-
condition: null,
714-
rightSeparator: Token(precedingComments: null),
715-
updaters: NodeList(isEmpty: true),
716-
) &&
717-
var forParts
718-
when node.rightParenthesis.precedingComments == null:
719-
forPartsPiece = buildPiece((b) {
720-
b.token(node.leftParenthesis);
721-
b.token(forParts.leftSeparator);
722-
b.token(forParts.rightSeparator);
723-
b.token(node.rightParenthesis);
724-
});
725-
726-
case ForParts forParts &&
727-
ForPartsWithDeclarations(variables: AstNode? initializer):
728-
case ForParts forParts &&
729-
ForPartsWithExpression(initialization: AstNode? initializer):
730-
// In a C-style for loop, treat the for loop parts like an argument list
731-
// where each clause is a separate argument. This means that when they
732-
// split, they split like:
733-
//
734-
// for (
735-
// initializerClause;
736-
// conditionClause;
737-
// incrementClause
738-
// ) {
739-
// body;
740-
// }
741-
var partsList =
742-
DelimitedListBuilder(this, const ListStyle(commas: Commas.none));
743-
partsList.leftBracket(node.leftParenthesis);
744-
745-
// The initializer clause.
746-
if (initializer != null) {
747-
partsList.addCommentsBefore(initializer.beginToken);
748-
partsList.add(buildPiece((b) {
749-
b.visit(initializer);
750-
b.token(forParts.leftSeparator);
751-
}));
752-
} else {
753-
// No initializer, so look at the comments before `;`.
754-
partsList.addCommentsBefore(forParts.leftSeparator);
755-
partsList.add(tokenPiece(forParts.leftSeparator));
756-
}
757-
758-
// The condition clause.
759-
if (forParts.condition case var conditionExpression?) {
760-
partsList.addCommentsBefore(conditionExpression.beginToken);
761-
partsList.add(buildPiece((b) {
762-
b.visit(conditionExpression);
763-
b.token(forParts.rightSeparator);
764-
}));
765-
} else {
766-
partsList.addCommentsBefore(forParts.rightSeparator);
767-
partsList.add(tokenPiece(forParts.rightSeparator));
768-
}
769-
770-
// The update clauses.
771-
if (forParts.updaters.isNotEmpty) {
772-
partsList.addCommentsBefore(forParts.updaters.first.beginToken);
773-
partsList.add(createList(forParts.updaters,
774-
style: const ListStyle(commas: Commas.nonTrailing)));
775-
}
701+
var forPartsPiece = createForLoopParts(
702+
node.leftParenthesis, node.forLoopParts, node.rightParenthesis);
703+
var body = nodePiece(node.body);
776704

777-
partsList.rightBracket(node.rightParenthesis);
778-
forPartsPiece = partsList.build();
705+
var forPiece = ForPiece(forKeyword, forPartsPiece, body,
706+
hasBlockBody: node.body.isSpreadCollection);
779707

780-
case ForPartsWithPattern():
781-
throw UnimplementedError();
708+
// It looks weird to have multiple nested control flow elements on the same
709+
// line, so force the outer one to split if there is an inner one.
710+
if (node.body.isControlFlowElement) {
711+
forPiece.pin(State.split);
712+
}
782713

783-
case ForEachParts forEachParts &&
784-
ForEachPartsWithDeclaration(loopVariable: AstNode variable):
785-
case ForEachParts forEachParts &&
786-
ForEachPartsWithIdentifier(identifier: AstNode variable):
787-
// If a for-in loop, treat the for parts like an assignment, so they
788-
// split like:
789-
//
790-
// for (var variable in [
791-
// initializer,
792-
// ]) {
793-
// body;
794-
// }
795-
forPartsPiece = buildPiece((b) {
796-
b.token(node.leftParenthesis);
797-
b.add(createAssignment(
798-
variable, forEachParts.inKeyword, forEachParts.iterable,
799-
splitBeforeOperator: true));
800-
b.token(node.rightParenthesis);
801-
});
714+
return forPiece;
715+
}
802716

803-
case ForEachPartsWithPattern():
804-
throw UnimplementedError();
805-
}
717+
@override
718+
Piece visitForStatement(ForStatement node) {
719+
var forKeyword = buildPiece((b) {
720+
b.modifier(node.awaitKeyword);
721+
b.token(node.forKeyword);
722+
});
806723

724+
var forPartsPiece = createForLoopParts(
725+
node.leftParenthesis, node.forLoopParts, node.rightParenthesis);
807726
var body = nodePiece(node.body);
808727

809728
return ForPiece(forKeyword, forPartsPiece, body,
@@ -1824,7 +1743,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<Piece> with PieceFactory {
18241743
var initializerPiece = nodePiece(initializer, commaAfter: true);
18251744

18261745
variables.add(AssignPiece(variablePiece, initializerPiece,
1827-
isValueDelimited: initializer.canBlockSplit));
1746+
allowInnerSplit: initializer.canBlockSplit));
18281747
} else {
18291748
variables.add(tokenPiece(variable.name, commaAfter: true));
18301749
}

‎lib/src/front_end/piece_factory.dart

Lines changed: 146 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -213,6 +213,137 @@ mixin PieceFactory {
213213
});
214214
}
215215

216+
/// Creates a piece for the parentheses and inner parts of a for statement or
217+
/// for element.
218+
Piece createForLoopParts(Token leftParenthesis, ForLoopParts forLoopParts,
219+
Token rightParenthesis) {
220+
switch (forLoopParts) {
221+
// Edge case: A totally empty for loop is formatted just as `(;;)` with
222+
// no splits or spaces anywhere.
223+
case ForPartsWithExpression(
224+
initialization: null,
225+
leftSeparator: Token(precedingComments: null),
226+
condition: null,
227+
rightSeparator: Token(precedingComments: null),
228+
updaters: NodeList(isEmpty: true),
229+
)
230+
when rightParenthesis.precedingComments == null:
231+
return buildPiece((b) {
232+
b.token(leftParenthesis);
233+
b.token(forLoopParts.leftSeparator);
234+
b.token(forLoopParts.rightSeparator);
235+
b.token(rightParenthesis);
236+
});
237+
238+
case ForParts forParts &&
239+
ForPartsWithDeclarations(variables: AstNode? initializer):
240+
case ForParts forParts &&
241+
ForPartsWithExpression(initialization: AstNode? initializer):
242+
// In a C-style for loop, treat the for loop parts like an argument list
243+
// where each clause is a separate argument. This means that when they
244+
// split, they split like:
245+
//
246+
// for (
247+
// initializerClause;
248+
// conditionClause;
249+
// incrementClause
250+
// ) {
251+
// body;
252+
// }
253+
var partsList =
254+
DelimitedListBuilder(this, const ListStyle(commas: Commas.none));
255+
partsList.leftBracket(leftParenthesis);
256+
257+
// The initializer clause.
258+
if (initializer != null) {
259+
partsList.addCommentsBefore(initializer.beginToken);
260+
partsList.add(buildPiece((b) {
261+
b.visit(initializer);
262+
b.token(forParts.leftSeparator);
263+
}));
264+
} else {
265+
// No initializer, so look at the comments before `;`.
266+
partsList.addCommentsBefore(forParts.leftSeparator);
267+
partsList.add(tokenPiece(forParts.leftSeparator));
268+
}
269+
270+
// The condition clause.
271+
if (forParts.condition case var conditionExpression?) {
272+
partsList.addCommentsBefore(conditionExpression.beginToken);
273+
partsList.add(buildPiece((b) {
274+
b.visit(conditionExpression);
275+
b.token(forParts.rightSeparator);
276+
}));
277+
} else {
278+
partsList.addCommentsBefore(forParts.rightSeparator);
279+
partsList.add(tokenPiece(forParts.rightSeparator));
280+
}
281+
282+
// The update clauses.
283+
if (forParts.updaters.isNotEmpty) {
284+
partsList.addCommentsBefore(forParts.updaters.first.beginToken);
285+
partsList.add(createList(forParts.updaters,
286+
style: const ListStyle(commas: Commas.nonTrailing)));
287+
}
288+
289+
partsList.rightBracket(rightParenthesis);
290+
return partsList.build();
291+
292+
case ForPartsWithPattern():
293+
throw UnimplementedError();
294+
295+
case ForEachParts forEachParts &&
296+
ForEachPartsWithDeclaration(loopVariable: AstNode variable):
297+
case ForEachParts forEachParts &&
298+
ForEachPartsWithIdentifier(identifier: AstNode variable):
299+
// If a for-in loop, treat the for parts like an assignment, so they
300+
// split like:
301+
//
302+
// for (var variable in [
303+
// initializer,
304+
// ]) {
305+
// body;
306+
// }
307+
// TODO(tall): Passing `allowInnerSplit: true` allows output like:
308+
//
309+
// // 1
310+
// for (variable in longExpression +
311+
// thatWraps) {
312+
// ...
313+
// }
314+
//
315+
// Versus the split in the initializer forcing a split before `in` too:
316+
//
317+
// // 2
318+
// for (variable
319+
// in longExpression +
320+
// thatWraps) {
321+
// ...
322+
// }
323+
//
324+
// This is also allowed:
325+
//
326+
// // 3
327+
// for (variable
328+
// in longExpression + thatWraps) {
329+
// ...
330+
// }
331+
//
332+
// Currently, the formatter prefers 1 over 3. We may want to revisit
333+
// that and prefer 3 instead.
334+
return buildPiece((b) {
335+
b.token(leftParenthesis);
336+
b.add(createAssignment(
337+
variable, forEachParts.inKeyword, forEachParts.iterable,
338+
splitBeforeOperator: true, allowInnerSplit: true));
339+
b.token(rightParenthesis);
340+
});
341+
342+
case ForEachPartsWithPattern():
343+
throw UnimplementedError();
344+
}
345+
}
346+
216347
/// Creates a normal (not function-typed) formal parameter with a name and/or
217348
/// type annotation.
218349
///
@@ -688,11 +819,23 @@ mixin PieceFactory {
688819
/// If [splitBeforeOperator] is `true`, then puts [operator] at the beginning
689820
/// of the next line when it splits. Otherwise, puts the operator at the end
690821
/// of the preceding line.
822+
///
823+
/// If [allowInnerSplit] is `true`, then a newline inside the target or
824+
/// right-hand side doesn't force splitting at the operator itself.
691825
Piece createAssignment(
692826
AstNode target, Token operator, Expression rightHandSide,
693827
{bool splitBeforeOperator = false,
694828
bool includeComma = false,
695-
bool spaceBeforeOperator = true}) {
829+
bool spaceBeforeOperator = true,
830+
bool allowInnerSplit = false}) {
831+
// If the right-hand side can have block formatting, then a newline in
832+
// it doesn't force the operator to split, as in:
833+
//
834+
// var list = [
835+
// element,
836+
// ];
837+
allowInnerSplit |= rightHandSide.canBlockSplit;
838+
696839
if (splitBeforeOperator) {
697840
var targetPiece = nodePiece(target);
698841

@@ -703,7 +846,7 @@ mixin PieceFactory {
703846
});
704847

705848
return AssignPiece(targetPiece, initializer,
706-
isValueDelimited: rightHandSide.canBlockSplit);
849+
allowInnerSplit: allowInnerSplit);
707850
} else {
708851
var targetPiece = buildPiece((b) {
709852
b.visit(target);
@@ -713,7 +856,7 @@ mixin PieceFactory {
713856
var initializer = nodePiece(rightHandSide, commaAfter: includeComma);
714857

715858
return AssignPiece(targetPiece, initializer,
716-
isValueDelimited: rightHandSide.canBlockSplit);
859+
allowInnerSplit: allowInnerSplit);
717860
}
718861
}
719862

‎lib/src/piece/assign.dart

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,12 +44,12 @@ class AssignPiece extends Piece {
4444
/// The right-hand side of the operation.
4545
final Piece value;
4646

47-
/// Whether the right-hand side is a delimited expression that should receive
48-
/// block-like formatting.
49-
final bool _isValueDelimited;
47+
/// Whether a newline is allowed in the right-hand side without forcing a
48+
/// split at the assignment operator.
49+
final bool _allowInnerSplit;
5050

51-
AssignPiece(this.target, this.value, {bool isValueDelimited = false})
52-
: _isValueDelimited = isValueDelimited;
51+
AssignPiece(this.target, this.value, {bool allowInnerSplit = false})
52+
: _allowInnerSplit = allowInnerSplit;
5353

5454
// TODO(tall): The old formatter allows the first operand of a split
5555
// conditional expression to be on the same line as the `=`, as in:
@@ -91,9 +91,9 @@ class AssignPiece extends Piece {
9191

9292
@override
9393
void format(CodeWriter writer, State state) {
94-
// A split in either child piece forces splitting after the "=" unless it's
95-
// a delimited expression.
96-
if (state == State.unsplit && !_isValueDelimited) {
94+
// A split in either child piece forces splitting at assignment operator
95+
// unless specifically allowed.
96+
if (!_allowInnerSplit && state == State.unsplit) {
9797
writer.setAllowNewlines(false);
9898
}
9999

0 commit comments

Comments
 (0)