Improve how expressions in control flow headers are handled. (#1326)
Improve how parenthesized expressions in control flow are handled.
In the process of writing tests for if statements, I realized that the
way if conditions are formatted was different than switch values. And
it turns out that, based on looking at how all of the control flow in
Flutter is hand-formatted, the way I was handling switches was more
complex than needed.
It seems like the right behavior we want is to just visit the expression
directly and let the parentheses attach directly to it.
So I updated if, while, and switch to all do that.
Also added tests for comments in if statements.
diff --git a/lib/src/front_end/ast_node_visitor.dart b/lib/src/front_end/ast_node_visitor.dart
index 7700e0a..a9e7fa7 100644
--- a/lib/src/front_end/ast_node_visitor.dart
+++ b/lib/src/front_end/ast_node_visitor.dart
@@ -1280,7 +1280,7 @@
var list = DelimitedListBuilder(this,
const ListStyle(spaceWhenUnsplit: true, splitListIfBeforeSplits: true));
- createSwitchValue(node.switchKeyword, node.leftParenthesis, node.expression,
+ startControlFlow(node.switchKeyword, node.leftParenthesis, node.expression,
node.rightParenthesis);
space();
list.leftBracket(node.leftBracket);
@@ -1304,7 +1304,7 @@
@override
void visitSwitchStatement(SwitchStatement node) {
- createSwitchValue(node.switchKeyword, node.leftParenthesis, node.expression,
+ startControlFlow(node.switchKeyword, node.leftParenthesis, node.expression,
node.rightParenthesis);
// Attach the ` {` after the `)` in the [ListPiece] created by
diff --git a/lib/src/front_end/piece_factory.dart b/lib/src/front_end/piece_factory.dart
index 6a01bab..5f955fc 100644
--- a/lib/src/front_end/piece_factory.dart
+++ b/lib/src/front_end/piece_factory.dart
@@ -148,6 +148,18 @@
);
}
+ /// Visits the leading keyword and parenthesized expression at the beginning
+ /// of an `if`, `while`, or `switch` expression or statement.
+ void startControlFlow(Token keyword, Token leftParenthesis, Expression value,
+ Token rightParenthesis) {
+ // Attach the keyword to the `(`.
+ token(keyword);
+ space();
+ token(leftParenthesis);
+ visit(value);
+ token(rightParenthesis);
+ }
+
/// Creates metadata annotations for a directive.
///
/// Always forces the annotations to be on a previous line.
@@ -231,11 +243,8 @@
// Recurses through the else branches to flatten them into a linear if-else
// chain handled by a single [IfPiece].
void traverse(IfStatement node) {
- token(node.ifKeyword);
- space();
- token(node.leftParenthesis);
- visit(node.expression);
- token(node.rightParenthesis);
+ startControlFlow(node.ifKeyword, node.leftParenthesis, node.expression,
+ node.rightParenthesis);
var condition = pieces.split();
// Edge case: When the then branch is a block and there is an else clause
@@ -432,21 +441,6 @@
pieces.give(builder.build());
}
- /// Visits the `switch (expr)` part of a switch statement or expression.
- void createSwitchValue(Token switchKeyword, Token leftParenthesis,
- Expression value, Token rightParenthesis) {
- // Attach the `switch ` as part of the `(`.
- token(switchKeyword);
- space();
-
- createList(
- leftBracket: leftParenthesis,
- [value],
- rightBracket: rightParenthesis,
- style: const ListStyle(
- commas: Commas.none, splitCost: 2, allowBlockElement: true));
- }
-
/// Creates a class, enum, extension, mixin, or mixin application class
/// declaration.
///
diff --git a/test/expression/switch.stmt b/test/expression/switch.stmt
index 6d4aac4..4d8817d 100644
--- a/test/expression/switch.stmt
+++ b/test/expression/switch.stmt
@@ -52,14 +52,12 @@
1 => b,
2 => c,
};
->>> Split before switch value.
+>>> Don't split at parentheses.
e = switch ("a long string that must wrap") {
0 => "ok"
};
<<<
-e = switch (
- "a long string that must wrap"
-) {
+e = switch ("a long string that must wrap") {
0 => "ok",
};
>>> Split in delimited value expression.
diff --git a/test/statement/if.stmt b/test/statement/if.stmt
index d5817b2..8743dec 100644
--- a/test/statement/if.stmt
+++ b/test/statement/if.stmt
@@ -1,11 +1,38 @@
40 columns |
->>> Split in condition.
+>>> Don't split before or after condition.
+if (veryLongConditionExpressionWithNoSplit) { body;}
+<<<
+if (veryLongConditionExpressionWithNoSplit) {
+ body;
+}
+>>> Split inside condition expression.
if (veryLongCondition || anotherLongCondition) { body; }
<<<
if (veryLongCondition ||
anotherLongCondition) {
body;
}
+>>> Condition expressions can use block formatting.
+if ([element, element, element, element]) { body; }
+<<<
+if ([
+ element,
+ element,
+ element,
+ element,
+]) {
+ body;
+}
+>>>
+if (someFunction(argument, argument, argument)) { body; }
+<<<
+if (someFunction(
+ argument,
+ argument,
+ argument,
+)) {
+ body;
+}
>>> Indentation.
if ( true ) { return 42; } else { return 13; }
<<<
diff --git a/test/statement/if_comment.stmt b/test/statement/if_comment.stmt
new file mode 100644
index 0000000..8fb13b9
--- /dev/null
+++ b/test/statement/if_comment.stmt
@@ -0,0 +1,80 @@
+40 columns |
+>>> Line comment after `if`.
+if // comment
+(c) { body; }
+<<<
+if // comment
+(c) {
+ body;
+}
+>>> Line comment before condition.
+if (// comment
+c) { body; }
+<<<
+if ( // comment
+c) {
+ body;
+}
+>>> Line comment after condition.
+if (c // comment
+){ body; }
+<<<
+if (c // comment
+) {
+ body;
+}
+>>> Line comment after `)`.
+if (c) // comment
+{ body; }
+<<<
+if (c) // comment
+{
+ body;
+}
+>>> Line comment after body.
+if (c)
+{ body; } // comment
+<<<
+if (c) {
+ body;
+} // comment
+>>> Line comment before `else`.
+if (c) { body; } // comment
+else { other; }
+<<<
+if (c) {
+ body;
+} // comment
+else {
+ other;
+}
+>>> Line comment after `else`.
+if (c) { body; } else// comment
+{ other; }
+<<<
+if (c) {
+ body;
+} else // comment
+{
+ other;
+}
+>>> Line comment after `else` body.
+if (c) { body; } else { other; }// comment
+<<<
+if (c) {
+ body;
+} else {
+ other;
+} // comment
+>>> Line comments in logic condition.
+if (// Do stuff.
+ condition1 ||
+ // More stuff.
+ condition2) { body; }
+<<<
+if ( // Do stuff.
+ condition1 ||
+ // More stuff.
+ condition2) {
+ body;
+}
\ No newline at end of file
diff --git a/test/statement/switch.stmt b/test/statement/switch.stmt
index 8f99c84..7b3cacb 100644
--- a/test/statement/switch.stmt
+++ b/test/statement/switch.stmt
@@ -251,15 +251,13 @@
default:
break;
}
->>> Split before the switch expression.
+>>> Don't split at parentheses.
switch ("a long string that must wrap") {
case 0:
return "ok";
}
<<<
-switch (
- "a long string that must wrap"
-) {
+switch ("a long string that must wrap") {
case 0:
return "ok";
}
diff --git a/test/statement/while.stmt b/test/statement/while.stmt
index e8c2204..380b9b0 100644
--- a/test/statement/while.stmt
+++ b/test/statement/while.stmt
@@ -7,7 +7,7 @@
while (true);
<<<
while (true) ;
->>> Don't split before long condition.
+>>> Don't split at parentheses.
while (aLongConditionExpressionThatWraps) {
;
}
@@ -24,6 +24,18 @@
expressionThatWraps) {
;
}
+>>> Block format condition expressions that allow it.
+while (function(argument, argument, argument)) {
+ ;
+}
+<<<
+while (function(
+ argument,
+ argument,
+ argument,
+)) {
+ ;
+}
>>> Unbraced body.
while (condition) something(i);
<<<