Skip to content

Zip @param tags and parameters together instead of looking for a name #18832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 39 additions & 0 deletions src/compiler/binder.ts
Original file line number Diff line number Diff line change
Expand Up @@ -722,10 +722,49 @@ namespace ts {
break;
default:
bindEachChild(node);
if (isFunctionLikeDeclaration(node)) {
bindJSDocParameters(node);
}
break;
}
}

function bindJSDocParameters(node: SignatureDeclaration): void {
const { parameters } = node;
if (parameters.length === 0) {
return;
}

// Clear any previous binding
for (const p of parameters) {
p.jsdocParamTags = undefined;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we need to do this? Is it because the binder is retained between edits?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The source tree node may have been reused from a previous version of the AST so its properties must be reset.

}

let paramIndex = 0;
for (const tag of getJSDocTags(parent)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to be O(mn) where m = |tags| and n = |params|. That's because, for each tag, we examine every parameter starting at 0. Is this necessary? I think it would suffice to increment paramIndex until a single matching tag is found.

Even better, you could iterate over the parameters instead and keep a non-restarting index into the tags. You can give up when the tags run out.

zip :: [Tag] -> [Param] -> [Param]
zip (t:tags) (p:params) | name t == name p = p { tags = Just t } : zip tags params
zip (t:tags) params = zip tags params
zip [] params = params
zip tags [] = []

Note that the Haskell version doesn't side-effect, it just assumes there is a field tags :: Maybe Tag that is set to Nothing when this function is called.

let tagIndex = 0;
for (const param of parameters) {
  while (tags[tagIndex].name.escapedText !== param.name.escapedText && tagIndex < tags.length) {
    tagIndex++;
  }
  if (tagIndex >= tags.length) {
    break;
  }
  if (tags[tagIndex].name.escapedText === param.name.escapedText) {
    param.jsdocParamTags = append(param.jsdocParamTags, tags[tagIndex]);
  }
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to keep supporting out-of-order @param tags, but in the case where they are in order this will be O(n) because we start looking at the previous paramIndex so usually we only go through the loop once.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I misunderstood the const startParamIndex = paramIndex. This is better than what I proposed, then.

if (!isJSDocParameterTag(tag)) {
continue;
}

// If we run out of parameters, just do nothing.
// Will error in `checkJSDocParameterTag` since it will have no symbol.
const startParamIndex = paramIndex;
do {
const param = parameters[paramIndex];
paramIndex++;
if (paramIndex === parameters.length) {
paramIndex = 0;
}

if (param.name.kind !== SyntaxKind.Identifier || getLeftmostName(tag.name).escapedText === param.name.escapedText) {
tag.symbol = param.symbol;
param.jsdocParamTags = append(param.jsdocParamTags, tag);
break;
}
} while (paramIndex !== startParamIndex);
}
}

function isNarrowingExpression(expr: Expression): boolean {
switch (expr.kind) {
case SyntaxKind.Identifier:
Expand Down
14 changes: 7 additions & 7 deletions src/compiler/checker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6747,8 +6747,8 @@ namespace ts {
return isInJavaScriptFile(node) && (
// node.type should only be a JSDocOptionalType when node is a parameter of a JSDocFunctionType
node.type && node.type.kind === SyntaxKind.JSDocOptionalType
|| getJSDocParameterTags(node).some(({ isBracketed, typeExpression }) =>
isBracketed || !!typeExpression && typeExpression.type.kind === SyntaxKind.JSDocOptionalType));
|| some(node.jsdocParamTags, ({ isBracketed, typeExpression }) =>
isBracketed || !!typeExpression && typeExpression.type.kind === SyntaxKind.JSDocOptionalType));
}

function tryFindAmbientModule(moduleName: string, withAugmentations: boolean) {
Expand Down Expand Up @@ -21856,7 +21856,7 @@ namespace ts {

function checkJSDocParameterTag(node: JSDocParameterTag) {
checkSourceElement(node.typeExpression);
if (!getParameterSymbolFromJSDoc(node)) {
if (!node.symbol) {
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`
Expand All @@ -21872,7 +21872,7 @@ namespace ts {
!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));
idText(getLeftmostName(node.name)));
}
}
}
Expand Down Expand Up @@ -24853,7 +24853,7 @@ namespace ts {
return;
}

const param = getParameterSymbolFromJSDoc(paramTag);
const param = paramTag.symbol;
if (!param) {
// We will error in `checkJSDocParameterTag`.
return;
Expand Down Expand Up @@ -24881,7 +24881,7 @@ namespace ts {
Because `a` will just be of type `number | undefined`. A synthetic `...args` will also be added, which *will* get an array type.
*/
const lastParamDeclaration = lastOrUndefined(host.parameters);
const symbol = getParameterSymbolFromJSDoc(paramTag);
const symbol = paramTag.symbol;
if (!lastParamDeclaration ||
symbol && lastParamDeclaration.symbol === symbol && isRestParameter(lastParamDeclaration)) {
return createArrayType(type);
Expand Down Expand Up @@ -25335,7 +25335,7 @@ namespace ts {
}

if (entityName.parent!.kind === SyntaxKind.JSDocParameterTag) {
return getParameterSymbolFromJSDoc(entityName.parent as JSDocParameterTag);
return entityName.parent.symbol;
}

if (entityName.parent.kind === SyntaxKind.TypeParameter && entityName.parent.parent.kind === SyntaxKind.JSDocTemplateTag) {
Expand Down
1 change: 1 addition & 0 deletions src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -855,6 +855,7 @@ namespace ts {
questionToken?: QuestionToken; // Present on optional parameter
type?: TypeNode; // Optional type annotation
initializer?: Expression; // Optional initializer
/* @internal */ jsdocParamTags?: JSDocParameterTag[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does doing this in the binder instead of the checker affect performance? I don't understand the performance characteristics of the binder well enough to predict, so maybe a perf test is in order.

}

export interface BindingElement extends NamedDeclaration {
Expand Down
67 changes: 31 additions & 36 deletions src/compiler/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1800,6 +1800,23 @@ namespace ts {
(node as ModuleDeclaration).body;
}

export function forEachJSDoc(node: Node, cb: (doc: JSDoc | JSDocTag) => void): void {
if (isJSDocPropertyTag(node)) {
cb(node);
return;
}
if (isJSDocTypedefTag(node)) {
cb(node.parent);
return;
}
if (isParameter(node)) {
forEach(node.jsdocParamTags, cb);
}
for (const tag of getJSDocCommentsAndTags(node)) {
cb(tag);
}
}

export function getJSDocCommentsAndTags(node: Node): ReadonlyArray<JSDoc | JSDocTag> {
let result: (JSDoc | JSDocTag)[] | undefined;
getJSDocCommentsAndTagsWorker(node);
Expand Down Expand Up @@ -1833,11 +1850,6 @@ namespace ts {
getJSDocCommentsAndTagsWorker(parent);
}

// Pull parameter comments from declaring function as well
if (node.kind === SyntaxKind.Parameter) {
result = addRange(result, getJSDocParameterTags(node as ParameterDeclaration));
}

if (isVariableLike(node) && hasInitializer(node) && hasJSDocNodes(node.initializer)) {
result = addRange(result, node.initializer.jsDoc);
}
Expand All @@ -1848,23 +1860,6 @@ namespace ts {
}
}

/** Does the opposite of `getJSDocParameterTags`: given a JSDoc parameter, finds the parameter corresponding to it. */
export function getParameterSymbolFromJSDoc(node: JSDocParameterTag): Symbol | undefined {
if (node.symbol) {
return node.symbol;
}
if (!isIdentifier(node.name)) {
return undefined;
}
const name = node.name.escapedText;
const decl = getHostSignatureFromJSDoc(node);
if (!decl) {
return undefined;
}
const parameter = find(decl.parameters, p => p.name.kind === SyntaxKind.Identifier && p.name.escapedText === name);
return parameter && parameter.symbol;
}

export function getHostSignatureFromJSDoc(node: JSDocParameterTag): SignatureDeclaration | undefined {
const host = getJSDocHost(node);
const decl = getSourceOfDefaultedAssignment(host) ||
Expand Down Expand Up @@ -3866,6 +3861,10 @@ namespace ts {
}
}

export function getLeftmostName(node: EntityName): Identifier {
return node.kind === SyntaxKind.QualifiedName ? getLeftmostName(node.left) : node;
}

export function compareDataObjects(dst: any, src: any): boolean {
if (!dst || !src || Object.keys(dst).length !== Object.keys(src).length) {
return false;
Expand Down Expand Up @@ -4616,17 +4615,9 @@ namespace ts {
* initializer is the containing function. The tags closest to the
* node are returned first, so in the previous example, the param
* tag on the containing function expression would be first.
*
* Does not return tags for binding patterns, because JSDoc matches
* parameters by name and binding patterns do not have a name.
*/
export function getJSDocParameterTags(param: ParameterDeclaration): ReadonlyArray<JSDocParameterTag> {
if (param.name && isIdentifier(param.name)) {
const name = param.name.escapedText;
return getJSDocTags(param.parent).filter((tag): tag is JSDocParameterTag => isJSDocParameterTag(tag) && isIdentifier(tag.name) && tag.name.escapedText === name);
}
// a binding pattern doesn't have a name, so it's not possible to match it a JSDoc parameter, which is identified by name
return emptyArray;
return param.jsdocParamTags || emptyArray;
}

/**
Expand Down Expand Up @@ -4681,12 +4672,13 @@ namespace ts {
* tag directly on the node would be returned.
*/
export function getJSDocType(node: Node): TypeNode | undefined {
let tag: JSDocTypeTag | JSDocParameterTag | undefined = getFirstJSDocTag(node, isJSDocTypeTag);
if (!tag && isParameter(node)) {
tag = find(getJSDocParameterTags(node), tag => !!tag.typeExpression);
const tag = getFirstJSDocTag(node, isJSDocTypeTag);
if (tag) {
return tag.typeExpression && tag.typeExpression.type;
}
else if (isParameter(node)) {
return forEach(node.jsdocParamTags, tag => tag.typeExpression && tag.typeExpression.type);
}

return tag && tag.typeExpression && tag.typeExpression.type;
}

/**
Expand All @@ -4702,6 +4694,9 @@ namespace ts {

/** Get all JSDoc tags related to a node, including those on parent nodes. */
export function getJSDocTags(node: Node): ReadonlyArray<JSDocTag> {
if ((node as ParameterDeclaration).jsdocParamTags) {
return (node as ParameterDeclaration).jsdocParamTags;
}
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) {
Expand Down
17 changes: 3 additions & 14 deletions src/services/jsDoc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -55,28 +55,17 @@ namespace ts.JsDoc {
// from Array<T> - Array<string> and Array<number>
const documentationComment: SymbolDisplayPart[] = [];
forEachUnique(declarations, declaration => {
for (const { comment } of getCommentHavingNodes(declaration)) {
if (comment === undefined) continue;
forEachJSDoc(declaration, ({comment}) => {
if (comment === undefined) return;
if (documentationComment.length) {
documentationComment.push(lineBreakPart());
}
documentationComment.push(textPart(comment));
}
});
});
return documentationComment;
}

function getCommentHavingNodes(declaration: Declaration): ReadonlyArray<JSDoc | JSDocTag> {
switch (declaration.kind) {
case SyntaxKind.JSDocPropertyTag:
return [declaration as JSDocPropertyTag];
case SyntaxKind.JSDocTypedefTag:
return [(declaration as JSDocTypedefTag).parent];
default:
return getJSDocCommentsAndTags(declaration);
}
}

export function getJsDocTagsFromDeclarations(declarations?: Declaration[]): JSDocTagInfo[] {
// Only collect doc comments from duplicate declarations once.
const tags: JSDocTagInfo[] = [];
Expand Down
3 changes: 0 additions & 3 deletions tests/baselines/reference/api/tsserverlibrary.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3020,9 +3020,6 @@ declare namespace ts {
* initializer is the containing function. The tags closest to the
* node are returned first, so in the previous example, the param
* tag on the containing function expression would be first.
*
* Does not return tags for binding patterns, because JSDoc matches
* parameters by name and binding patterns do not have a name.
*/
function getJSDocParameterTags(param: ParameterDeclaration): ReadonlyArray<JSDocParameterTag>;
/**
Expand Down
3 changes: 0 additions & 3 deletions tests/baselines/reference/api/typescript.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3075,9 +3075,6 @@ declare namespace ts {
* initializer is the containing function. The tags closest to the
* node are returned first, so in the previous example, the param
* tag on the containing function expression would be first.
*
* Does not return tags for binding patterns, because JSDoc matches
* parameters by name and binding patterns do not have a name.
*/
function getJSDocParameterTags(param: ParameterDeclaration): ReadonlyArray<JSDocParameterTag>;
/**
Expand Down
12 changes: 12 additions & 0 deletions tests/baselines/reference/jsdocParamTagMatching1.symbols
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
=== /a.js ===
/**
* @param {{ x: number, y: string }} p
* @param {number} m
*/
function f({ x, y }, m) {
>f : Symbol(f, Decl(a.js, 0, 0))
>x : Symbol(x, Decl(a.js, 4, 12))
>y : Symbol(y, Decl(a.js, 4, 15))
>m : Symbol(m, Decl(a.js, 4, 20))
}

12 changes: 12 additions & 0 deletions tests/baselines/reference/jsdocParamTagMatching1.types
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
=== /a.js ===
/**
* @param {{ x: number, y: string }} p
* @param {number} m
*/
function f({ x, y }, m) {
>f : ({ x, y }: { x: number; y: string; }, m: number) => void
>x : number
>y : string
>m : number
}

11 changes: 11 additions & 0 deletions tests/cases/compiler/jsdocParamTagMatching1.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
// @allowJs: true
// @checkJs: true
// @noEmit: true
// @Filename: /a.js

/**
* @param {{ x: number, y: string }} p
* @param {number} m
*/
function f({ x, y }, m) {
}
6 changes: 6 additions & 0 deletions tests/cases/fourslash/findAllRefsJsDocParam.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
/// <reference path='fourslash.ts' />

/////** @param [|s|] */
////const x = [|{| "isWriteAccess": true, "isDefinition": true |}s|] => [|s|];

verify.singleReferenceGroup("(parameter) s: any");
4 changes: 2 additions & 2 deletions tests/cases/fourslash/jsDocFunctionSignatures12.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@
////function f1(o) {
//// o/**/;
////}
goTo.marker();
verify.quickInfoIs("(parameter) o: any");

verify.quickInfoAt("", `(parameter) o: any`);
2 changes: 1 addition & 1 deletion tests/cases/fourslash/jsDocTypedefQuickInfo1.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,4 +30,4 @@
//// }
//// foo1({x: 'abc'});

verify.baselineQuickInfo();
verify.baselineQuickInfo();
27 changes: 6 additions & 21 deletions tests/cases/fourslash/signatureHelpWhenEditingCallExpression.ts
Original file line number Diff line number Diff line change
@@ -1,30 +1,15 @@
/// <reference path='fourslash.ts' />

// @noLib: true
/////**
//// * Returns the substring at the specified location within a String object.
//// * @param start The zero-based index integer indicating the beginning of the substring.
//// * @param end Zero-based index integer indicating the end of the substring. The substring includes the characters up to, but not including, the character indicated by end.
//// * If end is omitted, the characters from start through the end of the original string are returned.
//// */
////function foo(start: number, end?: number) {
//// * @param start START
//// */
////function foo(start: number) {
//// return "";
////}
////
////fo/*1*/
////foo/*1*/
goTo.marker('1');
verify.not.signatureHelpPresent();
edit.insert("o");
verify.not.signatureHelpPresent();
edit.insert("(");
verify.currentParameterHelpArgumentDocCommentIs("The zero-based index integer indicating the beginning of the substring.");
edit.insert("10,");
verify.currentParameterHelpArgumentDocCommentIs("Zero-based index integer indicating the end of the substring. The substring includes the characters up to, but not including, the character indicated by end.\nIf end is omitted, the characters from start through the end of the original string are returned.");
edit.insert(" ");
verify.currentParameterHelpArgumentDocCommentIs("Zero-based index integer indicating the end of the substring. The substring includes the characters up to, but not including, the character indicated by end.\nIf end is omitted, the characters from start through the end of the original string are returned.");
edit.insert(", ");
edit.backspace(3);
verify.currentParameterHelpArgumentDocCommentIs("Zero-based index integer indicating the end of the substring. The substring includes the characters up to, but not including, the character indicated by end.\nIf end is omitted, the characters from start through the end of the original string are returned.");
edit.insert("12");
verify.currentParameterHelpArgumentDocCommentIs("Zero-based index integer indicating the end of the substring. The substring includes the characters up to, but not including, the character indicated by end.\nIf end is omitted, the characters from start through the end of the original string are returned.");
edit.insert(")");
verify.not.signatureHelpPresent();
verify.currentParameterHelpArgumentDocCommentIs("START");