Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Format for-in loops. #1314

Merged
merged 2 commits into from
Nov 9, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
140 changes: 95 additions & 45 deletions lib/src/front_end/ast_node_visitor.dart
Original file line number Diff line number Diff line change
Expand Up @@ -339,7 +339,9 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>

@override
void visitDeclaredIdentifier(DeclaredIdentifier node) {
throw UnimplementedError();
modifier(node.keyword);
visit(node.type, after: space);
token(node.name);
}

@override
Expand Down Expand Up @@ -476,66 +478,114 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>

@override
void visitForStatement(ForStatement node) {
modifier(node.awaitKeyword);
token(node.forKeyword);
var forKeyword = pieces.split();

// Treat the for loop parts sort of as an argument list where each clause
// is a separate argument. This means that when they split, they split like:
//
// ```
// for (
// initializerClause;
// conditionClause;
// incrementClause
// ) {
// body;
// }
// ```
var partsList =
DelimitedListBuilder(this, const ListStyle(commas: Commas.none));
partsList.leftBracket(node.leftParenthesis);

var forLoopParts = node.forLoopParts;
switch (forLoopParts) {
Piece forPartsPiece;
switch (node.forLoopParts) {
// Edge case: A totally empty for loop is formatted just as `(;;)` with
// no splits or spaces anywhere.
case ForPartsWithExpression(
initialization: null,
leftSeparator: Token(precedingComments: null),
condition: null,
rightSeparator: Token(precedingComments: null),
updaters: NodeList(isEmpty: true),
)
initialization: null,
leftSeparator: Token(precedingComments: null),
condition: null,
rightSeparator: Token(precedingComments: null),
updaters: NodeList(isEmpty: true),
) &&
var forParts
when node.rightParenthesis.precedingComments == null:
token(forLoopParts.leftSeparator);
token(forLoopParts.rightSeparator);
token(node.leftParenthesis);
token(forParts.leftSeparator);
token(forParts.rightSeparator);
token(node.rightParenthesis);
forPartsPiece = pieces.split();

case ForParts forParts &&
ForPartsWithDeclarations(variables: AstNode? initializer):
case ForParts forParts &&
ForPartsWithExpression(initialization: AstNode? initializer):
// In a C-style for loop, treat the for loop parts like an argument list
// where each clause is a separate argument. This means that when they
// split, they split like:
//
// ```
// for (
// initializerClause;
// conditionClause;
// incrementClause
// ) {
// body;
// }
// ```
var partsList =
DelimitedListBuilder(this, const ListStyle(commas: Commas.none));
partsList.leftBracket(node.leftParenthesis);

// The initializer clause.
if (initializer != null) {
partsList.addCommentsBefore(initializer.beginToken);
visit(initializer);
} else {
// No initializer, so look at the comments before `;`.
partsList.addCommentsBefore(forParts.leftSeparator);
}

token(forParts.leftSeparator);
partsList.add(pieces.split());

// The condition clause.
if (forParts.condition case var conditionExpression?) {
partsList.addCommentsBefore(conditionExpression.beginToken);
visit(conditionExpression);
} else {
partsList.addCommentsBefore(forParts.rightSeparator);
}

case ForPartsWithDeclarations():
partsList.addCommentsBefore(forLoopParts.variables.beginToken);
visit(forLoopParts.variables);
finishForParts(forLoopParts, partsList);
token(forParts.rightSeparator);
partsList.add(pieces.split());

case ForPartsWithExpression(:var initialization?):
partsList.addCommentsBefore(initialization.beginToken);
visit(initialization);
finishForParts(forLoopParts, partsList);
// The update clauses.
if (forParts.updaters.isNotEmpty) {
partsList.addCommentsBefore(forParts.updaters.first.beginToken);
createList(forParts.updaters,
style: const ListStyle(commas: Commas.nonTrailing));
partsList.add(pieces.split());
}

case ForPartsWithExpression():
// No initializer part.
partsList.addCommentsBefore(forLoopParts.leftSeparator);
finishForParts(forLoopParts, partsList);
partsList.rightBracket(node.rightParenthesis);
pieces.split();
forPartsPiece = partsList.build();

case ForPartsWithPattern():
case ForEachPartsWithDeclaration():
case ForEachPartsWithIdentifier():
throw UnimplementedError();

case ForEachParts forEachParts &&
ForEachPartsWithDeclaration(loopVariable: AstNode variable):
case ForEachParts forEachParts &&
ForEachPartsWithIdentifier(identifier: AstNode variable):
// If a for-in loop, treat the for parts like an assignment, so they
// split like:
//
// ```
// for (var variable in [
// initializer,
// ]) {
// body;
// }
// ```
token(node.leftParenthesis);
visit(variable);

finishAssignment(forEachParts.inKeyword, forEachParts.iterable,
munificent marked this conversation as resolved.
Show resolved Hide resolved
splitBeforeOperator: true);
token(node.rightParenthesis);
forPartsPiece = pieces.split();

case ForEachPartsWithPattern():
throw UnimplementedError();
}

partsList.rightBracket(node.rightParenthesis);
var forPartsPiece = partsList.build();

pieces.split();
visit(node.body);
var body = pieces.take();

Expand Down
58 changes: 22 additions & 36 deletions lib/src/front_end/piece_factory.dart
Original file line number Diff line number Diff line change
Expand Up @@ -526,22 +526,34 @@ mixin PieceFactory implements CommentWriter {
if (body.keyword != null) space();
}

/// Creates a [Piece] for some code followed by an `=` and an expression in
/// any place where an `=` appears:
/// Creates a [Piece] with "assignment-like" splitting.
///
/// This is used, obviously, for assignments and variable declarations to
/// handle splitting after the `=`, but is also used in any context where an
/// expression follows something that it "defines" or "initializes":
///
/// * Assignment
/// * Variable declaration
/// * Constructor initializer
/// * Expression (`=>`) function body
/// * Named argument or named record field (`:`)
/// * Map entry (`:`)
/// * For-in loop iterator (`in`)
///
/// This is also used for map literal entries and named arguments which are
/// also sort of like bindings. In that case, [operator] is the `:`.
///
/// This method assumes the code to the left of the `=` or `:` has already
/// This method assumes the code to the left of the operator has already
/// been visited.
void finishAssignment(Token operator, Expression rightHandSide) {
if (operator.type == TokenType.EQ) space();
token(operator);
var target = pieces.split();
void finishAssignment(Token operator, Expression rightHandSide,
{bool splitBeforeOperator = false}) {
munificent marked this conversation as resolved.
Show resolved Hide resolved
Piece target;
if (splitBeforeOperator) {
target = pieces.split();
token(operator);
space();
} else {
if (operator.type == TokenType.EQ) space();
token(operator);
target = pieces.split();
}

visit(rightHandSide);

Expand All @@ -559,32 +571,6 @@ mixin PieceFactory implements CommentWriter {
rightBracket: argumentList.rightParenthesis);
}

/// Writes the condition and updaters parts of a [ForParts] after the
/// subclass's initializer clause has been written.
void finishForParts(ForParts forLoopParts, DelimitedListBuilder partsList) {
token(forLoopParts.leftSeparator);
partsList.add(pieces.split());

// The condition clause.
if (forLoopParts.condition case var conditionExpression?) {
partsList.addCommentsBefore(conditionExpression.beginToken);
visit(conditionExpression);
} else {
partsList.addCommentsBefore(forLoopParts.rightSeparator);
}

token(forLoopParts.rightSeparator);
partsList.add(pieces.split());

// The update clauses.
if (forLoopParts.updaters.isNotEmpty) {
partsList.addCommentsBefore(forLoopParts.updaters.first.beginToken);
createList(forLoopParts.updaters,
style: const ListStyle(commas: Commas.nonTrailing));
partsList.add(pieces.split());
}
}

/// Finishes writing a named function declaration or anonymous function
/// expression after the return type (if any) and name (if any) has been
/// written.
Expand Down
18 changes: 11 additions & 7 deletions lib/src/piece/assign.dart
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ import 'piece.dart';
///
/// This piece is also used for map entries and named arguments where `:` is
/// followed by an expression or element because those also want to support the
/// "block-like" formatting of delimited expressions on the right.
/// "block-like" formatting of delimited expressions on the right, and for the
/// `in` clause in for-in loops.
///
/// These constructs can be formatted three ways:
///
Expand Down Expand Up @@ -42,18 +43,21 @@ class AssignPiece extends Piece {
/// This is only allowed when the value is a delimited expression.
static const State _insideValue = State(1);

/// Split after the `=` and allow splitting inside the value.
/// Split at the operator and allow splitting inside the value.
///
/// This is more costly because, when it's possible to split inside a
/// delimited value, we want to prefer that.
static const State _atEquals = State(2, cost: 2);
static const State _atOperator = State(2, cost: 2);

/// The left-hand side of the `=` and the `=` itself.
/// The left-hand side of the operation. Includes the operator unless it is
/// `in`.
final Piece target;

/// The right-hand side of the `=`.
/// The right-hand side of the operation.
final Piece value;

/// Whether the right-hand side is a delimited expression that should receive
/// block-like formatting.
final bool _isValueDelimited;

AssignPiece(this.target, this.value, {required bool isValueDelimited})
Expand Down Expand Up @@ -104,7 +108,7 @@ class AssignPiece extends Piece {

@override
List<State> get additionalStates =>
[if (_isValueDelimited) _insideValue, _atEquals];
[if (_isValueDelimited) _insideValue, _atOperator];

@override
void format(CodeWriter writer, State state) {
Expand All @@ -116,7 +120,7 @@ class AssignPiece extends Piece {
if (state != _insideValue) writer.setIndent(Indent.expression);

writer.format(target);
writer.splitIf(state == _atEquals);
writer.splitIf(state == _atOperator);
writer.format(value);
}

Expand Down
79 changes: 79 additions & 0 deletions test/statement/for_in.stmt
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
40 columns |
>>> Declare variable with `var`.
for ( var file in files ) { body ; }
<<<
for (var file in files) {
body;
}
>>> Declare with type annotation.
for ( List < int > ints in intLists ) { body ; }
<<<
for (List<int> ints in intLists) {
body;
}
>>> Declare variable with `final` and type.
for (final Foo foo in foos) { body; }
<<<
for (final Foo foo in foos) {
body;
}
>>> Declare variable with just `final`.
for (final foo in foos) {
body;
}
<<<
for (final foo in foos) {
body;
}
>>> Use existing variable.
for ( x in things ) { body; }
<<<
for (x in things) {
body;
}
>>> Await for with declared variable.
foo() async {
await for ( var x in y ) { body ; }
}
<<<
foo() async {
await for (var x in y) {
body;
}
}
>>> Await for with existing variable.
foo() async {
await for ( x in y ) { body ; }
}
<<<
foo() async {
await for (x in y) {
body;
}
}
>>> Split before `in`.
for (var identifier in iteratableExpression) { body; }
<<<
for (var identifier
in iteratableExpression) {
body;
}
>>> Prefer block-like splitting after `in`.
for (var identifier in [element, element, element]) { body; }
<<<
for (var identifier in [
element,
element,
element,
]) {
body;
}
>>> Unsplit non-block body.
for (i in sequence) something(i);
<<<
for (i in sequence) something(i);
>>> Split non-block body.
for (i in sequence) somethingMuchLonger(i);
<<<
for (i in sequence)
somethingMuchLonger(i);