From d74c4515623b5b88577ad930fda0be6a3f69906b Mon Sep 17 00:00:00 2001 From: Nathan Shively-Sanders <293473+sandersn@users.noreply.github.com> Date: Fri, 15 Jun 2018 11:15:16 -0700 Subject: [PATCH] Simplify getJSDocCommentAndTags Previously, getJSDocCommentAndTags could recur up to four times if any of four predicates matched. However, to avoid duplicates, the predicates have been tuned to be mutually exclusive, which means that the recursion can be turned into a while loop. The while loop is much simpler and safer, since it is guaranteed to only walk up the tree one time. In addition, the extra check that adds jsdoc from initializers only runs once, before the loop, further reducing the opportunity for duplicate jsdocs. I thought about further simplifying the code that gets the next node to check, but to know when to stop the loop, I'd need a predicate that is as complicated as the code in `getNextJSDocCommentLocation`, so I kept the existing code, just reordering it for compactness. --- src/parser/utilities.ts | 78 +++++++++---------- tests/cases/fourslash/commentsClassMembers.ts | 8 +- .../cases/fourslash/commentsCommentParsing.ts | 6 +- .../fourslash/commentsFunctionExpression.ts | 28 +++---- 4 files changed, 59 insertions(+), 61 deletions(-) diff --git a/src/parser/utilities.ts b/src/parser/utilities.ts index 059916f8fa435..0f6d7650a524f 100644 --- a/src/parser/utilities.ts +++ b/src/parser/utilities.ts @@ -2070,53 +2070,51 @@ namespace ts { export function getJSDocCommentsAndTags(hostNode: Node): ReadonlyArray { let result: (JSDoc | JSDocTag)[] | undefined; - getJSDocCommentsAndTagsWorker(hostNode); - return result || emptyArray; + // Pull parameter comments from declaring function as well + if (isVariableLike(hostNode) && hasInitializer(hostNode) && hasJSDocNodes(hostNode.initializer!)) { + result = addRange(result, (hostNode.initializer as HasJSDoc).jsDoc!); + } - 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); - } - // Try to recognize this pattern when node is initializer of variable declaration and JSDoc comments are on containing variable statement. - // /** - // * @param {number} name - // * @returns {number} - // */ - // var x = function(name) { return name.length; } - if (parent.parent && (getSingleVariableOfVariableStatement(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); - } - 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) { - if (isBinaryExpression(parent)) { - getJSDocCommentsAndTagsWorker(parent.parent); - } - else { - getJSDocCommentsAndTagsWorker(parent); - } + let node: Node | undefined = hostNode; + while (node && node.parent) { + if (hasJSDocNodes(node)) { + result = addRange(result, node.jsDoc!); } - // Pull parameter comments from declaring function as well if (node.kind === SyntaxKind.Parameter) { result = addRange(result, getJSDocParameterTags(node as ParameterDeclaration)); + break; } + node = getNextJSDocCommentLocation(node); + } + return result || emptyArray; + } - if (isVariableLike(node) && hasInitializer(node) && node.initializer !== hostNode && hasJSDocNodes(node.initializer!)) { - result = addRange(result, (node.initializer as HasJSDoc).jsDoc); - } - - if (hasJSDocNodes(node)) { - result = addRange(result, node.jsDoc); - } + function getNextJSDocCommentLocation(node: Node) { + const parent = node.parent; + if (parent.kind === SyntaxKind.PropertyAssignment || + parent.kind === SyntaxKind.PropertyDeclaration || + parent.kind === SyntaxKind.ExpressionStatement && node.kind === SyntaxKind.PropertyAccessExpression || + getNestedModuleDeclaration(parent) || + isBinaryExpression(node) && node.operatorToken.kind === SyntaxKind.EqualsToken) { + return parent; + } + // Try to recognize this pattern when node is initializer of variable declaration and JSDoc comments are on containing variable statement. + // /** + // * @param {number} name + // * @returns {number} + // */ + // var x = function(name) { return name.length; } + else if (parent.parent && + (getSingleVariableOfVariableStatement(parent.parent) === node || + isBinaryExpression(parent) && parent.operatorToken.kind === SyntaxKind.EqualsToken)) { + return parent.parent; + } + else if (parent.parent && parent.parent.parent && + (getSingleVariableOfVariableStatement(parent.parent.parent) || + getSingleInitializerOfVariableStatementOrPropertyDeclaration(parent.parent.parent) === node || + getSourceOfDefaultedAssignment(parent.parent.parent))) { + return parent.parent.parent; } } diff --git a/tests/cases/fourslash/commentsClassMembers.ts b/tests/cases/fourslash/commentsClassMembers.ts index 6b598ab458c13..b0ed3e9e48bf6 100644 --- a/tests/cases/fourslash/commentsClassMembers.ts +++ b/tests/cases/fourslash/commentsClassMembers.ts @@ -572,12 +572,12 @@ verify.quickInfos({ }); goTo.marker('114'); -verify.completionListContains("a", "(property) cWithConstructorProperty.a: number", "this is first parameter a\nmore info about a"); -verify.quickInfoIs("(property) cWithConstructorProperty.a: number", "this is first parameter a\nmore info about a"); +verify.completionListContains("a", "(property) cWithConstructorProperty.a: number", "more info about a\nthis is first parameter a"); +verify.quickInfoIs("(property) cWithConstructorProperty.a: number", "more info about a\nthis is first parameter a"); goTo.marker('115'); -verify.completionListContains("a", "(parameter) a: number", "this is first parameter a\nmore info about a"); -verify.quickInfoIs("(parameter) a: number", "this is first parameter a\nmore info about a"); +verify.completionListContains("a", "(parameter) a: number", "more info about a\nthis is first parameter a"); +verify.quickInfoIs("(parameter) a: number", "more info about a\nthis is first parameter a"); verify.quickInfos({ 116: "this: this", diff --git a/tests/cases/fourslash/commentsCommentParsing.ts b/tests/cases/fourslash/commentsCommentParsing.ts index 6b0c880e9bbbe..bba86fea29914 100644 --- a/tests/cases/fourslash/commentsCommentParsing.ts +++ b/tests/cases/fourslash/commentsCommentParsing.ts @@ -420,7 +420,7 @@ verify.signatureHelp({ marker: "38", docComment: concatDoc, parameterDocComment: verify.quickInfoAt("38aq", "(parameter) bar: string", "is second string"); goTo.marker('39'); -verify.completionListContains("a", "(parameter) a: number", "it is first parameter\nthis is inline comment for a "); +verify.completionListContains("a", "(parameter) a: number", "this is inline comment for a \nit is first parameter"); verify.completionListContains("b", "(parameter) b: number", "this is inline comment for b"); verify.completionListContains("c", "(parameter) c: number", "it is third parameter"); verify.completionListContains("d", "(parameter) d: number", ""); @@ -430,7 +430,7 @@ const jsdocTestTags: ReadonlyArray = [ { name: "param", text: "a it is first parameter" }, { name: "param", text: "c it is third parameter" }, ]; -verify.signatureHelp({ marker: "40", docComment: jsdocTestDocComment, parameterDocComment: "it is first parameter\nthis is inline comment for a ", tags: jsdocTestTags }); +verify.signatureHelp({ marker: "40", docComment: jsdocTestDocComment, parameterDocComment: "this is inline comment for a \nit is first parameter", tags: jsdocTestTags }); verify.quickInfos({ "40q": [ "function jsDocParamTest(a: number, b: number, c: number, d: number): number", @@ -438,7 +438,7 @@ verify.quickInfos({ ], "40aq": [ "(parameter) a: number", - "it is first parameter\nthis is inline comment for a " + "this is inline comment for a \nit is first parameter" ] }); diff --git a/tests/cases/fourslash/commentsFunctionExpression.ts b/tests/cases/fourslash/commentsFunctionExpression.ts index bd9a5c934339f..257b834575687 100644 --- a/tests/cases/fourslash/commentsFunctionExpression.ts +++ b/tests/cases/fourslash/commentsFunctionExpression.ts @@ -32,7 +32,7 @@ ////} ////assig/*16*/ned/*17*/(/*18*/"hey"); -verify.quickInfoAt("1", "var lambdaFoo: (a: number, b: number) => number", "lambdaFoo var comment\nthis is lambda comment"); +verify.quickInfoAt("1", "var lambdaFoo: (a: number, b: number) => number", "this is lambda comment\nlambdaFoo var comment"); goTo.marker('2'); verify.completionListContains('a', '(parameter) a: number', 'param a'); @@ -42,18 +42,18 @@ verify.completionListContains('b', '(parameter) b: number', 'param b'); verify.quickInfoAt("3", "var lambddaNoVarComment: (a: number, b: number) => number", "this is lambda multiplication"); goTo.marker('4'); -verify.completionListContains('lambdaFoo', 'var lambdaFoo: (a: number, b: number) => number', 'lambdaFoo var comment\nthis is lambda comment'); +verify.completionListContains('lambdaFoo', 'var lambdaFoo: (a: number, b: number) => number', "this is lambda comment\nlambdaFoo var comment"); verify.completionListContains('lambddaNoVarComment', 'var lambddaNoVarComment: (a: number, b: number) => number', 'this is lambda multiplication'); verify.signatureHelp( { marker: "5", - docComment: "lambdaFoo var comment\nthis is lambda comment", + docComment: "this is lambda comment\nlambdaFoo var comment", parameterDocComment: "param a", }, { marker: "6", - docComment: "lambdaFoo var comment\nthis is lambda comment", + docComment: "this is lambda comment\nlambdaFoo var comment", parameterDocComment: "param b", }, ); @@ -61,31 +61,31 @@ verify.signatureHelp( // no documentation from nested lambda verify.quickInfos({ 7: "function anotherFunc(a: number): string", - 8: ["(local var) lambdaVar: (b: string) => string", "documentation\ninner docs "], + 8: ["(local var) lambdaVar: (b: string) => string", "inner docs \ndocumentation"], 9: ["(parameter) b: string", "inner parameter "], 10: "(local var) localVar: string", 11: "(local var) localVar: string", 12: ["(parameter) b: string", "inner parameter "], - 13: ["(local var) lambdaVar: (b: string) => string", "documentation\ninner docs "], + 13: ["(local var) lambdaVar: (b: string) => string", "inner docs \ndocumentation"], 14: [ "var assigned: (s: string) => number", - "On variable\nSummary on expression" + "Summary on expression\nOn variable" ] }); goTo.marker('15'); -verify.completionListContains('s', '(parameter) s: string', "the first parameter!\nparam on expression\nOn parameter "); -verify.quickInfoAt("16", "var assigned: (s: string) => number", "On variable\nSummary on expression"); +verify.completionListContains('s', '(parameter) s: string', "On parameter \nparam on expression\nthe first parameter!"); +verify.quickInfoAt("16", "var assigned: (s: string) => number", "Summary on expression\nOn variable"); goTo.marker('17'); -verify.completionListContains("assigned", "var assigned: (s: string) => number", "On variable\nSummary on expression"); +verify.completionListContains("assigned", "var assigned: (s: string) => number", "Summary on expression\nOn variable"); verify.signatureHelp({ marker: "18", - docComment: "On variable\nSummary on expression", - parameterDocComment: "the first parameter!\nparam on expression\nOn parameter ", + docComment: "Summary on expression\nOn variable", + parameterDocComment: "On parameter \nparam on expression\nthe first parameter!", tags: [ - { name: "param", text: "s the first parameter!" }, - { name: "returns", text: "the parameter's length" }, { name: "param", text: "s param on expression" }, { name: "returns", text: "return on expression" }, + { name: "param", text: "s the first parameter!" }, + { name: "returns", text: "the parameter's length" }, ], });