From 2da2ed40d31c448b034dbb9cf41800169794a193 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Sat, 12 May 2018 17:39:43 -0700 Subject: [PATCH 1/3] Avoid duplicates when finding jsdoc 1. Add a cheap assert for detecting duplicates. getJSDocTags must find either [1] 0 or 1 comments or [2] the first two comments must not be identical. This does not always find duplicates for nodes with 3 or more comments, but these nodes are exceptionally rare. This assert fired for over 20 of the around 250 tests we have that retrieve JSDoc at all. So I fixed the asserts in [2] and [3]. 2. There were overlapping cases from calls to getSourceOfAssignment and getSpecialPropertyAssignment. getSpecialPropertyAssignment is too restrictive, but was in the correct location (parent vs parent.parent), so I removed the getSourceOfAssignment call and replaced the getSpecialPropertyAssignment calls with a less restrictive check. 3. When a node's parent is a PropertyDeclaration, getJSDocCOmmentsAndTags would check the parent for jsdoc. But when the *node* is a PropertyDeclaration, getJSDocCommentsAndTags would use the jsdoc from the initializer. This second case is useful to make sure that property declarations get all their jsdoc, but results in duplicates for their initializers. I couldn't think of a better fix than tracking the previous node in the recursive lookup of getJSDocCommentsAndTags, which is a little clunky. --- src/compiler/utilities.ts | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-) diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index 29bd38803d048..de60c40c01db9 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -1844,14 +1844,14 @@ namespace ts { export function getJSDocCommentsAndTags(node: Node): ReadonlyArray { let result: (JSDoc | JSDocTag)[] | undefined; - getJSDocCommentsAndTagsWorker(node); + getJSDocCommentsAndTagsWorker(node, undefined); return result || emptyArray; - function getJSDocCommentsAndTagsWorker(node: Node): void { + function getJSDocCommentsAndTagsWorker(node: Node, previous: Node | undefined): void { const parent = node.parent; if (!parent) return; if (parent.kind === SyntaxKind.PropertyAssignment || parent.kind === SyntaxKind.PropertyDeclaration || getNestedModuleDeclaration(parent)) { - getJSDocCommentsAndTagsWorker(parent); + getJSDocCommentsAndTagsWorker(parent, node); } // Try to recognize this pattern when node is initializer of variable declaration and JSDoc comments are on containing variable statement. // /** @@ -1860,19 +1860,19 @@ namespace ts { // */ // var x = function(name) { return name.length; } if (parent.parent && - (getSingleVariableOfVariableStatement(parent.parent) === node || getSourceOfAssignment(parent.parent))) { - getJSDocCommentsAndTagsWorker(parent.parent); + (getSingleVariableOfVariableStatement(parent.parent) === node)) { + getJSDocCommentsAndTagsWorker(parent.parent, node); } if (parent.parent && parent.parent.parent && (getSingleVariableOfVariableStatement(parent.parent.parent) || getSingleInitializerOfVariableStatementOrPropertyDeclaration(parent.parent.parent) === node || getSourceOfDefaultedAssignment(parent.parent.parent))) { - getJSDocCommentsAndTagsWorker(parent.parent.parent); + getJSDocCommentsAndTagsWorker(parent.parent.parent, node); } - if (isBinaryExpression(node) && getSpecialPropertyAssignmentKind(node) !== SpecialPropertyAssignmentKind.None || - isBinaryExpression(parent) && getSpecialPropertyAssignmentKind(parent) !== SpecialPropertyAssignmentKind.None || + if (isBinaryExpression(node) && node.operatorToken.kind === SyntaxKind.EqualsToken || + isBinaryExpression(parent) && parent.operatorToken.kind === SyntaxKind.EqualsToken || node.kind === SyntaxKind.PropertyAccessExpression && node.parent && node.parent.kind === SyntaxKind.ExpressionStatement) { - getJSDocCommentsAndTagsWorker(parent); + getJSDocCommentsAndTagsWorker(parent, node); } // Pull parameter comments from declaring function as well @@ -1880,7 +1880,7 @@ namespace ts { result = addRange(result, getJSDocParameterTags(node as ParameterDeclaration)); } - if (isVariableLike(node) && hasInitializer(node) && hasJSDocNodes(node.initializer)) { + if (isVariableLike(node) && hasInitializer(node) && node.initializer !== previous && hasJSDocNodes(node.initializer)) { result = addRange(result, node.initializer.jsDoc); } @@ -4771,7 +4771,9 @@ namespace ts { let tags = (node as JSDocContainer).jsDocCache; // If cache is 'null', that means we did the work of searching for JSDoc tags and came up with nothing. if (tags === undefined) { - (node as JSDocContainer).jsDocCache = tags = flatMap(getJSDocCommentsAndTags(node), j => isJSDoc(j) ? j.tags : j); + const comments = getJSDocCommentsAndTags(node); + Debug.assert(comments.length < 2 || comments[0] !== comments[1]); + (node as JSDocContainer).jsDocCache = tags = flatMap(comments, j => isJSDoc(j) ? j.tags : j); } return tags; } From eaa3b4e73d943a132059a0073f3eeca9a66beb54 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Mon, 14 May 2018 15:52:04 -0700 Subject: [PATCH 2/3] Fix lint; remove new context parameter --- src/compiler/utilities.ts | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/compiler/utilities.ts b/src/compiler/utilities.ts index de60c40c01db9..d5cd22d7b7cc6 100644 --- a/src/compiler/utilities.ts +++ b/src/compiler/utilities.ts @@ -1842,16 +1842,16 @@ namespace ts { (node as ModuleDeclaration).body; } - export function getJSDocCommentsAndTags(node: Node): ReadonlyArray { + export function getJSDocCommentsAndTags(hostNode: Node): ReadonlyArray { let result: (JSDoc | JSDocTag)[] | undefined; - getJSDocCommentsAndTagsWorker(node, undefined); + getJSDocCommentsAndTagsWorker(hostNode); return result || emptyArray; - function getJSDocCommentsAndTagsWorker(node: Node, previous: Node | undefined): void { + function getJSDocCommentsAndTagsWorker(node: Node): void { const parent = node.parent; if (!parent) return; if (parent.kind === SyntaxKind.PropertyAssignment || parent.kind === SyntaxKind.PropertyDeclaration || getNestedModuleDeclaration(parent)) { - getJSDocCommentsAndTagsWorker(parent, node); + getJSDocCommentsAndTagsWorker(parent); } // Try to recognize this pattern when node is initializer of variable declaration and JSDoc comments are on containing variable statement. // /** @@ -1861,18 +1861,18 @@ namespace ts { // var x = function(name) { return name.length; } if (parent.parent && (getSingleVariableOfVariableStatement(parent.parent) === node)) { - getJSDocCommentsAndTagsWorker(parent.parent, node); + getJSDocCommentsAndTagsWorker(parent.parent); } if (parent.parent && parent.parent.parent && (getSingleVariableOfVariableStatement(parent.parent.parent) || getSingleInitializerOfVariableStatementOrPropertyDeclaration(parent.parent.parent) === node || getSourceOfDefaultedAssignment(parent.parent.parent))) { - getJSDocCommentsAndTagsWorker(parent.parent.parent, node); + getJSDocCommentsAndTagsWorker(parent.parent.parent); } if (isBinaryExpression(node) && node.operatorToken.kind === SyntaxKind.EqualsToken || isBinaryExpression(parent) && parent.operatorToken.kind === SyntaxKind.EqualsToken || node.kind === SyntaxKind.PropertyAccessExpression && node.parent && node.parent.kind === SyntaxKind.ExpressionStatement) { - getJSDocCommentsAndTagsWorker(parent, node); + getJSDocCommentsAndTagsWorker(parent); } // Pull parameter comments from declaring function as well @@ -1880,7 +1880,7 @@ namespace ts { result = addRange(result, getJSDocParameterTags(node as ParameterDeclaration)); } - if (isVariableLike(node) && hasInitializer(node) && node.initializer !== previous && hasJSDocNodes(node.initializer)) { + if (isVariableLike(node) && hasInitializer(node) && node.initializer !== hostNode && hasJSDocNodes(node.initializer)) { result = addRange(result, node.initializer.jsDoc); } From 104c6d59496658af5a2a0f192e0dbadaf6af21b2 Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Tue, 15 May 2018 14:48:25 -0700 Subject: [PATCH 3/3] Update importJsNodeModule3 with fix --- tests/cases/fourslash/importJsNodeModule3.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/tests/cases/fourslash/importJsNodeModule3.ts b/tests/cases/fourslash/importJsNodeModule3.ts index 6c44dd65b63ca..46ff4d009bebf 100644 --- a/tests/cases/fourslash/importJsNodeModule3.ts +++ b/tests/cases/fourslash/importJsNodeModule3.ts @@ -37,12 +37,9 @@ edit.backspace(2); edit.insert('z('); verify.signatureHelp({ text: "z(a: number | boolean, b: string[]): string", - // TODO: GH#24129 - parameterDocComment: "The first param\nThe first param", + parameterDocComment: "The first param", tags: [ { name: "param", text: "a The first param" }, { name: "param", text: "b The second param" }, - { name: "param", text: "a The first param" }, - { name: "param", text: "b The second param" }, ], });