Repository: jlongster/prettier
Pull request #218 (2017-01-15 16:55:35)

Remove faulty location check on template literals that throws in Nuclide
There's a handful of files inside of Nuclide that throw exceptions because an assertion is raised.

```
{ AssertionError: ']' === '`'
    at fixTemplateLiteral (/Users/vjeux/random/prettier/src/util.js:105:10)
    at Object.util.fixFaultyLocations (/Users/vjeux/random/prettier/src/util.js:45:5)
    at getSortedChildNodes (/Users/vjeux/random/prettier/src/comments.js:25:8)
    at getSortedChildNodes (/Users/vjeux/random/prettier/src/comments.js:61:5)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:71:20)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at decorateComment (/Users/vjeux/random/prettier/src/comments.js:85:7)
    at /Users/vjeux/random/prettier/src/comments.js:129:5
```

When trying https://github.com/facebook/nuclide/blob/master/pkg/nuclide-task-runner/lib/main.js#L174

It throws in the fixTemplateLiteral method.

That method was added to fix https://github.com/benjamn/recast/issues/216 more than a year ago to fix the following code:

```js
var x = {
  y: () => Relay.QL`
    query {
      ${foo},
      field,
    }
  `
};
```

I've checked (and added a test) and it now parses and prints correctly without that method. So it should be safe to remove.

jlongster: Oh **hot damn** do I love PRs like this. I had no idea what that code did and it looked like a lot of work so I'm glad we can remove it. I wonder if we can look at the rest of `fixFaultyLocations` and see if we really need to do that. If so we could streamline more of the comments functionality.

This makes me so happy! Thank you!
jlongster: There goes that flaky test again :( I just restarted so we'll see... I'm so confounded by that.

Patch:
diff --git a/src/util.js b/src/util.js
index bc51155ef..ae727036c 100644
--- a/src/util.js
+++ b/src/util.js
@@ -41,9 +41,7 @@ function expandLoc(parentNode, childNode) {
 }
 
 util.fixFaultyLocations = function(node, text) {
-  if (node.type === "TemplateLiteral") {
-    fixTemplateLiteral(node, text);
-  } else if (node.decorators) {
+  if (node.decorators) {
     // Expand the loc of the node responsible for printing the decorators
     // (here, the decorated node) so that it includes node.decorators.
     node.decorators.forEach(function(decorator) {
@@ -79,68 +77,6 @@ util.fixFaultyLocations = function(node, text) {
   }
 };
 
-function fixTemplateLiteral(node, text) {
-  assert.strictEqual(node.type, "TemplateLiteral");
-
-  if (node.quasis.length === 0) {
-    // If there are no quasi elements, then there is nothing to fix.
-    return;
-  }
-
-  // First we need to exclude the opening ` from the loc of the first
-  // quasi element, in case the parser accidentally decided to include it.
-  var afterLeftBackTickPos = locStart(node);
-  assert.strictEqual(text.charAt(afterLeftBackTickPos), "`");
-  assert.ok(afterLeftBackTickPos < text.length);
-  var firstQuasi = node.quasis[(0)];
-  if (locStart(firstQuasi) - afterLeftBackTickPos < 0) {
-    setLocStart(firstQuasi, afterLeftBackTickPos);
-  }
-
-  // Next we need to exclude the closing ` from the loc of the last quasi
-  // element, in case the parser accidentally decided to include it.
-  var rightBackTickPos = locEnd(node);
-  assert.ok(rightBackTickPos >= 0);
-  assert.strictEqual(text.charAt(rightBackTickPos), "`");
-  var lastQuasi = node.quasis[node.quasis.length - 1];
-  if (rightBackTickPos - locEnd(lastQuasi) < 0) {
-    setLocEnd(locEnd(lastQuasi), rightBackTickPos);
-  }
-
-  // Now we need to exclude ${ and } characters from the loc's of all
-  // quasi elements, since some parsers accidentally include them.
-  node.expressions.forEach(function(expr, i) {
-    // Rewind from the start loc over any whitespace and the ${ that
-    // precedes the expression. The position of the $ should be the
-    // same as the end of the preceding quasi element, but some
-    // parsers accidentally include the ${ in the loc of the quasi
-    // element.
-    var dollarCurlyPos = skipSpaces(text, locStart(expr) - 1, true);
-    if (
-      dollarCurlyPos - 1 >= 0 && text.charAt(dollarCurlyPos - 1) === "{" &&
-        dollarCurlyPos - 2 >= 0 &&
-        text.charAt(dollarCurlyPos - 2) === "$"
-    ) {
-      var quasiBefore = node.quasis[i];
-      if (dollarCurlyPos - locEnd(quasiBefore) < 0) {
-        setLocEnd(quasiBefore, dollarCurlyPos);
-      }
-    }
-
-    // Likewise, some parsers accidentally include the } that follows
-    // the expression in the loc of the following quasi element.
-    var rightCurlyPos = skipSpaces(text, locEnd(expr));
-    if (text.charAt(rightCurlyPos) === "}") {
-      assert.ok(rightCurlyPos + 1 < text.length);
-      // Now rightCurlyPos is technically the position just after the }.
-      var quasiAfter = node.quasis[i + 1];
-      if (locStart(quasiAfter) - rightCurlyPos < 0) {
-        setLocStart(locStart(quasiAfter), rightCurlyPos);
-      }
-    }
-  });
-}
-
 util.isExportDeclaration = function(node) {
   if (node)
     switch (node.type) {


Test patch:
diff --git a/tests/template/__snapshots__/jsfmt.spec.js.snap b/tests/template/__snapshots__/jsfmt.spec.js.snap
index 55258e7bd..70c3fb59a 100644
--- a/tests/template/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/template/__snapshots__/jsfmt.spec.js.snap
@@ -1,3 +1,32 @@
+exports[`test faulty-locations.js 1`] = `
+"var o = {
+  [\`key\`]: () => {
+    // Comment
+  }
+};
+
+var x = {
+  y: () => Relay.QL\`
+    query {
+      \${foo},
+      field,
+    }
+  \`
+};
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+var o = { [\`key\`]: () => {} };
+
+var x = {
+  y: () => Relay.QL\`
+    query {
+      \${foo},
+      field,
+    }
+  \`
+};
+"
+`;
+
 exports[`test template.js 1`] = `
 "/* @flow */
 
diff --git a/tests/template/faulty-locations.js b/tests/template/faulty-locations.js
new file mode 100644
index 000000000..15a8559c0
--- /dev/null
+++ b/tests/template/faulty-locations.js
@@ -0,0 +1,14 @@
+var o = {
+  [`key`]: () => {
+    // Comment
+  }
+};
+
+var x = {
+  y: () => Relay.QL`
+    query {
+      ${foo},
+      field,
+    }
+  `
+};