feat(11378): check param names in JSDoc (microsoft#47257)

Author: a-tarasyuk

src/compiler/checker.ts

@@ -34472,6 +34472,7 @@ namespace ts {
 }
 
 checkTypeParameters(getEffectiveTypeParameterDeclarations(node));
+checkUnmatchedJSDocParameters(node);
 
 forEach(node.parameters, checkParameter);
 
@@ -36235,40 +36236,7 @@ namespace ts {
 
 function checkJSDocParameterTag(node: JSDocParameterTag) {
 checkSourceElement(node.typeExpression);
-if (!getParameterSymbolFromJSDoc(node)) {
-const decl = getHostSignatureFromJSDoc(node);
-// don't issue an error for invalid hosts -- just functions --
-// and give a better error message when the host function mentions `arguments`
-// but the tag doesn't have an array type
-if (decl) {
-const i = getJSDocTags(decl).filter(isJSDocParameterTag).indexOf(node);
-if (i > -1 && i < decl.parameters.length && isBindingPattern(decl.parameters[i].name)) {
-return;
-}
-if (!containsArgumentsReference(decl)) {
-if (isQualifiedName(node.name)) {
-error(node.name,
-Diagnostics.Qualified_name_0_is_not_allowed_without_a_leading_param_object_1,
-entityNameToString(node.name),
-entityNameToString(node.name.left));
-}
-else {
-error(node.name,
-Diagnostics.JSDoc_param_tag_has_name_0_but_there_is_no_parameter_with_that_name,
-idText(node.name));
-}
-}
-else if (findLast(getJSDocTags(decl), isJSDocParameterTag) === node &&
-node.typeExpression && node.typeExpression.type &&
-!isArrayType(getTypeFromTypeNode(node.typeExpression.type))) {
-error(node.name,
-Diagnostics.JSDoc_param_tag_has_name_0_but_there_is_no_parameter_with_that_name_It_would_match_arguments_if_it_had_an_array_type,
-idText(node.name.kind === SyntaxKind.QualifiedName ? node.name.right : node.name));
-}
-}
-}
 }
-
 function checkJSDocPropertyTag(node: JSDocPropertyTag) {
 checkSourceElement(node.typeExpression);
 }
@@ -38564,6 +38532,45 @@ namespace ts {
 }
 }
 
+function checkUnmatchedJSDocParameters(node: SignatureDeclaration) {
+const jsdocParameters = filter(getJSDocTags(node), isJSDocParameterTag);
+if (!length(jsdocParameters)) return;
+
+const isJs = isInJSFile(node);
+const parameters = new Set<__String>();
+const excludedParameters = new Set<number>();
+forEach(node.parameters, ({ name }, index) => {
+if (isIdentifier(name)) {
+parameters.add(name.escapedText);
+}
+if (isBindingPattern(name)) {
+excludedParameters.add(index);
+}
+});
+
+const containsArguments = containsArgumentsReference(node);
+if (containsArguments) {
+const lastJSDocParam = lastOrUndefined(jsdocParameters);
+if (lastJSDocParam && isIdentifier(lastJSDocParam.name) && lastJSDocParam.typeExpression &&
+lastJSDocParam.typeExpression.type && !parameters.has(lastJSDocParam.name.escapedText) && !isArrayType(getTypeFromTypeNode(lastJSDocParam.typeExpression.type))) {
+errorOrSuggestion(isJs, lastJSDocParam.name, Diagnostics.JSDoc_param_tag_has_name_0_but_there_is_no_parameter_with_that_name_It_would_match_arguments_if_it_had_an_array_type, idText(lastJSDocParam.name));
+}
+}
+else {
+forEach(jsdocParameters, ({ name }, index) => {
+if (excludedParameters.has(index) || isIdentifier(name) && parameters.has(name.escapedText)) {
+return;
+}
+if (isQualifiedName(name)) {
+errorOrSuggestion(isJs, name, Diagnostics.Qualified_name_0_is_not_allowed_without_a_leading_param_object_1, entityNameToString(name), entityNameToString(name.left));
+}
+else {
+errorOrSuggestion(isJs, name, Diagnostics.JSDoc_param_tag_has_name_0_but_there_is_no_parameter_with_that_name, idText(name));
+}
+});
+}
+}
+
 /**
 * Check each type parameter and check that type parameters have no duplicate type parameter declarations
 */

src/compiler/diagnosticMessages.json

@@ -7135,6 +7135,18 @@
 "category": "Message",
 "code": 95170
 },
