Print binary and logical expressions in a nicer format (#262)

* Print binary and logical expressions in a nicer format (fixes #89)

* Improve algorithm (wip)

* Change grouping

* Final (hopefully?) pass at binary/logical expressions

* Update snapshots

* Add tests

src/fast-path.js

@@ -1,5 +1,6 @@
 var assert = require("assert");
 var types = require("ast-types");
+var util = require("./util");
 var n = types.namedTypes;
 var Node = n.Node;
 var isArray = types.builtInTypes.array;
@@ -262,9 +263,9 @@ FPp.needsParens = function(assumeExpressionContext) {
         case "BinaryExpression":
         case "LogicalExpression":
           var po = parent.operator;
-          var pp = PRECEDENCE[po];
+          var pp = util.getPrecedence(po);
           var no = node.operator;
-          var np = PRECEDENCE[no];
+          var np = util.getPrecedence(no);
 
           if (pp > np) {
             return true;
@@ -424,24 +425,6 @@ function isUnaryLike(node) {
   ) || n.SpreadElement && n.SpreadElement.check(node) || n.SpreadProperty && n.SpreadProperty.check(node);
 }
 
-var PRECEDENCE = {};
-[
-  [ "||" ],
-  [ "&&" ],
-  [ "|" ],
-  [ "^" ],
-  [ "&" ],
-  [ "==", "===", "!=", "!==" ],
-  [ "<", ">", "<=", ">=", "in", "instanceof" ],
-  [ ">>", "<<", ">>>" ],
-  [ "+", "-" ],
-  [ "*", "/", "%", "**" ]
-].forEach(function(tier, i) {
-  tier.forEach(function(op) {
-    PRECEDENCE[op] = i;
-  });
-});
-
 function containsCallExpression(node) {
   if (n.CallExpression.check(node)) {
     return true;

src/printer.js

@@ -179,15 +179,18 @@ function genericPrintNoParens(path, options, print) {
         ])
       );
     case "BinaryExpression":
-    case "LogicalExpression":
-      return group(
-        concat([
-          path.call(print, "left"),
-          " ",
-          n.operator,
-          indent(options.tabWidth, concat([ line, path.call(print, "right") ]))
-        ])
-      );
+    case "LogicalExpression": {
+      const parts = [];
+      printBinaryishExpressions(path, parts, print, options);
+
+      return group(concat([
+        // Don't include the initial expression in the indentation
+        // level. The first item is guaranteed to be the first
+        // left-most expression.
+        parts.length > 0 ? parts[0] : "",
+        indent(options.tabWidth, concat(parts.slice(1)))
+      ]));
+    }
     case "AssignmentPattern":
       return concat([
         path.call(print, "left"),
@@ -2254,6 +2257,55 @@ function maybeWrapJSXElementInParens(path, elem, options) {
   );
 }
 
+function isBinaryish(node) {
+  return node.type === "BinaryExpression" || node.type === "LogicalExpression";
+}
+
+// For binary expressions to be consistent, we need to group
+// subsequent operators with the same precedence level under a single
+// group. Otherwise they will be nested such that some of them break
+// onto new lines but not all. Operators with the same precedence
+// level should either all break or not. Because we group them by
+// precedence level and the AST is structured based on precedence
+// level, things are naturally broken up correctly, i.e. `&&` is
+// broken before `+`.
+function printBinaryishExpressions(path, parts, print) {
+  let node = path.getValue();
+
+  // We treat BinaryExpression and LogicalExpression nodes the same.
+  if(isBinaryish(node)) {
+    // Put all operators with the same precedence level in the same
+    // group. The reason we only need to do this with the `left`
+    // expression is because given an expression like `1 + 2 - 3`, it
+    // is always parsed like `((1 + 2) - 3)`, meaning the `left` side
+    // is where the rest of the expression will exist. Binary
+    // expressions on the right side mean they have a difference
+    // precedence level and should be treated as a separate group, so
+    // print them normally.
+    if(util.getPrecedence(node.left.operator) === util.getPrecedence(node.operator)) {
+      // Flatten them out by recursively calling this function. The
+      // printed values will all be appended to `parts`.
+      path.call(left => printBinaryishExpressions(left, parts, print), "left");
+    }
+    else {
+      parts.push(path.call(print, "left"));
+    }
+
+    parts.push(
+      " ",
+      node.operator,
+      line,
+      path.call(print, "right")
+    );
+  }
+  else {
+    // Our stopping case. Simply print the node normally.
+    parts.push(path.call(print));
+  }
+
+  return parts;
+}
+
 function adjustClause(clause, options, forceSpace) {
   if (clause === "") {
     return ";";

src/util.js

@@ -222,3 +222,25 @@ function htmlEscapeInsideDoubleQuote(str) {
   //    .replace(/>/g, '>');
 }
 util.htmlEscapeInsideDoubleQuote = htmlEscapeInsideDoubleQuote;
+
+var PRECEDENCE = {};
+[
+  [ "||" ],
+  [ "&&" ],
+  [ "|" ],
+  [ "^" ],
+  [ "&" ],
+  [ "==", "===", "!=", "!==" ],
+  [ "<", ">", "<=", ">=", "in", "instanceof" ],
+  [ ">>", "<<", ">>>" ],
+  [ "+", "-" ],
+  [ "*", "/", "%", "**" ]
+].forEach(function(tier, i) {
+  tier.forEach(function(op) {
+    PRECEDENCE[op] = i;
+  });
+});
+
+util.getPrecedence = function(op) {
+  return PRECEDENCE[op];
+}

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

@@ -87,12 +87,14 @@ type DuplexStreamOptions =
   };
 
 function hasObjectMode_bad(options: DuplexStreamOptions): boolean {
-  return options.objectMode || options.readableObjectMode ||
+  return options.objectMode ||
+    options.readableObjectMode ||
     options.writableObjectMode; // error, undefined ~> boolean
 }
 
 function hasObjectMode_ok(options: DuplexStreamOptions): boolean {
-  return !!(options.objectMode || options.readableObjectMode ||
+  return !!(options.objectMode ||
+    options.readableObjectMode ||
     options.writableObjectMode);
 }
 "

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

@@ -1,3 +1,65 @@
+exports[`test binary-expressions.js 1`] = `
+"// It should always break the highest precedence operators first, and
+// break them all at the same time.
+
+const x = longVariable + longVariable + longVariable;
+const x = longVariable + longVariable + longVariable + longVariable - longVariable + longVariable;
+const x = longVariable + longVariable * longVariable + longVariable - longVariable + longVariable;
+const x = longVariable + longVariable * longVariable * longVariable / longVariable + longVariable;
+
+const x = longVariable && longVariable && longVariable && longVariable && longVariable && longVariable;
+const x = longVariable && longVariable || longVariable && longVariable || longVariable && longVariable;
+
+const x = longVariable * longint && longVariable >> 0 && longVariable + longVariable;
+
+const x = longVariable > longint && longVariable === 0 + longVariable * longVariable;
+
+foo(obj.property * new Class() && obj instanceof Class && longVariable ? number + 5 : false);
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// It should always break the highest precedence operators first, and
+// break them all at the same time.
+
+const x = longVariable + longVariable + longVariable;
+const x = longVariable +
+  longVariable +
+  longVariable +
+  longVariable -
+  longVariable +
+  longVariable;
+const x = longVariable +
+  longVariable * longVariable +
+  longVariable -
+  longVariable +
+  longVariable;
+const x = longVariable +
+  longVariable * longVariable * longVariable / longVariable +
+  longVariable;
+
+const x = longVariable &&
+  longVariable &&
+  longVariable &&
+  longVariable &&
+  longVariable &&
+  longVariable;
+const x = longVariable && longVariable ||
+  longVariable && longVariable ||
+  longVariable && longVariable;
+
+const x = longVariable * longint &&
+  longVariable >> 0 &&
+  longVariable + longVariable;
+
+const x = longVariable > longint &&
+  longVariable === 0 + longVariable * longVariable;
+
+foo(
+  obj.property * new Class() && obj instanceof Class && longVariable
+    ? number + 5
+    : false
+);
+"
+`;
+
 exports[`test directives.js 1`] = `
 "\"use strict\";
 

tests/prettier/binary-expressions.js

@@ -0,0 +1,16 @@
+// It should always break the highest precedence operators first, and
+// break them all at the same time.
+
+const x = longVariable + longVariable + longVariable;
+const x = longVariable + longVariable + longVariable + longVariable - longVariable + longVariable;
+const x = longVariable + longVariable * longVariable + longVariable - longVariable + longVariable;
+const x = longVariable + longVariable * longVariable * longVariable / longVariable + longVariable;
+
+const x = longVariable && longVariable && longVariable && longVariable && longVariable && longVariable;
+const x = longVariable && longVariable || longVariable && longVariable || longVariable && longVariable;
+
+const x = longVariable * longint && longVariable >> 0 && longVariable + longVariable;
+
+const x = longVariable > longint && longVariable === 0 + longVariable * longVariable;
+
+foo(obj.property * new Class() && obj instanceof Class && longVariable ? number + 5 : false);