Fix binary op as body in arrow expression (#921)

* Implement new logic for wrapping binary op in arrowFunctionExpression.

* Add new test cases.

* Reuse new helper function in order to fix #917.

* Add new test case.

* Extend heuristic to dive deeper into mixed types.

* Add new test.

* Enhance logic to cover more cases.

* Add new test cases.

* Disable Flow as it gets BindExpression as an unexpected token.

* Simplify getCombinedDeepest function.

* Add missing case.

* Extract all conditions in switch cases to one top level condition.

* Refactor implementation to make it cleaner and also handle ExpressionStatement.

* Update related test cases.

* Add new test case.

* Make condition less expensive.

* Clean up unecessary conditions, simplify condition involving startsWithOpenCurlyBrace.

* Update and add new test cases for better coverage.

* Remove unecessary condition, refactor canBeFirstInStatement to drop some useless parens.

* Update test cases accordingly 🚀.

src/fast-path.js

@@ -245,14 +245,16 @@ FPp.needsParens = function(assumeExpressionContext) {
     return true;
   }
 
+  if (
+    ((parent.type === "ArrowFunctionExpression" && parent.body === node) ||
+    parent.type === "ExpressionStatement") &&
+    startsWithOpenCurlyBrace(node)
+  ) {
+    return true;
+  }
+
   switch (node.type) {
     case "CallExpression":
-      if (
-        node.callee.type === "ObjectExpression" &&
-        parent.type === "ArrowFunctionExpression"
-      ) {
-        return true;
-      }
       return false;
 
     case "SpreadElement":
@@ -305,13 +307,6 @@ FPp.needsParens = function(assumeExpressionContext) {
         return true;
       }
 
-      if (
-        node.operator === "instanceof" &&
-        parent.type === "ArrowFunctionExpression"
-      ) {
-        return true;
-      }
-
     case "LogicalExpression":
       switch (parent.type) {
         case "CallExpression":
@@ -525,17 +520,6 @@ FPp.needsParens = function(assumeExpressionContext) {
 
       return false;
 
-    case "ObjectExpression":
-      if (parent.type === "ArrowFunctionExpression" && name === "body") {
-        return true;
-      }
-      if (parent.type === "TaggedTemplateExpression") {
-        return true;
-      }
-      if (parent.type === "MemberExpression") {
-        return name === "object" && parent.object === node;
-      }
-
     case "StringLiteral":
       if (parent.type === "ExpressionStatement") {
         return true;
@@ -583,12 +567,41 @@ function containsCallExpression(node) {
   return false;
 }
 
+function startsWithOpenCurlyBrace(node) {
+  node = getLeftMost(node);
+  switch (node.type) {
+    case "ObjectExpression":
+      return true;
+    case "MemberExpression":
+      return startsWithOpenCurlyBrace(node.object);
+    case "TaggedTemplateExpression":
+      return startsWithOpenCurlyBrace(node.tag);
+    case "CallExpression":
+      return startsWithOpenCurlyBrace(node.callee);
+    case "ConditionalExpression":
+      return startsWithOpenCurlyBrace(node.test);
+    case "UpdateExpression":
+      return !node.prefix && startsWithOpenCurlyBrace(node.argument);
+    case "BindExpression":
+      return node.object && startsWithOpenCurlyBrace(node.object);
+    case "SequenceExpression":
+      return startsWithOpenCurlyBrace(node.expressions[0])
+    default:
+      return false;
+  }
+}
+
+function getLeftMost(node) {
+  if (node.left) {
+    return getLeftMost(node.left);
+  } else {
+    return node;
+  }
+}
+
 FPp.canBeFirstInStatement = function() {
-  var node = this.getNode();
-  return !n.FunctionExpression.check(node) &&
-    !n.ObjectExpression.check(node) &&
-    !n.ClassExpression.check(node) &&
-    !(n.AssignmentExpression.check(node) && n.ObjectPattern.check(node.left));
+  const node = this.getNode();
+  return !n.FunctionExpression.check(node) && !n.ClassExpression.check(node);
 };
 
 FPp.firstInStatement = function() {

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

@@ -10,6 +10,23 @@ new (() => {});
 if ((() => {}) ? 1 : 0) {}
 let f = () => ({}())
 let a = () => ({} instanceof a);
+a = () => ({} && a);
+a = () => ({}() && a);
+a = () => ({} && a && b);
+a = () => ({} + a);
+a = () => ({}()() && a);
+a = () => ({}.b && a);
+a = () => ({}[b] && a);
+a = () => ({}\`\` && a);
+a = () => ({} = 0);
+a = () => ({}, a);
+a => a instanceof {};
+a => ({}().b && 0)
+a => ({}::b()\`\`[''].c++ && 0 ? 0 : 0)
+a => ({}().c = 0)
+x => ({}()())
+x => ({}()\`\`)
+x => ({}().b)
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 (a => {}).length;
 typeof (() => {});
@@ -21,6 +38,23 @@ if ((() => {}) ? 1 : 0) {
 }
 let f = () => ({}());
 let a = () => ({} instanceof a);
+a = () => ({} && a);
+a = () => ({}() && a);
+a = () => ({} && a && b);
+a = () => ({} + a);
+a = () => ({}()() && a);
+a = () => ({}.b && a);
+a = () => ({}[b] && a);
+a = () => ({}\`\` && a);
+a = () => ({} = 0);
+a = () => ({}, a);
+(a => a instanceof {});
+(a => ({}().b && 0));
+(a => ({}::b()\`\`[\\"\\"].c++ && 0 ? 0 : 0));
+(a => ({}().c = 0));
+(x => ({}()()));
+(x => ({}()\`\`));
+(x => ({}().b));
 "
 `;
 

tests/arrows/arrow_function_expression.js

@@ -7,3 +7,20 @@ new (() => {});
 if ((() => {}) ? 1 : 0) {}
 let f = () => ({}())
 let a = () => ({} instanceof a);
+a = () => ({} && a);
+a = () => ({}() && a);
+a = () => ({} && a && b);
+a = () => ({} + a);
+a = () => ({}()() && a);
+a = () => ({}.b && a);
+a = () => ({}[b] && a);
+a = () => ({}`` && a);
+a = () => ({} = 0);
+a = () => ({}, a);
+a => a instanceof {};
+a => ({}().b && 0)
+a => ({}::b()``[''].c++ && 0 ? 0 : 0)
+a => ({}().c = 0)
+x => ({}()())
+x => ({}()``)
+x => ({}().b)

tests/arrows/jsfmt.spec.js

@@ -1 +1 @@
-run_spec(__dirname);
+run_spec(__dirname, {parser: 'babylon'});

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

@@ -162,7 +162,7 @@ let tests = [
     x + \\"\\"; // error
     \\"\\" + x; // error
     x + {}; // error
-    ({}) + x; // error
+    ({} + x); // error
   },
 
   // when one side is a string or number and the other is invalid, we
@@ -310,10 +310,10 @@ let tests = [
 \\"foo\\" < 1; // error
 \\"foo\\" < \\"bar\\";
 1 < { foo: 1 }; // error
-({ foo: 1 }) < 1; // error
-({ foo: 1 }) < { foo: 1 }; // error
+({ foo: 1 } < 1); // error
+({ foo: 1 } < { foo: 1 }); // error
 \\"foo\\" < { foo: 1 }; // error
-({ foo: 1 }) < \\"foo\\"; // error
+({ foo: 1 } < \\"foo\\"); // error
 
 var x = (null: ?number);
 1 < x; // 2 errors: null !~> number; undefined !~> number

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

@@ -92,7 +92,7 @@ let tests = [
   function() {
     null in {}; // error
     void 0 in {}; // error
-    ({}) in {}; // error
+    ({} in {}); // error
     [] in {}; // error
     false in []; // error
   },

tests/flow/object-method/__snapshots__/jsfmt.spec.js.snap

@@ -126,7 +126,7 @@ function foo() {
 
 function bar(f: () => void) {
   f(); // passing global object as \`this\`
-  ({ f }).f(); // passing container object as \`this\`
+  ({ f }.f()); // passing container object as \`this\`
 }
 
 bar(foo); // error, since \`this\` is used non-trivially in \`foo\`

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

@@ -2582,7 +2582,7 @@ let tests = [
   function(x: A) {
     if (x.kind === null.toString()) {
     } // error, method on null
-    if (({ kind: 1 }).kind === null.toString()) {
+    if ({ kind: 1 }.kind === null.toString()) {
     } // error, method on null
   },
 

tests/method-chain/__snapshots__/jsfmt.spec.js.snap

@@ -295,8 +295,8 @@ exports[`test.js 1`] = `
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 method().then(x => x)[\\"abc\\"](x => x)[abc](x => x);
 
-({}).a().b();
-({}).a().b();
+({}.a().b());
+({}.a().b());
 "
 `;
 

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

@@ -4,10 +4,22 @@ exports[`expression.js 1`] = `
 "() => ({}\`\`);
 ({})\`\`;
 a = () => ({}).x;
+({} && a, b);
+({}::b, 0);
+({}::b()\`\`[''].c++ && 0 ? 0 : 0, 0);
+({}(), 0);
+({} = 0);
+(({} = 0), 1);
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
-(() => ({})\`\`);
-({})\`\`;
-a = () => ({}).x;
+(() => ({}\`\`));
+({}\`\`);
+a = () => ({}.x);
+({} && a, b);
+({}::b, 0);
+({}::b()\`\`[\\"\\"].c++ && 0 ? 0 : 0, 0);
+({}(), 0);
+({} = 0);
+({} = 0), 1;
 "
 `;
 

tests/objects/expression.js

@@ -1,3 +1,9 @@
 () => ({}``);
 ({})``;
 a = () => ({}).x;
+({} && a, b);
+({}::b, 0);
+({}::b()``[''].c++ && 0 ? 0 : 0, 0);
+({}(), 0);
+({} = 0);
+(({} = 0), 1);

tests/objects/jsfmt.spec.js

@@ -1 +1 @@
-run_spec(__dirname);
+run_spec(__dirname, {parser: 'babylon'});

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

@@ -125,7 +125,7 @@ b.c\`\`;
 new B()\`\`;
 
 // \\"ObjectExpression\\"
-({})\`\`;
+({}\`\`);
 
 // \\"SequenceExpression\\"
 (b, c)\`\`;