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.

Author: munificent

lib/src/front_end/ast_node_visitor.dart

@@ -1280,7 +1280,7 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
     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 @@ class AstNodeVisitor extends ThrowingAstVisitor<void>
 
   @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

lib/src/front_end/piece_factory.dart

@@ -148,6 +148,18 @@ mixin PieceFactory implements CommentWriter {
     );
   }
 
+  /// 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 @@ mixin PieceFactory implements CommentWriter {
     // 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 @@ mixin PieceFactory implements CommentWriter {
     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.
   ///

test/expression/switch.stmt

@@ -52,14 +52,12 @@ e = switch (obj) {
   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.

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;  }
 <<<

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;
+}

test/statement/switch.stmt

@@ -251,15 +251,13 @@ switch (fruit) {
   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";
 }

test/statement/while.stmt

@@ -7,7 +7,7 @@ while (true) {}
 while (true);
 <<<
 while (true) ;
->>> Don't split before long condition.
+>>> Don't split at parentheses.
 while (aLongConditionExpressionThatWraps) {
   ;
 }
@@ -24,6 +24,18 @@ while (aLongCondition +
     expressionThatWraps) {
   ;
 }
+>>> Block format condition expressions that allow it.
+while (function(argument, argument, argument)) {
+  ;
+}
+<<<
+while (function(
+  argument,
+  argument,
+  argument,
+)) {
+  ;
+}
 >>> Unbraced body.
 while (condition) something(i);
 <<<