Repository: jlongster/prettier
Pull request #262 (2017-01-17 05:26:21)

Print binary and logical expressions in a nicer format
This is a complete rewrite of binary and logical expressions that, if anything, is at least a whole lot more consistent. There may be tweaks to actual output for certain patterns, but before it was somewhat arbitrary what was broken across lines.

Now, if an expression is broken up, it's all or nothing:

```js
var x = reallyLongArg() + reallyLongArg() * reallyLongArg() % reallyLongArg() + reallyLongArg() + reallyLongArg() + reallyLongArg();
```

Turns into:

```js
  var x = reallyLongArg() +
    reallyLongArg() *
    reallyLongArg() %
    reallyLongArg() +
    reallyLongArg() +
    reallyLongArg() +
    reallyLongArg();
```

The idea is that we shouldn't touch anything that you could write out by hand without adding parens. The AST makes this a little difficult, but I think the solution ended up elegant. I spent a lot of hours writing 50 lines of code, and it's mostly comments!

If there is an expression that has a difference precedence than the default, that becomes a new sub-group. For example:

```js
var z = 1 + 2 + 3 * (4 * 5 + 3) / (100000000000000000 * 328233872942374 + 3423942394239) + 2343242342342343 + 10000;
```

This turns into:

```
var z = 1 +
  2 +
  3 *
  (4 * 5 + 3) /
  (100000000000000000 * 328233872942374 + 3423942394239) +
  2343242342342343 +
  10000;
```

Notice how there are 2 expressions in there that aren't broken; this is because that have difference precedence and become sub-groups. I think this feels natural and looks good. Also parens are always properly kept.

