From ce6e8979504cf03a1feb0b62c5a62166440ba31b Mon Sep 17 00:00:00 2001
From: Christopher Chedeau <vjeuxx@gmail.com>
Date: Tue, 21 Mar 2017 12:13:13 -0700
Subject: [PATCH] [RFC] Fix comment location for binary expressions (#1043)

The root cause is that we're calling `printComments` with an empty string, meaning that the leading/trailing comments are not correctly inserted at the right location.

So, the way to fix it is to call `p => concat(parts)` but because we're mutating the array in place, it doesn't work. We need to mutate it and create a copy. But, the root call is actually checking the shape of the parts array which our code is now breaking...

```js
          // 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] : "",
```

The consequence is that binary expressions are no longer indented if the first expression has a comment (but now it places the comment properly!), which seems like a good trade-off.

I'm not sure if we should merge this one or instead refactor this code such that it doesn't rely on mutation and looking at the shape of the printed tree. `printMemberChain` is a good thing to reference for inspiration.

Fixes #946
---
 src/printer.js                                   |  6 +++++-
 .../__snapshots__/jsfmt.spec.js.snap             | 15 +++++++++++++++
 tests/binary-expressions/comment.js              |  5 +++++
 tests/comments/__snapshots__/jsfmt.spec.js.snap  | 16 ++++++++--------
 4 files changed, 33 insertions(+), 9 deletions(-)
 create mode 100644 tests/binary-expressions/comment.js

diff --git a/src/printer.js b/src/printer.js
index cd91c89d..b79c9fb8 100644
--- a/src/printer.js
+++ b/src/printer.js
@@ -3013,7 +3013,11 @@ function printBinaryishExpressions(path, parts, print, options, isNested) {
     // the other ones since we don't call the normal print on BinaryExpression,
     // only for the left and right parts
     if (isNested && node.comments) {
-      parts.push(comments.printComments(path, p => "", options));
+      parts.splice(
+        0,
+        parts.length,
+        comments.printComments(path, p => concat(parts.slice()), options)
+      );
     }
   } else {
     // Our stopping case. Simply print the node normally.
diff --git a/tests/binary-expressions/__snapshots__/jsfmt.spec.js.snap b/tests/binary-expressions/__snapshots__/jsfmt.spec.js.snap
index acb94da4..09286db6 100644
--- a/tests/binary-expressions/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/binary-expressions/__snapshots__/jsfmt.spec.js.snap
@@ -19,6 +19,21 @@ function f() {
 "
 `;
 
+exports[`comment.js 1`] = `
+"a = (
+  // Commment 1
+  (Math.random() * (yRange * (1 - minVerticalFraction)))
+  + (minVerticalFraction * yRange)
+) - offset;
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+a =
+  // Commment 1
+  Math.random() * (yRange * (1 - minVerticalFraction)) +
+  minVerticalFraction * yRange -
+  offset;
+"
+`;
+
 exports[`exp.js 1`] = `
 "a ** b ** c;
 (a ** b) ** c;
diff --git a/tests/binary-expressions/comment.js b/tests/binary-expressions/comment.js
new file mode 100644
index 00000000..242ab169
--- /dev/null
+++ b/tests/binary-expressions/comment.js
@@ -0,0 +1,5 @@
+a = (
+  // Commment 1
+  (Math.random() * (yRange * (1 - minVerticalFraction)))
+  + (minVerticalFraction * yRange)
+) - offset;
diff --git a/tests/comments/__snapshots__/jsfmt.spec.js.snap b/tests/comments/__snapshots__/jsfmt.spec.js.snap
index 5c4bdb77..675a0790 100644
--- a/tests/comments/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/comments/__snapshots__/jsfmt.spec.js.snap
@@ -712,10 +712,10 @@ export type AsyncExecuteOptions = child_process$execFileOpts & {
 // optional trailing comma gets moved all the way to the beginning
 const regex = new RegExp(
   \\"^\\\\\\\\s*\\" + // beginning of the line
-    \\"name\\\\\\\\s*=\\\\\\\\s*\\" + // name =
-    \\"['\\\\\\"]\\" + // opening quotation mark
-    escapeStringRegExp(target.name) + // target name
-    \\"['\\\\\\"]\\" + // closing quotation mark
+  \\"name\\\\\\\\s*=\\\\\\\\s*\\" + // name =
+  \\"['\\\\\\"]\\" + // opening quotation mark
+  escapeStringRegExp(target.name) + // target name
+  \\"['\\\\\\"]\\" + // closing quotation mark
     \\",?$\\" // optional trailing comma
 );
 
@@ -938,10 +938,10 @@ export type AsyncExecuteOptions = child_process$execFileOpts & {
 // optional trailing comma gets moved all the way to the beginning
 const regex = new RegExp(
   \\"^\\\\\\\\s*\\" + // beginning of the line
-    \\"name\\\\\\\\s*=\\\\\\\\s*\\" + // name =
-    \\"['\\\\\\"]\\" + // opening quotation mark
-    escapeStringRegExp(target.name) + // target name
-    \\"['\\\\\\"]\\" + // closing quotation mark
+  \\"name\\\\\\\\s*=\\\\\\\\s*\\" + // name =
+  \\"['\\\\\\"]\\" + // opening quotation mark
+  escapeStringRegExp(target.name) + // target name
+  \\"['\\\\\\"]\\" + // closing quotation mark
     \\",?$\\" // optional trailing comma
 );