Repository: jlongster/prettier
Pull request #599 (2017-02-04 03:35:10)

Do not attach comments to EmptyStatement
Since we don't print EmptyStatement, we don't want to attach comments to them. I've tried actually printing the EmptyStatement but it messes up a ton of other things.


Patch:
diff --git a/src/comments.js b/src/comments.js
index bd5c25d7f..35e80fa01 100644
--- a/src/comments.js
+++ b/src/comments.js
@@ -28,7 +28,7 @@ function getSortedChildNodes(node, text, resultArray) {
   util.fixFaultyLocations(node, text);
 
   if (resultArray) {
-    if (n.Node.check(node)) {
+    if (n.Node.check(node) && node.type !== "EmptyStatement") {
       // This reverse insertion sort almost always takes constant
       // time because we almost always (maybe always?) append the
       // nodes in order anyway.
@@ -145,7 +145,8 @@ function attach(comments, ast, text) {
       // We also need to check if it's the first line of the file.
       if (
         handleMemberExpressionComment(enclosingNode, followingNode, comment) ||
-          handleIfStatementComments(enclosingNode, followingNode, comment)
+          handleIfStatementComments(enclosingNode, followingNode, comment) ||
+          handleTryStatementComments(enclosingNode, followingNode, comment)
       ) {
         // We're good
       } else if (followingNode) {
@@ -286,6 +287,14 @@ function addBlockStatementFirstComment(node, comment) {
   }
 }
 
+function addBlockOrNotComment(node, comment) {
+  if (node.type === "BlockStatement") {
+    addBlockStatementFirstComment(node, comment);
+  } else {
+    addLeadingComment(node, comment);
+  }
+}
+
 // There are often comments before the else clause of if statements like
 //
 //   if (1) { ... }
@@ -315,11 +324,33 @@ function handleIfStatementComments(enclosingNode, followingNode, comment) {
   }
 
   if (followingNode.type === "IfStatement") {
-    if (followingNode.consequent.type === "BlockStatement") {
-      addBlockStatementFirstComment(followingNode.consequent, comment);
-    } else {
-      addLeadingComment(followingNode.consequent, comment);
-    }
+    addBlockOrNotComment(followingNode.consequent, comment);
+    return true;
+  }
+
+  return false;
+}
+
+// Same as IfStatement but for TryStatement
+function handleTryStatementComments(enclosingNode, followingNode, comment) {
+  if (
+    !enclosingNode || enclosingNode.type !== "TryStatement" || !followingNode
+  ) {
+    return false;
+  }
+
+  if (followingNode.type === "BlockStatement") {
+    addBlockStatementFirstComment(followingNode, comment);
+    return true;
+  }
+
+  if (followingNode.type === "TryStatement") {
+    addBlockOrNotComment(followingNode.finalizer, comment);
+    return true;
+  }
+
+  if (followingNode.type === "CatchClause") {
+    addBlockOrNotComment(followingNode.body, comment);
     return true;
   }
 
diff --git a/src/fast-path.js b/src/fast-path.js
index 53ef717c2..d4dbca3a3 100644
--- a/src/fast-path.js
+++ b/src/fast-path.js
@@ -497,6 +497,11 @@ FPp.needsParens = function(assumeExpressionContext) {
         return name === "object" && parent.object === node;
       }
 
+    case "StringLiteral":
+      if (parent.type === "ExpressionStatement") {
+        return true;
+      }
+
     default:
       if (
         parent.type === "NewExpression" &&
diff --git a/src/printer.js b/src/printer.js
index 6eff3025c..1ec841514 100644
--- a/src/printer.js
+++ b/src/printer.js
@@ -1377,9 +1377,15 @@ function genericPrintNoParens(path, options, print) {
         namedTypes.ObjectTypeProperty.check(parent) ||
         namedTypes.ObjectTypeCallProperty.check(parent) ||
         namedTypes.DeclareFunction.check(path.getParentNode(2)));
+
       var needsColon = isArrowFunctionTypeAnnotation &&
         namedTypes.TypeAnnotation.check(parent);
 
+      if (isObjectTypePropertyAFunction(parent)) {
+        isArrowFunctionTypeAnnotation = true;
+        needsColon = true;
+      }
+
       if (needsColon) {
         parts.push(": ");
       }
@@ -1499,6 +1505,11 @@ function genericPrintNoParens(path, options, print) {
       var isFunction = !n.variance &&
         !n.optional &&
         n.value.type === "FunctionTypeAnnotation";
+
+      if (isObjectTypePropertyAFunction(n)) {
+        isFunction = true;
+      }
+
       return concat([
         n.static ? "static " : "",
         variance,
@@ -2077,7 +2088,11 @@ function printMemberChain(path, options, print) {
     if (node.type === "CallExpression") {
       printedNodes.unshift({
         node: node,
-        printed: printArgumentsList(path, options, print)
+        printed: comments.printComments(
+          path,
+          p => printArgumentsList(path, options, print),
+          options
+        )
       });
       path.call(callee => rec(callee), "callee");
     } else if (node.type === "MemberExpression") {
@@ -2097,7 +2112,14 @@ function printMemberChain(path, options, print) {
       });
     }
   }
-  rec(path);
+  // Note: the comments of the root node have already been printed, so we
+  // need to extract this first call without printing them as they would
+  // if handled inside of the recursive call.
+  printedNodes.unshift({
+    node: path.getValue(),
+    printed: printArgumentsList(path, options, print)
+  });
+  path.call(callee => rec(callee), "callee");
 
   // Once we have a linear list of printed nodes, we want to create groups out
   // of it.
@@ -2673,6 +2695,16 @@ function isLastStatement(path) {
   return body && body[body.length - 1] === node;
 }
 
+// Hack to differentiate between the following two which have the same ast
+// type T = { method: () => void };
+// type T = { method(): void };
+function isObjectTypePropertyAFunction(node) {
+  return node.type === "ObjectTypeProperty" &&
+    node.value.type === "FunctionTypeAnnotation" &&
+    !node.static &&
+    util.locStart(node.key) !== util.locStart(node.value);
+}
+
 function shouldPrintSameLine(node) {
   const type = node.type;
   return namedTypes.Literal.check(node) ||


Test patch:
diff --git a/tests/comments/__snapshots__/jsfmt.spec.js.snap b/tests/comments/__snapshots__/jsfmt.spec.js.snap
index 4627eed57..06a16a1fc 100644
--- a/tests/comments/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/comments/__snapshots__/jsfmt.spec.js.snap
@@ -747,3 +747,59 @@ exports[`test jsx.js 2`] = `
 </div>;
 "
 `;
+
+exports[`test try.js 1`] = `
+"// comment 1
+try {
+  // comment 2
+}
+// comment 3
+catch(e) {
+  // comment 4
+}
+// comment 5
+finally // comment 6
+{
+  // comment 7
+}
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// comment 1
+try {
+  // comment 2
+} catch (e) {
+  // comment 3
+  // comment 4
+} finally { // comment 6
+  // comment 5
+  // comment 7
+}
+"
+`;
+
+exports[`test try.js 2`] = `
+"// comment 1
+try {
+  // comment 2
+}
+// comment 3
+catch(e) {
+  // comment 4
+}
+// comment 5
+finally // comment 6
+{
+  // comment 7
+}
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// comment 1
+try {
+  // comment 2
+} catch (e) {
+  // comment 3
+  // comment 4
+} finally { // comment 6
+  // comment 5
+  // comment 7
+}
+"
+`;
diff --git a/tests/comments/try.js b/tests/comments/try.js
new file mode 100644
index 000000000..1c9205765
--- /dev/null
+++ b/tests/comments/try.js
@@ -0,0 +1,13 @@
+// comment 1
+try {
+  // comment 2
+}
+// comment 3
+catch(e) {
+  // comment 4
+}
+// comment 5
+finally // comment 6
+{
+  // comment 7
+}
diff --git a/tests/expression_statement/__snapshots__/jsfmt.spec.js.snap b/tests/expression_statement/__snapshots__/jsfmt.spec.js.snap
new file mode 100644
index 000000000..54a281bd1
--- /dev/null
+++ b/tests/expression_statement/__snapshots__/jsfmt.spec.js.snap
@@ -0,0 +1,18 @@
+exports[`test no_regression.js 1`] = `
+"// Ensure no regression.
+\"use strict\";
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// Ensure no regression.
+\"use strict\";
+
+"
+`;
+
+exports[`test use_strict.js 1`] = `
+"// Parentheses around expression statement should be preserved in this case.
+(\"use strict\");
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+// Parentheses around expression statement should be preserved in this case.
+(\"use strict\");
+"
+`;
diff --git a/tests/expression_statement/jsfmt.spec.js b/tests/expression_statement/jsfmt.spec.js
new file mode 100755
index 000000000..67caec390
--- /dev/null
+++ b/tests/expression_statement/jsfmt.spec.js
@@ -0,0 +1,2 @@
+// TODO: Re-enable Flow when the following fix is merged facebook/flow#3234.
+run_spec(__dirname, {parser: 'babylon'});
diff --git a/tests/expression_statement/no_regression.js b/tests/expression_statement/no_regression.js
new file mode 100644
index 000000000..8a5e0f44d
--- /dev/null
+++ b/tests/expression_statement/no_regression.js
@@ -0,0 +1,2 @@
+// Ensure no regression.
+"use strict";
diff --git a/tests/expression_statement/use_strict.js b/tests/expression_statement/use_strict.js
new file mode 100755
index 000000000..31bce8687
--- /dev/null
+++ b/tests/expression_statement/use_strict.js
@@ -0,0 +1,2 @@
+// Parentheses around expression statement should be preserved in this case.
+("use strict");
diff --git a/tests/flow/binding/__snapshots__/jsfmt.spec.js.snap b/tests/flow/binding/__snapshots__/jsfmt.spec.js.snap
index 239ca9dc4..495adf455 100644
--- a/tests/flow/binding/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/flow/binding/__snapshots__/jsfmt.spec.js.snap
@@ -416,13 +416,13 @@ function var_var() {
 
 function function_toplevel() {
   function a() {}
-  function a() {}
+  function a() {} // OK
 }
 
 function function_block() {
   {
     function a() {}
-    function a() {}
+    function a() {} // error: name already bound
   }
 }
 
diff --git a/tests/flow/constructor_annots/__snapshots__/jsfmt.spec.js.snap b/tests/flow/constructor_annots/__snapshots__/jsfmt.spec.js.snap
index e3ff68427..d1f66e222 100644
--- a/tests/flow/constructor_annots/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/flow/constructor_annots/__snapshots__/jsfmt.spec.js.snap
@@ -38,7 +38,7 @@ exports.Foo = Foo;
 
 // so you want to type Foo, by declaring it as a class
 interface IFooPrototype {
-  m(): number
+  m: () => number
 }
 interface IFoo extends IFooPrototype {
   static (): void,
diff --git a/tests/flow/object-method/__snapshots__/jsfmt.spec.js.snap b/tests/flow/object-method/__snapshots__/jsfmt.spec.js.snap
index ba57ea5cc..13d76e253 100644
--- a/tests/flow/object-method/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/flow/object-method/__snapshots__/jsfmt.spec.js.snap
@@ -129,7 +129,7 @@ function bar(f: () => void) {
 
 bar(foo); // error, since \`this\` is used non-trivially in \`foo\`
 
-function qux(o: { f(): void }) {
+function qux(o: { f: () => void }) {
   o.f(); // passing o as \`this\`
 }
 
diff --git a/tests/flow/refinements/__snapshots__/jsfmt.spec.js.snap b/tests/flow/refinements/__snapshots__/jsfmt.spec.js.snap
index 5d8d537ba..e569f13dd 100644
--- a/tests/flow/refinements/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/flow/refinements/__snapshots__/jsfmt.spec.js.snap
@@ -43,7 +43,7 @@ function foo(x: ?number) {
 
 type Bar = {
   parent: ?Bar,
-  doStuff(): void
+  doStuff: () => void
 };
 
 function bar0(x: Bar) {
diff --git a/tests/flow/taint/__snapshots__/jsfmt.spec.js.snap b/tests/flow/taint/__snapshots__/jsfmt.spec.js.snap
index 856977b2f..5a9b44db6 100644
--- a/tests/flow/taint/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/flow/taint/__snapshots__/jsfmt.spec.js.snap
@@ -151,13 +151,13 @@ function foo(x: $Tainted<string>, o: Object) {
   // Error
   o.f(x);
 }
-function foo1(x: $Tainted<string>, o: { f(y: $Tainted<string>): void }) {
+function foo1(x: $Tainted<string>, o: { f: (y: $Tainted<string>) => void }) {
   o.f(x);
 }
 function foo2(o1: Object, o2: { t: $Tainted<string> }) {
   o1.f(o2.t);
 }
-function foo3<T>(x: $Tainted<T>, o: { f(y: $Tainted<T>): void }) {
+function foo3<T>(x: $Tainted<T>, o: { f: (y: $Tainted<T>) => void }) {
   o.f(x);
 }
 function f_foo1(x: $Tainted<string>, f: Function) {
diff --git a/tests/flow/type_only_vars/__snapshots__/jsfmt.spec.js.snap b/tests/flow/type_only_vars/__snapshots__/jsfmt.spec.js.snap
index 119c8288b..bedfebb27 100644
--- a/tests/flow/type_only_vars/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/flow/type_only_vars/__snapshots__/jsfmt.spec.js.snap
@@ -57,7 +57,7 @@ import typeof A from \"./A.js\";
 import type { Foo, Bar as Baz } from \"./A.js\";
 
 type duck = {
-  quack(): string
+  quack: () => string
 };
 
 // These string types should confict with the imported types
diff --git a/tests/flow_method/__snapshots__/jsfmt.spec.js.snap b/tests/flow_method/__snapshots__/jsfmt.spec.js.snap
new file mode 100644
index 000000000..59d94ddce
--- /dev/null
+++ b/tests/flow_method/__snapshots__/jsfmt.spec.js.snap
@@ -0,0 +1,14 @@
+exports[`test method.js 1`] = `
+"type T = { method: () => void };
+type T = { method(): void };
+declare class X { method(): void }
+declare function f(): void;
+var f: () => void;
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+type T = { method: () => void };
+type T = { method(): void };
+declare class X { method(): void }
+declare function f(): void;
+var f: () => void;
+"
+`;
diff --git a/tests/flow_method/jsfmt.spec.js b/tests/flow_method/jsfmt.spec.js
new file mode 100644
index 000000000..989047bcc
--- /dev/null
+++ b/tests/flow_method/jsfmt.spec.js
@@ -0,0 +1,1 @@
+run_spec(__dirname);
diff --git a/tests/flow_method/method.js b/tests/flow_method/method.js
new file mode 100644
index 000000000..405f3fe2b
--- /dev/null
+++ b/tests/flow_method/method.js
@@ -0,0 +1,5 @@
+type T = { method: () => void };
+type T = { method(): void };
+declare class X { method(): void }
+declare function f(): void;
+var f: () => void;
diff --git a/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap b/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap
index d298b8359..773b687b3 100644
--- a/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap
+++ b/tests/last_argument_expansion/__snapshots__/jsfmt.spec.js.snap
@@ -21,7 +21,7 @@ export default function searchUsers(action$) {
     .map(action => action.payload.query)
     .filter(q => !!q)
     .switchMap(q =>
-      Observable.timer(800)
+      Observable.timer(800) // debounce
         .takeUntil(action$.ofType(ActionTypes.CLEARED_SEARCH_RESULTS))
         .mergeMap(() =>
           Observable.merge(