Heuristic for grouping parameters in method signatures (#10357)

* Heuristic for grouping parameters in method signatures
* Apply the same logic to function declaration and Flow types
* Group only if there is only one parameter
* Don't group if type param has constraints/default or >1 type param

changelog_unreleased/typescript/10024.md

@@ -1,4 +1,4 @@
-#### Fix unnecessary line breaks in method type declaration parameters (#10024 by @sosukesuzuki)
+#### Fix unnecessary line breaks in method type declaration parameters (#10024 by @sosukesuzuki, #10357 by @thorn0)
 
 <!-- prettier-ignore -->
 ```js

src/language-js/print/function-parameters.js

@@ -4,7 +4,7 @@ const { getNextNonSpaceNonCommentCharacter } = require("../../common/util");
 const { printDanglingComments } = require("../../main/comments");
 const {
   builders: { line, hardline, softline, group, indent, ifBreak },
-  utils: { removeLines },
+  utils: { removeLines, willBreak },
 } = require("../../document");
 const {
   getFunctionParameters,
@@ -189,4 +189,48 @@ function shouldHugFunctionParameters(node) {
   );
 }
 
-module.exports = { printFunctionParameters, shouldHugFunctionParameters };
+function getReturnTypeNode(functionNode) {
+  let returnTypeNode;
+  if (functionNode.returnType) {
+    returnTypeNode = functionNode.returnType;
+    if (returnTypeNode.typeAnnotation) {
+      returnTypeNode = returnTypeNode.typeAnnotation;
+    }
+  } else if (functionNode.typeAnnotation) {
+    returnTypeNode = functionNode.typeAnnotation;
+  }
+  return returnTypeNode;
+}
+
+// When parameters are grouped, the return type annotation breaks first.
+function shouldGroupFunctionParameters(functionNode, returnTypeDoc) {
+  const returnTypeNode = getReturnTypeNode(functionNode);
+  if (!returnTypeNode) {
+    return false;
+  }
+
+  const typeParameters =
+    functionNode.typeParameters && functionNode.typeParameters.params;
+  if (typeParameters) {
+    if (typeParameters.length > 1) {
+      return false;
+    }
+    if (typeParameters.length === 1) {
+      const typeParameter = typeParameters[0];
+      if (typeParameter.constraint || typeParameter.default) {
+        return false;
+      }
+    }
+  }
+
+  return (
+    getFunctionParameters(functionNode).length === 1 &&
+    (isObjectType(returnTypeNode) || willBreak(returnTypeDoc))
+  );
+}
+
+module.exports = {
+  printFunctionParameters,
+  shouldHugFunctionParameters,
+  shouldGroupFunctionParameters,
+};

src/language-js/print/function.js

@@ -25,7 +25,10 @@ const {
   CommentCheckFlags,
 } = require("../utils");
 const { locEnd } = require("../loc");
-const { printFunctionParameters } = require("./function-parameters");
+const {
+  printFunctionParameters,
+  shouldGroupFunctionParameters,
+} = require("./function-parameters");
 const { printPropertyKey } = require("./property");
 const { printFunctionTypeParameters } = require("./misc");
 
@@ -53,11 +56,20 @@ function printFunctionDeclaration(path, print, options, expandArg) {
     parts.push(path.call(print, "id"));
   }
 
+  const parametersDoc = printFunctionParameters(
+    path,
+    print,
+    options,
+    expandArg
+  );
+  const returnTypeDoc = printReturnType(path, print, options);
+  const shouldGroupParameters = shouldGroupFunctionParameters(n, returnTypeDoc);
+
   parts.push(
     printFunctionTypeParameters(path, options, print),
     group([
-      printFunctionParameters(path, print, options, expandArg),
-      printReturnType(path, print, options),
+      shouldGroupParameters ? group(parametersDoc) : parametersDoc,
+      returnTypeDoc,
     ]),
     n.body ? " " : "",
     path.call(print, "body")
@@ -110,15 +122,22 @@ function printMethod(path, options, print) {
 }
 
 function printMethodInternal(path, options, print) {
+  const node = path.getNode();
+  const parametersDoc = printFunctionParameters(path, print, options);
+  const returnTypeDoc = printReturnType(path, print, options);
+  const shouldGroupParameters = shouldGroupFunctionParameters(
+    node,
+    returnTypeDoc
+  );
   const parts = [
     printFunctionTypeParameters(path, options, print),
     group([
-      printFunctionParameters(path, print, options),
-      printReturnType(path, print, options),
+      shouldGroupParameters ? group(parametersDoc) : parametersDoc,
+      returnTypeDoc,
     ]),
   ];
 
-  if (path.getNode().body) {
+  if (node.body) {
     parts.push(" ", path.call(print, "body"));
   } else {
     parts.push(options.semi ? ";" : "");

src/language-js/print/type-annotation.js

@@ -15,7 +15,10 @@ const {
   shouldPrintComma,
 } = require("../utils");
 const { printAssignment } = require("./assignment");
-const { printFunctionParameters } = require("./function-parameters");
+const {
+  printFunctionParameters,
+  shouldGroupFunctionParameters,
+} = require("./function-parameters");
 const { printArrayItems } = require("./array");
 
 function shouldHugType(node) {
@@ -244,26 +247,34 @@ function printFunctionType(path, options, print) {
     parts.push("(");
   }
 
-  parts.push(
-    printFunctionParameters(
-      path,
-      print,
-      options,
-      /* expandArg */ false,
-      /* printTypeParams */ true
-    )
+  const parametersDoc = printFunctionParameters(
+    path,
+    print,
+    options,
+    /* expandArg */ false,
+    /* printTypeParams */ true
   );
 
   // The returnType is not wrapped in a TypeAnnotation, so the colon
   // needs to be added separately.
-  if (n.returnType || n.predicate || n.typeAnnotation) {
-    parts.push(
-      isArrowFunctionTypeAnnotation ? " => " : ": ",
-      path.call(print, "returnType"),
-      path.call(print, "predicate"),
-      path.call(print, "typeAnnotation")
-    );
+  const returnTypeDoc =
+    n.returnType || n.predicate || n.typeAnnotation
+      ? [
+          isArrowFunctionTypeAnnotation ? " => " : ": ",
+          path.call(print, "returnType"),
+          path.call(print, "predicate"),
+          path.call(print, "typeAnnotation"),
+        ]
+      : "";
+
+  const shouldGroupParameters = shouldGroupFunctionParameters(n, returnTypeDoc);
+
+  parts.push(shouldGroupParameters ? group(parametersDoc) : parametersDoc);
+
+  if (returnTypeDoc) {
+    parts.push(returnTypeDoc);
   }
+
   if (needsParens) {
     parts.push(")");
   }

src/language-js/print/typescript.js

@@ -25,7 +25,10 @@ const { locStart, locEnd } = require("../loc");
 
 const { printOptionalToken, printTypeScriptModifiers } = require("./misc");
 const { printTernary } = require("./ternary");
-const { printFunctionParameters } = require("./function-parameters");
+const {
+  printFunctionParameters,
+  shouldGroupFunctionParameters,
+} = require("./function-parameters");
 const { printTemplateLiteral } = require("./template-literal");
 const { printArrayItems } = require("./array");
 const { printObject } = require("./object");
@@ -349,37 +352,46 @@ function printTypescript(path, options, print) {
         { shouldBreak }
       );
     }
-    case "TSMethodSignature":
+    case "TSMethodSignature": {
       parts.push(
-        group([
-          n.accessibility ? [n.accessibility, " "] : "",
-          n.export ? "export " : "",
-          n.static ? "static " : "",
-          n.readonly ? "readonly " : "",
-          n.computed ? "[" : "",
-          path.call(print, "key"),
-          n.computed ? "]" : "",
-          printOptionalToken(path),
-          printFunctionParameters(
-            path,
-            print,
-            options,
-            /* expandArg */ false,
-            /* printTypeParams */ true
-          ),
-        ])
+        n.accessibility ? [n.accessibility, " "] : "",
+        n.export ? "export " : "",
+        n.static ? "static " : "",
+        n.readonly ? "readonly " : "",
+        n.computed ? "[" : "",
+        path.call(print, "key"),
+        n.computed ? "]" : "",
+        printOptionalToken(path)
       );
 
-      if (n.returnType || n.typeAnnotation) {
-        parts.push(
-          group([
-            ": ",
-            path.call(print, "returnType"),
-            path.call(print, "typeAnnotation"),
-          ])
-        );
+      const parametersDoc = printFunctionParameters(
+        path,
+        print,
+        options,
+        /* expandArg */ false,
+        /* printTypeParams */ true
+      );
+
+      const returnTypePropertyName = n.returnType
+        ? "returnType"
+        : "typeAnnotation";
+      const returnTypeNode = n[returnTypePropertyName];
+      const returnTypeDoc = returnTypeNode
+        ? path.call(print, returnTypePropertyName)
+        : "";
+      const shouldGroupParameters = shouldGroupFunctionParameters(
+        n,
+        returnTypeDoc
+      );
+
+      parts.push(shouldGroupParameters ? group(parametersDoc) : parametersDoc);
+
+      if (returnTypeNode) {
+        parts.push(": ", group(returnTypeDoc));
       }
+
       return group(parts);
+    }
     case "TSNamespaceExportDeclaration":
       parts.push("export as namespace ", path.call(print, "id"));
 

tests/flow-repo/dictionary/__snapshots__/jsfmt.spec.js.snap

@@ -56,9 +56,10 @@ function foo0(
   return x;
 }
 
-function foo2(x: {
+function foo2(x: { [key: string]: number }): {
   [key: string]: number,
-}): { [key: string]: number, +toString: () => string } {
+  +toString: () => string,
+} {
   // x's prototype has a toString method
   return x;
 }
@@ -468,9 +469,10 @@ function mix_with_declared_props(o: { [k: number]: X, p: Y }, x: X, y: Y) {
 }
 
 // Indeed, dict types are still Objects and have Object.prototype stuff
-function object_prototype(o: {
+function object_prototype(o: { [k: string]: number }): {
   [k: string]: number,
-}): { [k: string]: number, +toString: () => string } {
+  +toString: () => string,
+} {
   (o.toString(): boolean); // error: string ~> boolean
   return o; // ok
 }
@@ -776,9 +778,10 @@ function foo3(x: { [key: string]: number }): { foo: number } {
 }
 
 // error: foo can't exist in x
-function foo4(x: {
+function foo4(x: { [key: string]: number }): {
   [key: string]: number,
-}): { [key: string]: number, foo: string } {
+  foo: string,
+} {
   return x;
 }
 

tests/flow-repo/missing_annotation/__snapshots__/jsfmt.spec.js.snap

@@ -132,9 +132,7 @@ var Foo = {
     };
   },
 
-  d: function (
-    arg: string
-  ): {
+  d: function (arg: string): {
     bar: string,
   } {
     return {

tests/flow-repo/spread/__snapshots__/jsfmt.spec.js.snap

@@ -46,9 +46,7 @@ function parseCounter(line: string): number {
   return 0;
 }
 
-function parseGroup(
-  lines: Array<string>
-): {
+function parseGroup(lines: Array<string>): {
   counter: number,
   begin: number,
   end: number,

tests/flow/method/__snapshots__/jsfmt.spec.js.snap

@@ -24,6 +24,63 @@ type Foo = {
 ================================================================================
 `;
 
+exports[`consistent-breaking.js format 1`] = `
+====================================options=====================================
+parsers: ["flow", "babel"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+class Foo1 {
+  flowParseFunctionTypeParams(
+    params: N.FlowFunctionTypeParam[] = [],
+  ): { params: N.FlowFunctionTypeParam[], rest: ?N.FlowFunctionTypeParam } {
+    // ...
+  }
+}
+
+type Foo2 = {
+  flowParseFunctionTypeParams(
+    params: N.FlowFunctionTypeParam[]
+  ): { params: N.FlowFunctionTypeParam[], rest: ?N.FlowFunctionTypeParam }
+}
+
+{
+  function flowParseFunctionTypeParams(
+    params: N.FlowFunctionTypeParam[]
+  ): { params: N.FlowFunctionTypeParam[], rest: ?N.FlowFunctionTypeParam } {
+    // ...
+  }
+}
+
+=====================================output=====================================
+class Foo1 {
+  flowParseFunctionTypeParams(params: N.FlowFunctionTypeParam[] = []): {
+    params: N.FlowFunctionTypeParam[],
+    rest: ?N.FlowFunctionTypeParam,
+  } {
+    // ...
+  }
+}
+
+type Foo2 = {
+  flowParseFunctionTypeParams(params: N.FlowFunctionTypeParam[]): {
+    params: N.FlowFunctionTypeParam[],
+    rest: ?N.FlowFunctionTypeParam,
+  },
+};
+
+{
+  function flowParseFunctionTypeParams(params: N.FlowFunctionTypeParam[]): {
+    params: N.FlowFunctionTypeParam[],
+    rest: ?N.FlowFunctionTypeParam,
+  } {
+    // ...
+  }
+}
+
+================================================================================
+`;
+
 exports[`method.js format 1`] = `
 ====================================options=====================================
 parsers: ["flow", "babel"]

tests/flow/method/consistent-breaking.js

@@ -0,0 +1,21 @@
+class Foo1 {
+  flowParseFunctionTypeParams(
+    params: N.FlowFunctionTypeParam[] = [],
+  ): { params: N.FlowFunctionTypeParam[], rest: ?N.FlowFunctionTypeParam } {
+    // ...
+  }
+}
+
+type Foo2 = {
+  flowParseFunctionTypeParams(
+    params: N.FlowFunctionTypeParam[]
+  ): { params: N.FlowFunctionTypeParam[], rest: ?N.FlowFunctionTypeParam }
+}
+
+{
+  function flowParseFunctionTypeParams(
+    params: N.FlowFunctionTypeParam[]
+  ): { params: N.FlowFunctionTypeParam[], rest: ?N.FlowFunctionTypeParam } {
+    // ...
+  }
+}

tests/typescript/generic/__snapshots__/jsfmt.spec.js.snap

@@ -181,3 +181,43 @@ export default {
 
 ================================================================================
 `;
+
+exports[`ungrouped-parameters.ts format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+function filterTooltipWithFoo<F extends Field>(oldEncoding: Encoding<F>): {
+  customTooltipWithoutAggregatedField?:
+    | StringFieldDefWithCondition<F>
+    | StringValueDefWithCondition<F>
+    | StringFieldDef<F>[];
+  filteredEncoding: Encoding<F>;
+} {
+  const {tooltip, ...filteredEncoding} = oldEncoding;
+  if (!tooltip) {
+    return {filteredEncoding};
+  }
+  // ...
+}
+
+=====================================output=====================================
+function filterTooltipWithFoo<F extends Field>(
+  oldEncoding: Encoding<F>
+): {
+  customTooltipWithoutAggregatedField?:
+    | StringFieldDefWithCondition<F>
+    | StringValueDefWithCondition<F>
+    | StringFieldDef<F>[];
+  filteredEncoding: Encoding<F>;
+} {
+  const { tooltip, ...filteredEncoding } = oldEncoding;
+  if (!tooltip) {
+    return { filteredEncoding };
+  }
+  // ...
+}
+
+================================================================================
+`;

tests/typescript/generic/ungrouped-parameters.ts

@@ -0,0 +1,13 @@
+function filterTooltipWithFoo<F extends Field>(oldEncoding: Encoding<F>): {
+  customTooltipWithoutAggregatedField?:
+    | StringFieldDefWithCondition<F>
+    | StringValueDefWithCondition<F>
+    | StringFieldDef<F>[];
+  filteredEncoding: Encoding<F>;
+} {
+  const {tooltip, ...filteredEncoding} = oldEncoding;
+  if (!tooltip) {
+    return {filteredEncoding};
+  }
+  // ...
+}

tests/typescript/method/__snapshots__/jsfmt.spec.js.snap

@@ -1,5 +1,55 @@
 // Jest Snapshot v1, https://goo.gl/fbAQLP
 
+exports[`issue-10352-consistency.ts format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+export interface Store {
+  getRecord(collectionName: string, documentPath: string): TaskEither<Error, Option<GenericRecord>>;
+}
+
+export default class StoreImpl extends Service implements Store {
+  getRecord(collectionName: string, documentPath: string): TaskEither<Error, Option<GenericRecord>> {
+    // Do some stuff.
+  }
+}
+
+export function loadPlugin(
+  name: string,
+  dirname: string,
+): { filepath: string, value: mixed } {
+  // ...
+}
+
+=====================================output=====================================
+export interface Store {
+  getRecord(
+    collectionName: string,
+    documentPath: string
+  ): TaskEither<Error, Option<GenericRecord>>;
+}
+
+export default class StoreImpl extends Service implements Store {
+  getRecord(
+    collectionName: string,
+    documentPath: string
+  ): TaskEither<Error, Option<GenericRecord>> {
+    // Do some stuff.
+  }
+}
+
+export function loadPlugin(
+  name: string,
+  dirname: string
+): { filepath: string; value: mixed } {
+  // ...
+}
+
+================================================================================
+`;
+
 exports[`method-signature.ts format 1`] = `
 ====================================options=====================================
 parsers: ["typescript"]
@@ -44,6 +94,40 @@ type Bar = {
 ================================================================================
 `;
 
+exports[`method-signature-with-wrapped-return-type.ts format 1`] = `
+====================================options=====================================
+parsers: ["typescript"]
+printWidth: 80
+                                                                                | printWidth
+=====================================input======================================
+type ReleaseToolConfig = {
+  get(key: "changelog"): {
+    get(key: "repo"): string;
+    get(key: "labels"): Map<string, string>;
+  };
+};
+
+type ReleaseToolConfig2 = {
+  get(key: "changelog"): \`
+  \`
+};
+
+=====================================output=====================================
+type ReleaseToolConfig = {
+  get(key: "changelog"): {
+    get(key: "repo"): string;
+    get(key: "labels"): Map<string, string>;
+  };
+};
+
+type ReleaseToolConfig2 = {
+  get(key: "changelog"): \`
+  \`;
+};
+
+================================================================================
+`;
+
 exports[`semi.ts format 1`] = `
 ====================================options=====================================
 parsers: ["typescript"]

tests/typescript/method/issue-10352-consistency.ts

@@ -0,0 +1,16 @@
+export interface Store {
+  getRecord(collectionName: string, documentPath: string): TaskEither<Error, Option<GenericRecord>>;
+}
+
+export default class StoreImpl extends Service implements Store {
+  getRecord(collectionName: string, documentPath: string): TaskEither<Error, Option<GenericRecord>> {
+    // Do some stuff.
+  }
+}
+
+export function loadPlugin(
+  name: string,
+  dirname: string,
+): { filepath: string, value: mixed } {
+  // ...
+}

tests/typescript/method/method-signature-with-wrapped-return-type.ts

@@ -0,0 +1,11 @@
+type ReleaseToolConfig = {
+  get(key: "changelog"): {
+    get(key: "repo"): string;
+    get(key: "labels"): Map<string, string>;
+  };
+};
+
+type ReleaseToolConfig2 = {
+  get(key: "changelog"): `
+  `
+};