Break sequence expressions (#2388)

* Add SequenceExpression tests

* Clean up printing of SequenceExpressions

* Remove test and lint

src/fast-path.js

@@ -185,7 +185,8 @@ FastPath.prototype.needsParens = function(options) {
 
   if (
     (parent.type === "ArrowFunctionExpression" &&
-      parent.body === node &&
+    parent.body === node &&
+    node.type !== "SequenceExpression" && // these have parens added anyway
       startsWithNoLookaheadToken(node, /* forbidFunctionAndClass */ false)) ||
     (parent.type === "ExpressionStatement" &&
       startsWithNoLookaheadToken(node, /* forbidFunctionAndClass */ true))
@@ -376,6 +377,11 @@ FastPath.prototype.needsParens = function(options) {
         case "ExpressionStatement":
           return name !== "expression";
 
+        case "ArrowFunctionExpression":
+          // We do need parentheses, but SequenceExpressions are handled
+          // specially when printing bodies of arrow functions.
+          return name !== "body";
+
         default:
           // Otherwise err on the side of overparenthesization, adding
           // explicit exceptions above if this proves overzealous.

src/printer.js

@@ -454,7 +454,6 @@ function genericPrintNoParens(path, options, print, args) {
       parts.push(" =>");
 
       const body = path.call(bodyPath => print(bodyPath, args), "body");
-      const collapsed = concat([concat(parts), " ", body]);
 
       // We want to always keep these types of nodes on the same line
       // as the arrow.
@@ -463,11 +462,23 @@ function genericPrintNoParens(path, options, print, args) {
         (n.body.type === "ArrayExpression" ||
           n.body.type === "ObjectExpression" ||
           n.body.type === "BlockStatement" ||
-          n.body.type === "SequenceExpression" ||
           isTemplateOnItsOwnLine(n.body, options.originalText) ||
           n.body.type === "ArrowFunctionExpression")
       ) {
-        return group(collapsed);
+        return group(concat([concat(parts), " ", body]));
+      }
+
+      // We handle sequence expressions as the body of arrows specially,
+      // so that the required parentheses end up on their own lines.
+      if (n.body.type === "SequenceExpression") {
+        return group(
+          concat([
+            concat(parts),
+            group(
+              concat([" (", indent(concat([softline, body])), softline, ")"])
+            )
+          ])
+        );
       }
 
       // if the arrow function is expanded as last argument, we are adding a
@@ -779,7 +790,8 @@ function genericPrintNoParens(path, options, print, args) {
           );
         } else if (
           n.argument.type === "LogicalExpression" ||
-          n.argument.type === "BinaryExpression"
+          n.argument.type === "BinaryExpression" ||
+          n.argument.type === "SequenceExpression"
         ) {
           parts.push(
             group(
@@ -1124,25 +1136,26 @@ function genericPrintNoParens(path, options, print, args) {
 
       return concat(parts);
     case "SequenceExpression": {
-      const parent = path.getParentNode();
-      const shouldInline =
-        parent.type === "ReturnStatement" ||
-        parent.type === "ForStatement" ||
-        parent.type === "ExpressionStatement";
-
-      if (shouldInline) {
-        return join(", ", path.map(print, "expressions"));
+      const parent = path.getParentNode(0);
+      if (
+        parent.type === "ExpressionStatement" ||
+        parent.type === "ForStatement"
+      ) {
+        // For ExpressionStatements and for-loop heads, which are among
+        // the few places a SequenceExpression appears unparenthesized, we want
+        // to indent expressions after the first.
+        const parts = [];
+        path.each(p => {
+          if (p.getName() === 0) {
+            parts.push(print(p));
+          } else {
+            parts.push(",", indent(concat([line, print(p)])));
+          }
+        }, "expressions");
+        return group(concat(parts));
       }
       return group(
-        concat([
-          indent(
-            concat([
-              softline,
-              join(concat([",", line]), path.map(print, "expressions"))
-            ])
-          ),
-          softline
-        ])
+        concat([join(concat([",", line]), path.map(print, "expressions"))])
       );
     }
     case "ThisExpression":

tests/comments/__snapshots__/jsfmt.spec.js.snap

@@ -995,12 +995,13 @@ function excessiveEverything() {
   )
 }
 
-function sequenceExpression() {
-  return (
-    // Reason for a
-    a
-  ), b
-}
+// See https://github.com/prettier/prettier/issues/2392
+// function sequenceExpression() {
+//   return (
+//     // Reason for a
+//     a
+//   ), b
+// }
 
 function sequenceExpressionInside() {
   return ( // Reason for a
@@ -1125,12 +1126,13 @@ function excessiveEverything() {
   );
 }
 
-function sequenceExpression() {
-  return (
-    // Reason for a
-    a, b
-  );
-}
+// See https://github.com/prettier/prettier/issues/2392
+// function sequenceExpression() {
+//   return (
+//     // Reason for a
+//     a
+//   ), b
+// }
 
 function sequenceExpressionInside() {
   return (

tests/comments/return-statement.js

@@ -93,12 +93,13 @@ function excessiveEverything() {
   )
 }
 
-function sequenceExpression() {
-  return (
-    // Reason for a
-    a
-  ), b
-}
+// See https://github.com/prettier/prettier/issues/2392
+// function sequenceExpression() {
+//   return (
+//     // Reason for a
+//     a
+//   ), b
+// }
 
 function sequenceExpressionInside() {
   return ( // Reason for a

tests/flow/optional/__snapshots__/jsfmt.spec.js.snap

@@ -349,7 +349,8 @@ function foo(x?: number, ...y: Array<string>): [?number, Array<string>] {
 }
 
 foo(); // OK
-foo(123), foo(123, "hello"); // OK // OK
+foo(123), // OK
+  foo(123, "hello"); // OK
 
 foo(true); // ERROR boolean ~> number
 foo(123, true); // ERROR boolean ~> string

tests/sequence_break/__snapshots__/jsfmt.spec.js.snap

@@ -2,12 +2,76 @@
 
 exports[`break.js 1`] = `
 const f = (argument1, argument2, argument3) =>
-  (doSomethingWithArgument(argument1), doSomethingWithArgument(argument2),argument1)
+  (doSomethingWithArgument(argument1), doSomethingWithArgument(argument2),argument1);
+(function(){
+	return aLongIdentifierName, aLongIdentifierName, aLongIdentifierName, aLongIdentifierName;
+});
+aLongIdentifierName, aLongIdentifierName, aLongIdentifierName, aLongIdentifierName;
+a.then(() => (aLongIdentifierName, aLongIdentifierName, aLongIdentifierName, aLongIdentifierName));
+for (aLongIdentifierName = 0, aLongIdentifierName = 0, aLongIdentifierName = 0, aLongIdentifierName = 0; test; update) {}
+(a = b ? c : function() { return 0; }),
+  (a = b ? c : function() { return 0; }),
+  (a = b ? c : function() { return 0; }),
+  (a = b ? c : function() { return 0; }),
+  (a = b ? c : function() { return 0; });
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 const f = (argument1, argument2, argument3) => (
   doSomethingWithArgument(argument1),
   doSomethingWithArgument(argument2),
   argument1
 );
+(function() {
+  return (
+    aLongIdentifierName,
+    aLongIdentifierName,
+    aLongIdentifierName,
+    aLongIdentifierName
+  );
+});
+aLongIdentifierName,
+  aLongIdentifierName,
+  aLongIdentifierName,
+  aLongIdentifierName;
+a.then(
+  () => (
+    aLongIdentifierName,
+    aLongIdentifierName,
+    aLongIdentifierName,
+    aLongIdentifierName
+  )
+);
+for (
+  aLongIdentifierName = 0,
+    aLongIdentifierName = 0,
+    aLongIdentifierName = 0,
+    aLongIdentifierName = 0;
+  test;
+  update
+) {}
+(a = b
+  ? c
+  : function() {
+      return 0;
+    }),
+  (a = b
+    ? c
+    : function() {
+        return 0;
+      }),
+  (a = b
+    ? c
+    : function() {
+        return 0;
+      }),
+  (a = b
+    ? c
+    : function() {
+        return 0;
+      }),
+  (a = b
+    ? c
+    : function() {
+        return 0;
+      });
 
 `;

tests/sequence_break/break.js

@@ -1,2 +1,13 @@
 const f = (argument1, argument2, argument3) =>
-  (doSomethingWithArgument(argument1), doSomethingWithArgument(argument2),argument1)
+  (doSomethingWithArgument(argument1), doSomethingWithArgument(argument2),argument1);
+(function(){
+	return aLongIdentifierName, aLongIdentifierName, aLongIdentifierName, aLongIdentifierName;
+});
+aLongIdentifierName, aLongIdentifierName, aLongIdentifierName, aLongIdentifierName;
+a.then(() => (aLongIdentifierName, aLongIdentifierName, aLongIdentifierName, aLongIdentifierName));
+for (aLongIdentifierName = 0, aLongIdentifierName = 0, aLongIdentifierName = 0, aLongIdentifierName = 0; test; update) {}
+(a = b ? c : function() { return 0; }),
+  (a = b ? c : function() { return 0; }),
+  (a = b ? c : function() { return 0; }),
+  (a = b ? c : function() { return 0; }),
+  (a = b ? c : function() { return 0; });