+"Delete unused '@param' tag '{0}'": {
+"category": "Message",
+"code": 95171
+},
+"Delete all unused '@param' tags": {
+"category": "Message",
+"code": 95172
+},
+"Rename '@param' tag name '{0}' to '{1}'": {
+"category": "Message",
+"code": 95173
+},
 
 "No value exists in scope for the shorthand property '{0}'. Either declare one or provide an initializer.": {
 "category": "Error",

src/services/codefixes/fixUnmatchedParameter.ts

@@ -0,0 +1,104 @@
+/* @internal */
+namespace ts.codefix {
+const deleteUnmatchedParameter = "deleteUnmatchedParameter";
+const renameUnmatchedParameter = "renameUnmatchedParameter";
+
+const errorCodes = [
+Diagnostics.JSDoc_param_tag_has_name_0_but_there_is_no_parameter_with_that_name.code,
+];
+
+registerCodeFix({
+fixIds: [deleteUnmatchedParameter, renameUnmatchedParameter],
+errorCodes,
+getCodeActions: function getCodeActionsToFixUnmatchedParameter(context) {
+const { sourceFile, span } = context;
+const actions: CodeFixAction[] = [];
+const info = getInfo(sourceFile, span.start);
+if (info) {
+append(actions, getDeleteAction(context, info));
+append(actions, getRenameAction(context, info));
+return actions;
+}
+return undefined;
+},
+getAllCodeActions: function getAllCodeActionsToFixUnmatchedParameter(context) {
+const tagsToSignature = new Map<SignatureDeclaration, JSDocTag[]>();
+return createCombinedCodeActions(textChanges.ChangeTracker.with(context, changes => {
+eachDiagnostic(context, errorCodes, ({ file, start }) => {
+const info = getInfo(file, start);
+if (info) {
+tagsToSignature.set(info.signature, append(tagsToSignature.get(info.signature), info.jsDocParameterTag));
+}
+});
+
+tagsToSignature.forEach((tags, signature) => {
+if (context.fixId === deleteUnmatchedParameter) {
+const tagsSet = new Set(tags);
+changes.filterJSDocTags(signature.getSourceFile(), signature, t => !tagsSet.has(t));
+}
+});
+}));
+}
+});
+
+function getDeleteAction(context: CodeFixContext, { name, signature, jsDocParameterTag }: Info) {
+const changes = textChanges.ChangeTracker.with(context, changeTracker =>
+changeTracker.filterJSDocTags(context.sourceFile, signature, t => t !== jsDocParameterTag));
+return createCodeFixAction(
+deleteUnmatchedParameter,
+changes,
+[Diagnostics.Delete_unused_param_tag_0, name.getText(context.sourceFile)],
+deleteUnmatchedParameter,
+Diagnostics.Delete_all_unused_param_tags
+);
+}
+
+function getRenameAction(context: CodeFixContext, { name, signature, jsDocParameterTag }: Info) {
+if (!length(signature.parameters)) return undefined;
+
+const sourceFile = context.sourceFile;
+const tags = getJSDocTags(signature);
+const names = new Set<__String>();
+for (const tag of tags) {
+if (isJSDocParameterTag(tag) && isIdentifier(tag.name)) {
+names.add(tag.name.escapedText);
+}
+}
+// @todo - match to all available names instead to the first parameter name
+// @see /codeFixRenameUnmatchedParameter3.ts
+const parameterName = firstDefined(signature.parameters, p =>
+isIdentifier(p.name) && !names.has(p.name.escapedText) ? p.name.getText(sourceFile) : undefined);
+if (parameterName === undefined) return undefined;
+
+const newJSDocParameterTag = factory.updateJSDocParameterTag(
+jsDocParameterTag,
+jsDocParameterTag.tagName,
+factory.createIdentifier(parameterName),
+jsDocParameterTag.isBracketed,
+jsDocParameterTag.typeExpression,
+jsDocParameterTag.isNameFirst,
+jsDocParameterTag.comment
+);
+const changes = textChanges.ChangeTracker.with(context, changeTracker =>
+changeTracker.replaceJSDocComment(sourceFile, signature, map(tags, t => t === jsDocParameterTag ? newJSDocParameterTag : t)));
+return createCodeFixActionWithoutFixAll(renameUnmatchedParameter, changes, [Diagnostics.Rename_param_tag_name_0_to_1, name.getText(sourceFile), parameterName]);
+}
+
+interface Info {
+readonly signature: SignatureDeclaration;
+readonly jsDocParameterTag: JSDocParameterTag;
+readonly name: Identifier;
+}
+
+function getInfo(sourceFile: SourceFile, pos: number): Info | undefined {
+const token = getTokenAtPosition(sourceFile, pos);
+if (token.parent && isJSDocParameterTag(token.parent) && isIdentifier(token.parent.name)) {
+const jsDocParameterTag = token.parent;
+const signature = getHostSignatureFromJSDoc(jsDocParameterTag);
+if (signature) {
+return { signature, name: token.parent.name, jsDocParameterTag };
+}
+}
+return undefined;
+}
+}

src/services/textChanges.ts

@@ -495,29 +495,30 @@ namespace ts.textChanges {
 this.insertNodeAt(sourceFile, fnStart, tag, { preserveLeadingWhitespace: false, suffix: this.newLineCharacter + indent });
 }
 
+private createJSDocText(sourceFile: SourceFile, node: HasJSDoc) {
+const comments = flatMap(node.jsDoc, jsDoc =>
+isString(jsDoc.comment) ? factory.createJSDocText(jsDoc.comment) : jsDoc.comment) as JSDocComment[];
+const jsDoc = singleOrUndefined(node.jsDoc);
+return jsDoc && positionsAreOnSameLine(jsDoc.pos, jsDoc.end, sourceFile) && length(comments) === 0 ? undefined :
+factory.createNodeArray(intersperse(comments, factory.createJSDocText("\n")));
+}
+
+public replaceJSDocComment(sourceFile: SourceFile, node: HasJSDoc, tags: readonly JSDocTag[]) {
+this.insertJsdocCommentBefore(sourceFile, updateJSDocHost(node), factory.createJSDocComment(this.createJSDocText(sourceFile, node), factory.createNodeArray(tags)));
+}
+
 public addJSDocTags(sourceFile: SourceFile, parent: HasJSDoc, newTags: readonly JSDocTag[]): void {
-const comments = flatMap(parent.jsDoc, j => typeof j.comment === "string" ? factory.createJSDocText(j.comment) : j.comment) as JSDocComment[];
 const oldTags = flatMapToMutable(parent.jsDoc, j => j.tags);
 const unmergedNewTags = newTags.filter(newTag => !oldTags.some((tag, i) => {
 const merged = tryMergeJsdocTags(tag, newTag);
 if (merged) oldTags[i] = merged;
 return !!merged;
 }));
-const tags = [...oldTags, ...unmergedNewTags];
-const jsDoc = singleOrUndefined(parent.jsDoc);
-const comment = jsDoc && positionsAreOnSameLine(jsDoc.pos, jsDoc.end, sourceFile) && !length(comments) ? undefined :
-factory.createNodeArray(intersperse(comments, factory.createJSDocText("\n")));
-const tag = factory.createJSDocComment(comment, factory.createNodeArray(tags));
-const host = updateJSDocHost(parent);
-this.insertJsdocCommentBefore(sourceFile, host, tag);
+this.replaceJSDocComment(sourceFile, parent, [...oldTags, ...unmergedNewTags]);
 }
 
 public filterJSDocTags(sourceFile: SourceFile, parent: HasJSDoc, predicate: (tag: JSDocTag) => boolean): void {
-const comments = flatMap(parent.jsDoc, j => typeof j.comment === "string" ? factory.createJSDocText(j.comment) : j.comment) as JSDocComment[];
-const oldTags = flatMapToMutable(parent.jsDoc, j => j.tags);
-const tag = factory.createJSDocComment(factory.createNodeArray(intersperse(comments, factory.createJSDocText("\n"))), factory.createNodeArray([...(filter(oldTags, predicate) || emptyArray)]));
-const host = updateJSDocHost(parent);
-this.insertJsdocCommentBefore(sourceFile, host, tag);
+this.replaceJSDocComment(sourceFile, parent, filter(flatMapToMutable(parent.jsDoc, j => j.tags), predicate));
 }
 
 public replaceRangeWithText(sourceFile: SourceFile, range: TextRange, text: string): void {

src/services/tsconfig.json

@@ -87,6 +87,7 @@
 "codefixes/fixExtendsInterfaceBecomesImplements.ts",
 "codefixes/fixForgottenThisPropertyAccess.ts",
 "codefixes/fixInvalidJsxCharacters.ts",
+"codefixes/fixUnmatchedParameter.ts",
 "codefixes/fixUnusedIdentifier.ts",
 "codefixes/fixUnreachableCode.ts",
 "codefixes/fixUnusedLabel.ts",

tests/baselines/reference/checkJsdocParamOnVariableDeclaredFunctionExpression.errors.txt

@@ -0,0 +1,23 @@
+tests/cases/conformance/jsdoc/0.js(14,20): error TS8024: JSDoc '@param' tag has name 's', but there is no parameter with that name.
+
+
+==== tests/cases/conformance/jsdoc/0.js (1 errors) ====
+// @ts-check
+/**
+* @param {number=} n
+* @param {string} [s]
+*/
+var x = function foo(n, s) {}
+var y;
+/**
+* @param {boolean!} b
+*/
+y = function bar(b) {}
+
+/**
+* @param {string} s
+~
+!!! error TS8024: JSDoc '@param' tag has name 's', but there is no parameter with that name.
+*/
+var one = function (s) { }, two = function (untyped) { };
+

tests/cases/fourslash/codeFixDeleteUnmatchedParameter1.ts

@@ -0,0 +1,23 @@
+/// <reference path='fourslash.ts' />
+
+// @filename: a.ts
+/////**
+//// * @param {number} a
+//// * @param {number} b
+//// */
+////function foo() {}
+
+verify.codeFixAvailable([
+{ description: "Delete unused '@param' tag 'a'" },
+{ description: "Delete unused '@param' tag 'b'" },
+]);
+
+verify.codeFix({
+description: [ts.Diagnostics.Delete_unused_param_tag_0.message, "a"],
+index: 0,
+newFileContent:
+`/**
+* @param {number} b
+*/
+function foo() {}`,
+});

tests/cases/fourslash/codeFixDeleteUnmatchedParameter2.ts

@@ -0,0 +1,26 @@
+/// <reference path='fourslash.ts' />
+
+// @filename: a.ts
+/////**
+//// * @param {number} a
+//// * @param {string} b
+//// */
+////function foo(a: number) {
+//// a;
+////}
+
+verify.codeFixAvailable([
+{ description: "Delete unused '@param' tag 'b'" },
+]);
+
+verify.codeFix({
+description: [ts.Diagnostics.Delete_unused_param_tag_0.message, "b"],
+index: 0,
+newFileContent:
+`/**
+* @param {number} a
+*/
+function foo(a: number) {
+a;
+}`
+});

tests/cases/fourslash/codeFixDeleteUnmatchedParameter3.ts

@@ -0,0 +1,30 @@
+/// <reference path='fourslash.ts' />
+
+// @filename: a.ts
+/////**
+//// * @param {number} a
+//// * @param {string} b
+//// * @param {number} c
+//// */
+////function foo(a: number, c: number) {
+//// a;
+//// c;
+////}
+
+verify.codeFixAvailable([
+{ description: "Delete unused '@param' tag 'b'" },
+]);
+
+verify.codeFix({
+description: [ts.Diagnostics.Delete_unused_param_tag_0.message, "b"],
+index: 0,
+newFileContent:
+`/**
+* @param {number} a
+* @param {number} c
+*/
+function foo(a: number, c: number) {
+a;
+c;
+}`
+});

tests/cases/fourslash/codeFixDeleteUnmatchedParameter4.ts

@@ -0,0 +1,19 @@
+/// <reference path='fourslash.ts' />
+
+// @filename: a.ts
+/////**
+//// * @param {number} a
+//// */
+////function foo() {}
+
+verify.codeFixAvailable([
+{ description: "Delete unused '@param' tag 'a'" },
+]);
+
+verify.codeFix({
+description: [ts.Diagnostics.Delete_unused_param_tag_0.message, "a"],
+index: 0,
+newFileContent:
+`/** */
+function foo() {}`
+});