Handle @param blocks

1. Format multiple params nicely in a single-multiline block. 2. Generate only params that haven't already been documented. Existing documentation is not touched.
microsoft · sandersn · Oct 9, 2018 · Sep 28, 2018 · Sep 28, 2018 · Oct 1, 2018
commit 69dec3f7ad975ff94a0587e3ce8593f4f36a6d18
diff --git a/src/services/codefixes/inferFromUsage.ts b/src/services/codefixes/inferFromUsage.ts
@@ -165,25 +165,51 @@ namespace ts.codefix {
             return;
         }
 
-        zipWith<ParameterDeclaration, [Type | undefined, boolean], void>(containingFunction.parameters, types, (parameter, [type, isOptionalParameter]) => {
-            if (!(parameter.type || isInJSFile(parameter) && getJSDocType(parameter)) && !parameter.initializer) {
-                annotate(changes, sourceFile, parameter, type, program, isOptionalParameter);
+        if (isInJSFile(containingFunction)) {
+            annotateJSDocParameters(changes, sourceFile, containingFunction.parameters, types, program);
+        }
+        else {
+            zipWith<ParameterDeclaration, [Type | undefined, boolean], void>(containingFunction.parameters, types, (parameter, typair) => {
+                if (!parameter.type && !parameter.initializer) {
+                    annotate(changes, sourceFile, parameter, typair[0], program);
+                }
+            });
+        }
+    }
+
+    function annotateJSDocParameters(changes: textChanges.ChangeTracker, sourceFile: SourceFile, params: NodeArray<ParameterDeclaration>, types: [Type | undefined, boolean][], program: Program): void {
+        const triples = [];
+        for (let i = 0; i < params.length; i++) {
+            const param = params[i];
+            const [t, opt] = types[i];
+            const typeNode = t && getTypeNodeIfAccessible(t, param, program.getTypeChecker());
+            if (!(param.type || isInJSFile(param) && getJSDocType(param)) && !param.initializer && typeNode) {
+                triples.push([param, typeNode, opt] as [ParameterDeclaration, TypeNode, boolean]);
             }
-        });
+        }
+        changes.tryInsertJSDocParams(sourceFile, triples);
     }
 
     function annotateSetAccessor(changes: textChanges.ChangeTracker, sourceFile: SourceFile, setAccessorDeclaration: SetAccessorDeclaration, program: Program, cancellationToken: CancellationToken): void {
         const param = firstOrUndefined(setAccessorDeclaration.parameters);
         if (param && isIdentifier(setAccessorDeclaration.name) && isIdentifier(param.name)) {
             const type = inferTypeForVariableFromUsage(setAccessorDeclaration.name, program, cancellationToken) ||
                 inferTypeForVariableFromUsage(param.name, program, cancellationToken);
-            annotate(changes, sourceFile, param, type, program);
+            if (isInJSFile(setAccessorDeclaration)) {
+                const typeNode = type && getTypeNodeIfAccessible(type, param, program.getTypeChecker());
+                if (typeNode) {
+                    changes.tryInsertJSDocParams(sourceFile, [[param, typeNode, /*isOptionalParameter*/ false]]);
+                }
+            }
+            else {
+                annotate(changes, sourceFile, param, type, program);
+            }
         }
     }
 
