Repository: jlongster/prettier
Pull request #600 (2017-02-04 04:01:39)

Print CallExpression comments inside of memberChain
I made sure that MemberExpressions where printed correctly but didn't do the same for CallExpression. Now it is! It is a bit less straightforward because the comments for the root CallExpression have already been printed, so we need to extract the first call out.

(This is the last place where we drop comments on the test suite, we can start enabling throwing when comments are dropped!)


Patch:
diff --git a/src/comments.js b/src/comments.js
index bd5c25d7f..67d40de30 100644
--- a/src/comments.js
+++ b/src/comments.js
@@ -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/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(