[Here's a gist](https://gist.github.com/jlongster/dc68eef51952b0520287291072de1e85) with more examples. This was my test file.

I'm aware that there is a bug with removing parens where it shouldn't which you can see in the tests. I'll fix this tomorrow.

vjeux: <3

Can't wait to try it on on a real codebase first thing in the morning tomorrow!
jlongster: In https://github.com/jlongster/prettier/issues/46 @llimllib noted that this could be particularly ugly for certain binary expressions. I sort of agree, although I'm willing to take a little ugliness for automated formatting. However, maybe we can do a little more work here.

For logical expressions, we should always break each one on its own line so that the `&&` and the `||` are clear and work as separators. But we could do something different for binary expressions which is usually math. In fact, now that we have a lot more control over what's going on, just playing with the grouping I was able to produce this code:

```js
  var z = 1 + 2 + 3 * (4 * 5 + 3) / (100000000000000000 * 328233872942374 +
      3423942394239) + 2343242342342343 + 10000;
```

Maybe for binary exprs we allow it to flow as much as possible (exprs take up as much as they can on each line). Should we try to force "sub-groups" to break? It could look weird the the parens wrap across lines.
jlongster: I'll also add more tests to demonstrate the format. Not many current ones changed.
vjeux: One use case I have in mind where this would output "bad" result is when building html by string concatenation:

```js
var html =
  '<div>' + escape(title) + '</div>' +
    '<ul>' +
      '<li>' + escape(item) + '</li>' +
    '</ul>' +
  '</div>';
```

I would probably have to rewrite it using an array and .join to make it readable
jlongster: If the user wants to layout string concatenation in a special way that depends on the string contents, we'll never be able to support that. It goes against the goal of this printer which exists to enforce completely consistent syntax across an entire project, so we can't allow a set of binary expressions to differ solely based on the contents of their operands.

A couple options:

```js
var html = '<div>' + escape(title) + '                                                      ' +
  '<ul>                                                                                                       ' +
  '  <li> ' + item + '</li>'                                                                           ' +
  '</ul>                                                                                                     '  +
  '</div>                                                                                                   ' ;
```

Not the large amount of space on the right; that would keep each string on its own line. Many editors allow a "replace" mode where anything you type *replaces* the current characters instead of adding to it, and this would allow you to freely edit the HTML without consistently having to reformat the string concats.

But really, users should use string templates. This project is built in a modern era where we assume basic ES6 constructs are available. That solves the issue completely:

```js
var html = `
<div> ${escape(title)}
  <ul>
    <li>${item}</li>
  </ul>
</div>`;
```

We should encourage the above if you want to control the format of a string, which allows us to format binary expressions consistently.
vjeux: I'm so old school, of course, template literals are the way to solve this :)
llimllib: > Maybe for binary exprs we allow it to flow as much as possible

I think that's the best available answer, I don't think parens wrapping across lines is much of a problem.
llimllib: But like you said, I'm willing to sacrifice perfection for consistency on anything reasonable.
vjeux: This is the result of running master on nuclide and then master + this diff again on Nuclide. There are some spurious changes due to other bugs but this gives a good idea:

https://gist.github.com/vjeux/9bc3481bd06c9fc40273db9bdf4b8455
vjeux: It seems to be overly aggressive at splitting things:

```diff
-  while (a[a.length - 1 - i] === b[b.length - 1 - i] && i < a.length &&
-    i < b.length) {
+  while (a[a.length - 1 - i] ===
+    b[b.length - 1 - i] &&
+    i <
+    a.length &&
+    i <
+    b.length) {
```

I would have expected it to be converted to

```js
  while (a[a.length - 1 - i] === b[b.length - 1 - i] &&
    i < a.length &&
    i < b.length) {
```

We don't need to break on every single `<` or `===` as the expression fits in one line. 
vjeux: As you mentioned, it is missing parenthesis:

```diff
-      const nanoseconds = (milliseconds - seconds * 1000) * 1000000;
+      const nanoseconds = milliseconds - seconds * 1000 * 1000000;
```
llimllib: The `directory instanceof\n Directory` split looks bad too
vjeux: ```js
       if (
-        mirror.isNumber() && isNaN(mirror.value()) && cached.isNumber() &&
+        mirror.isNumber() &&
+          isNaN(mirror.value()) &&
+          cached.isNumber() &&
           isNaN(cached.value())
       ) {
```

I'm not sure about the indentation for the second and later. I think they should all be on the same line if they are of the same type. If we start adding || in there, then it makes sense to indent.
jlongster: > I'm not sure about the indentation for the second and later. I think they should all be on the same line if they are of the same type. If we start adding || in there, then it makes sense to indent.

What's difficult this case:

```js
var x = 1 + 2 + 3 + 4 + 5;
```

I think variable declarations assume that the right-hand side will indent itself. For example, objects print like this:

```js
var x = {
  prop1: 1,
  prop2: 2
};
```

The object is what adds indentation because it should be there regardless if it's a variable declaration or not. So if we remove the indentation for binary expressions, we would print this:

```js
var x = foo &&
bar &&
baz;
```

I'm still working on this and have the nuclide codebase to go by. I'll try to find a balance that will work better.
vjeux: I think that the indent makes sense for + to have an indent. It's nice to see that the second line is part of the same thing.

For your second example, I would expect it to look like

```js
var x =
  foo &&
  bar &&
  baz;
```

or

```js
var x = (
  foo &&
  bar &&
  baz
);
```
jlongster: Thanks for the title change! That definitely is more clear.
vjeux: Omg, so much better! Thanks for following through this!

As mentioned on gitter, the only thing that I feel uneasy about is the way `+` is handled. It feels overly aggressive to always split on every single `+`.

One thing we found is that it's a common pattern to have two string literals that are concatenated togethers meaning that before they were in two lines:

```js
  var injection = '(function (NM) {' +
    'NM.require("module")._load(' + injectorServerPath + ')' +
    '(' + JSON.stringify(options) + ')' +
  '})(NM)';
```

So, what we should try is to put as many `+` expressions as a line permits and enforce hard lines between two consecutive string literals.

In this example, we would lose the indentation but at least it would print with the same format.
jlongster: > In this example, we would lose the indentation but at least it would print with the same format.

You could easily regain indentation by putting it in the string: `'  NM.require...'`

Can you create a followup issue for that? I'll add some tests tomorrow and merge it.

Patch:
diff --git a/src/fast-path.js b/src/fast-path.js
index de8b4572d..19eb2be28 100644
--- a/src/fast-path.js
+++ b/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;
diff --git a/src/printer.js b/src/printer.js
index 7735f1091..3e2fd50f9 100644
--- a/src/printer.js
+++ b/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 ";";
diff --git a/src/util.js b/src/util.js
index 91c6e99e4..9f128958c 100644
--- a/src/util.js
+++ b/src/util.js
@@ -222,3 +222,25 @@ function htmlEscapeInsideDoubleQuote(str) {
   //    .replace(/>/g, '&gt;');
 }
 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];
+}


Test patch:
diff --git a/tests/intersection/__snapshots__/jsfmt.spec.js.snap b/tests/intersection/__snapshots__/jsfmt.spec.js.snap
index efee67571..b745dc7a5 100644
--- a/tests/intersection/__snapshots__/jsfmt.spec.js.snap
+++ b/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);
 }
 "
diff --git a/tests/prettier/__snapshots__/jsfmt.spec.js.snap b/tests/prettier/__snapshots__/jsfmt.spec.js.snap
index a7d0701c6..f3a305f9c 100644
--- a/tests/prettier/__snapshots__/jsfmt.spec.js.snap
+++ b/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\";
 
diff --git a/tests/prettier/binary-expressions.js b/tests/prettier/binary-expressions.js
new file mode 100644
index 000000000..8b0660f4f
--- /dev/null
+++ b/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);