-    function annotate(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: textChanges.TypeAnnotatable, type: Type | undefined, program: Program, isOptionalParameter?: boolean): void {
+    function annotate(changes: textChanges.ChangeTracker, sourceFile: SourceFile, declaration: textChanges.TypeAnnotatable, type: Type | undefined, program: Program): void {
         const typeNode = type && getTypeNodeIfAccessible(type, declaration, program.getTypeChecker());
-        if (typeNode) changes.tryInsertTypeAnnotation(sourceFile, declaration, typeNode, isOptionalParameter);
+        if (typeNode) changes.tryInsertTypeAnnotation(sourceFile, declaration, typeNode);
     }
 
     function getTypeNodeIfAccessible(type: Type, enclosingScope: Node, checker: TypeChecker): TypeNode | undefined {

diff --git a/src/services/textChanges.ts b/src/services/textChanges.ts
@@ -365,10 +365,24 @@ namespace ts.textChanges {
             this.replaceRangeWithText(sourceFile, createRange(pos), text);
         }
 
+        public tryInsertJSDocParams(sourceFile: SourceFile, annotations: [ParameterDe
8000
claration, TypeNode, boolean][]) {
+            const parent = annotations[0][0].parent;
+            const indent = getLineAndCharacterOfPosition(sourceFile, parent.getStart()).character;
+            let commentText = "\n";
+            for (const [node, type, isOptionalParameter] of annotations) {
+                if (isIdentifier(node.name)) {
+                    const printed = createPrinter().printNode(EmitHint.Unspecified, type, sourceFile);
+                    commentText += this.printJSDocParameter(indent, printed, node.name, isOptionalParameter);
+                }
+            }
+            commentText += " ".repeat(indent + 1);
+            this.insertCommentThenNewline(sourceFile, indent, parent.getStart(), commentText, CommentKind.Jsdoc);
+        }
+
         /** Prefer this over replacing a node with another that has a type annotation, as it avoids reformatting the other parts of the node. */
-        public tryInsertTypeAnnotation(sourceFile: SourceFile, topNode: TypeAnnotatable, type: TypeNode, isOptionalParameter?: boolean): void {
+        public tryInsertTypeAnnotation(sourceFile: SourceFile, topNode: TypeAnnotatable, type: TypeNode): void {
             if (isInJSFile(sourceFile) && topNode.kind !== SyntaxKind.PropertySignature) {
-                return this.tryInsertJSDocType(sourceFile, topNode, type, isOptionalParameter);
+                return this.tryInsertJSDocType(sourceFile, topNode, type);
             }
             const node = topNode as SignatureDeclaration | VariableDeclaration | ParameterDeclaration | PropertyDeclaration | PropertySignature;
             let endNode: Node | undefined;
@@ -391,16 +405,10 @@ namespace ts.textChanges {
          * TODO: Might need to only disallow node from being a parameterdecl, propertydecl, propertysig
          * (or correctly handle parameterdecl by walking up and adding a param, at least)
          */
-        public tryInsertJSDocType(sourceFile: SourceFile, node: Node, type: TypeNode, isOptionalParameter: boolean | undefined): void {
-            // TODO: Parameter code needs to be MUCH smarter (multiple parameters are ugly, etc)
-            // TODO: Parameter probably shouldn't need to manually unescape its text
+        public tryInsertJSDocType(sourceFile: SourceFile, node: Node, type: TypeNode): void {
             const printed = createPrinter().printNode(EmitHint.Unspecified, type, sourceFile);
             let commentText;
-            if (isParameter(node) && isIdentifier(node.name)) {
-                commentText = this.printJSDocParameter(printed, node.name, isOptionalParameter);
-                node = node.parent;
-            }
-            else if (isGetAccessorDeclaration(node)) {
+            if (isGetAccessorDeclaration(node)) {
                 commentText = ` @return {${printed}} `;
             }
             else {
@@ -411,12 +419,12 @@ namespace ts.textChanges {
             // this.replaceRangeWithText <-- SOMEDAY, when we support existing ones
         }
 
-        private printJSDocParameter(printed: string, name: Identifier, isOptionalParameter: boolean | undefined) {
+        private printJSDocParameter(indent: number, printed: string, name: Identifier, isOptionalParameter: boolean | undefined) {
             let printName = unescapeLeadingUnderscores(name.escapedText);
             if (isOptionalParameter) {
                 printName = `[${printName}]`;
             }
-            return ` @param {${printed}} ${printName} `;
+            return " ".repeat(indent) + ` * @param {${printed}} ${printName}\n`;
         }
 
         public insertTypeParameters(sourceFile: SourceFile, node: SignatureDeclaration, typeParameters: ReadonlyArray<TypeParameterDeclaration>): void {

diff --git a/tests/cases/fourslash/codeFixInferFromUsageCallJS.ts b/tests/cases/fourslash/codeFixInferFromUsageCallJS.ts
@@ -12,7 +12,9 @@ verify.codeFixAll({
     fixId: "inferFromUsage",
     fixAllDescription: "Infer all types from usage",
     newFileContent:
-`/** @param {() => void} b */
+`/**
+ * @param {() => void} b
+ */
 function wat(b) {
     b();
 }`});
diff --git a/tests/cases/fourslash/codeFixInferFromUsageMultipleParametersJS.ts b/tests/cases/fourslash/codeFixInferFromUsageMultipleParametersJS.ts
@@ -11,11 +11,13 @@
 
 
 verify.fileAfterCodeFix(
-`/** @param {number} a */
-/** @param {string} b */
-/** @param {{ a: number; }} c */
-/** @param {{ shouldNotBeHere: number; }} d */
-/** @param {(string | number)[]} d */
+`/**
+ * @param {number} a
+ * @param {string} b
+ * @param {{ a: number; }} c
+ * @param {{ shouldNotBeHere: number; }} d
+ * @param {(string | number)[]} d
+ */
 function f(a, b, c, d, e = 0, ...d ) {
 }
 f(1, "string", { a: 1 }, {shouldNotBeHere: 2}, {shouldNotBeHere: 2}, 3, "string");`, undefined, 6);
diff --git a/tests/cases/fourslash/codeFixInferFromUsageNumberIndexSignatureJS.ts b/tests/cases/fourslash/codeFixInferFromUsageNumberIndexSignatureJS.ts
@@ -9,7 +9,9 @@
 ////}
 
 verify.fileAfterCodeFix(
-`/** @param {number[]} a */
+`/**
+ * @param {number[]} a
+ */
 function f(a) {
     return a[0] + 1;
 }`, undefined, 2);
diff --git a/tests/cases/fourslash/codeFixInferFromUsageOptionalParamJS.ts b/tests/cases/fourslash/codeFixInferFromUsageOptionalParamJS.ts
@@ -11,7 +11,9 @@
 ////f(1);
 
 verify.fileAfterCodeFix(
-`/** @param {number} [a] */
+`/**
+ * @param {number} [a]
+ */
 function f(a) {
     a;
 }

diff --git a/tests/cases/fourslash/codeFixInferFromUsagePartialParameterListJS.ts b/tests/cases/fourslash/codeFixInferFromUsagePartialParameterListJS.ts
@@ -0,0 +1,29 @@
+/// <reference path='fourslash.ts' />
+
+// @allowJs: true
+// @checkJs: true
+// @noImplicitAny: true
+// @strictNullChecks: true
+// @Filename: important.js
+/////**
+//// * @param {*} y
+//// */
+////function f(x, y, z) {
+////    return x
+////}
+////f(1, 2, 3)
+
+verify.fileAfterCodeFix(
+`
+/**
+ * @param {*} y
+ */
+/**
+ * @param {number} x
+ * @param {number} z
+ */
+function f(x, y, z) {
+    return x
+}
+f(1, 2, 3)
+`, undefined, 2);
diff --git a/tests/cases/fourslash/codeFixInferFromUsagePropertyAccessJS.ts b/tests/cases/fourslash/codeFixInferFromUsagePropertyAccessJS.ts
@@ -17,9 +17,11 @@
 ////}
 
 verify.fileAfterCodeFix(
-`/** @param {{ b: { c: any; }; }} a */
-/** @param {{ n: () => number; }} m */
-/** @param {{ y: { z: number[]; }; }} x */
+`/**
+ * @param {{ b: { c: any; }; }} a
+ * @param {{ n: () => number; }} m
+ * @param {{ y: { z: number[]; }; }} x
+ */
 function foo(a, m, x) {
     a.b.c;
 

diff --git a/tests/cases/fourslash/codeFixInferFromUsageRestParam2JS.ts b/tests/cases/fourslash/codeFixInferFromUsageRestParam2JS.ts
@@ -15,7 +15,9 @@
 
 verify.fileAfterCodeFix(
 `/** @param {number} a */
-/** @param {(string | boolean)[]} rest */
+/**
+ * @param {(string | boolean)[]} rest
+ */
 function f(a, ...rest){
     a; rest;
 }

diff --git a/tests/cases/fourslash/codeFixInferFromUsageRestParam3JS.ts b/tests/cases/fourslash/codeFixInferFromUsageRestParam3JS.ts
@@ -12,7 +12,9 @@
 
 verify.fileAfterCodeFix(
 `/** @param {number} a */
-/** @param {number[]} rest */
+/**
+ * @param {number[]} rest
+ */
 function f(a, ...rest){
     a;
     rest.push(22);

diff --git a/tests/cases/fourslash/codeFixInferFromUsageRestParamJS.ts b/tests/cases/fourslash/codeFixInferFromUsageRestParamJS.ts
@@ -15,7 +15,9 @@
 
 verify.fileAfterCodeFix(
 `/** @param {number} a */
-/** @param {string[]} rest */
+/**
+ * @param {string[]} rest
+ */
 function f(a: number, ...rest){
     a; rest;
 }

diff --git a/tests/cases/fourslash/codeFixInferFromUsageSetterJS.ts b/tests/cases/fourslash/codeFixInferFromUsageSetterJS.ts
@@ -14,7 +14,9 @@
 verify.fileAfterCodeFix(
 `
 class C {
-    /** @param {number} v */
+    /**
+     * @param {number} v
+     */
     set x(v) {
         v;
     }

diff --git a/tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts b/tests/cases/fourslash/codeFixInferFromUsageSetterWithInaccessibleTypeJS.ts
@@ -26,7 +26,9 @@ verify.codeFix({
     description: "Infer type of 'x' from usage",
     newFileContent:
 `export class C {
-    /** @param {Promise<typeof import("/a")>} val */
+    /**
+     * @param {Promise<typeof import("/a")>} val
+     */
     set x(val) { val; }
     method() { this.x = import("./a"); }
 }`,

diff --git a/tests/cases/fourslash/codeFixInferFromUsageSingleLineClassJS.ts b/tests/cases/fourslash/codeFixInferFromUsageSingleLineClassJS.ts
@@ -11,7 +11,9 @@
 
 verify.fileAfterCodeFix(
 `
-class C {/** @param {number} x */
+class C {/**
+          * @param {number} x
+          */
          m(x) {return x;}}
 var c = new C()
 c.m(1)`, undefined, 2);
diff --git a/tests/cases/fourslash/codeFixInferFromUsageStringIndexSignatureJS.ts b/tests/cases/fourslash/codeFixInferFromUsageStringIndexSignatureJS.ts
@@ -14,7 +14,9 @@ verify.codeFix({
     index: 2,
     description: "Infer parameter types from usage",
     newFileContent:
-`/** @param {{ [x: string]: any; }} a */
+`/**
+ * @param {{ [x: string]: any; }} a
+ */
 function f(a) {
     return a['hi'];
 }`,

diff --git a/tests/cases/fourslash/codeFixInferFromUsage_allJS.ts b/tests/cases/fourslash/codeFixInferFromUsage_allJS.ts
@@ -24,14 +24,18 @@ verify.codeFixAll({
     fixId: "inferFromUsage",
     fixAllDescription: "Infer all types from usage",
     newFileContent:
-`/** @param {number} x */
-/** @param {string} y */
+`/**
+ * @param {number} x
+ * @param {string} y
+ */
 function f(x, y) {
     x += 0;
     y += "";
 }
 
-/** @param {number} z */
+/**
+ * @param {number} z
+ */
 function g(z) {
     return z * 2;
 }