Skip to content
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.

Sign up for GitHub

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

Js doc overloads #50789

Closed
wants to merge 4 commits into from
Closed

Js doc overloads #50789

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a51d682
Add support for JSDocOverloadTag
apendua Sep 15, 2022
b578825
Bind additional declarations from jsDoc
apendua Sep 15, 2022
e4b35c2
Update baseline for tests
apendua Sep 15, 2022
9f26e98
Add new tests along with baselines
apendua Sep 15, 2022
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
104 changes: 104 additions & 0 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3333,13 +3333,112 @@ namespace ts {
}
}

function extractJSDocDeclarations<T>(node: T): T[];
function extractJSDocDeclarations(node: Node): Node[] {
if (!isFunctionDeclaration(node) && !isMethodDeclaration(node)) {
return [node];
}
if (!isInJSFile(node) || !hasJSDocNodes(node)) {
return [node];
}
const declarations: Node[] = [];
const jsDocNodes: JSDoc[] = node.jsDoc!;
const lastJsDocNode = last(jsDocNodes);
let jsDocNodesBuffer: JSDoc[] = [];
let currentPos = node.pos;
for (const jsDoc of jsDocNodes) {
jsDocNodesBuffer.push(jsDoc);
if (jsDoc === lastJsDocNode) {
break;
}
const overloadTag = jsDoc.tags && find(jsDoc.tags, isJSDocOverloadTag);
if (!overloadTag) {
continue;
}
const newParameters: ParameterDeclaration[] = [];
for (const tag of jsDoc.tags) {
if (isJSDocParameterTag(tag) && tag.name.kind === SyntaxKind.Identifier) {
const newParameter = factory.createParameterDeclaration(
Copy link
Member

Choose a reason for hiding this comment

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

Synthesising declarations is not a technique that works in the typescript compiler. A better approach would be to make sure all the jsdoc nodes are still available to the checker. In the checker, make sure that each @overload results in a signature. You'll want to start near getSignatureFromDeclaration, or its callers, I think.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sandersn Agreed. I've also observed other BUGs related to IDE integrations that are coming from the fact that I am trying to synthesize those declarations, e.g. "did not expect identifier in trivia".

You'll want to start near getSignatureFromDeclaration, or its callers, I think.

Thank you for this suggestion! I've already tried that and it seems to be working much better:

https://github.com/microsoft/TypeScript/compare/main...apendua:js-doc-overload-tag?expand=1

How we should go about that? Should I close this pull request and open a new one?

Copy link
Member

Choose a reason for hiding this comment

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

It is up to you. I am fine with proceeding with this one; I squash merge PRs since it makes the git history easier to search.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sandersn I ended up opening a new PR here: #51234

I also used this opportunity to rebase onto the latest main. Thank you again for all your suggestions!

/*modifiers*/ undefined,
/*dotDotDotToken*/ undefined,
tag.name,
/*questionToken*/ undefined,
/*type*/ undefined,
/*initializer*/ undefined
);
(newParameter as Mutable<Node>).flags |= NodeFlags.JavaScriptFile;
newParameters.push(newParameter);
}
}
let newDeclaration: FunctionDeclaration | MethodDeclaration | undefined;
if (isFunctionDeclaration(node)) {
newDeclaration = factory.createFunctionDeclaration(
/*modifiers*/ undefined,
node.asteriskToken,
node.name,
/*typeParameters*/ undefined,
newParameters,
/*type*/ undefined,
/*body*/ undefined,
);
}
else if (isMethodDeclaration(node)) {
newDeclaration = factory.createMethodDeclaration(
/*modifiers*/ undefined,
node.asteriskToken,
node.name,
/*questionToken*/ undefined,
/*typeParameters*/ undefined,
newParameters,
/*type*/ undefined,
/*body*/ undefined,
);
if (node.flowNode) {
newDeclaration.flowNode = node.flowNode;
}
}
if (newDeclaration) {
for (const parameter of newParameters) {
setParent(parameter, newDeclaration);
}
newDeclaration.jsDoc = [];
(newDeclaration as Mutable<Node>).flags = node.flags;
setParent(newDeclaration, node.parent);
let end = -1;
for (const anotherJSDoc of jsDocNodesBuffer) {
setParent(anotherJSDoc, newDeclaration);
end = anotherJSDoc.end;
newDeclaration.jsDoc.push(anotherJSDoc);
}
setTextRangePosEnd(newDeclaration, currentPos, end);
declarations.push(newDeclaration);
currentPos = end;
jsDocNodesBuffer = [];
}
}
if (jsDocNodesBuffer.length < jsDocNodes.length) {
node.jsDoc = jsDocNodesBuffer;
node.jsDocCache = undefined;
if (currentPos !== node.pos) {
setTextRangePosEnd(node, currentPos, node.end);
}
}
declarations.push(node);
return declarations;
}

function bindFunctionDeclaration(node: FunctionDeclaration) {
if (!file.isDeclarationFile && !(node.flags & NodeFlags.Ambient)) {
if (isAsyncFunction(node)) {
emitFlags |= NodeFlags.HasAsyncFunctions;
}
}

const declarations = extractJSDocDeclarations(node);
for (let i = 0; i < declarations.length - 1; i += 1) {
bind(declarations[i]);
}

checkStrictModeFunctionName(node);
if (inStrictMode) {
checkStrictModeFunctionDeclaration(node);
Expand Down Expand Up @@ -3373,6 +3472,11 @@ namespace ts {
node.flowNode = currentFlow;
}

const declarations = extractJSDocDeclarations(node);
for (let i = 0; i < declarations.length - 1; i += 1) {
bind(declarations[i]);
}

return hasDynamicName(node)
? bindAnonymousDeclaration(node, symbolFlags, InternalSymbolName.Computed)
: declareSymbolAndAddToSymbolTable(node, symbolFlags, symbolExcludes);
Expand Down
27 changes: 14 additions & 13 deletions src/compiler/emitter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1639,6 +1639,7 @@ namespace ts {
case SyntaxKind.JSDocTag:
case SyntaxKind.JSDocClassTag:
case SyntaxKind.JSDocOverrideTag:
case SyntaxKind.JSDocOverloadTag:
return emitJSDocSimpleTag(node as JSDocTag);
case SyntaxKind.JSDocAugmentsTag:
case SyntaxKind.JSDocImplementsTag:
Expand Down Expand Up @@ -3660,13 +3661,13 @@ namespace ts {

function hasTrailingCommentsAtPosition(pos: number) {
let result = false;
forEachTrailingCommentRange(currentSourceFile?.text || "", pos + 1, () => result = true);
Copy link
Member

Choose a reason for hiding this comment

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

why are these additional ends needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sandersn I did this because I wanted to avoid JSDoc nodes duplication in .d.ts file, e.g.

// file.js
/**
 * @overload
 * @param {string} value
 * @return {string}
 */
/**
 * @overload
 * @param {number} value
 * @return {number}
 */
/**
 * @overload
 * @param {unknown} value
 */
function myFunction(value) {}

would result in:

// file.d.ts
/**
 * @overload
 * @param {string} value
 */
declare function myFunction(value: string): string;
/**
 * @overload
 * @param {number} value
 */
declare function myFunction(value: number): number;
/**
 * @overload
 * @param {string} value
 */
/**
 * @overload
 * @param {number} value
 */
/**
 * @overload
 * @param {unknown} value
 */
declare function myFunction(value);

Being able to specify end allowed me to limit the JSDocs that are emitted for a given parent node.

However this seems to only be one of many weird consequences of synthesizing declarations, which as you suggested does not seem to be the proper approach anyway.

forEachTrailingCommentRange(currentSourceFile?.text || "", pos + 1, /*end*/ undefined, () => result = true);
return result;
}

function hasLeadingCommentsAtPosition(pos: number) {
let result = false;
forEachLeadingCommentRange(currentSourceFile?.text || "", pos + 1, () => result = true);
forEachLeadingCommentRange(currentSourceFile?.text || "", pos + 1, /*end*/ undefined, () => result = true);
return result;
}

Expand Down Expand Up @@ -5444,7 +5445,7 @@ namespace ts {
// Emit leading comments if the position is not synthesized and the node
// has not opted out from emitting leading comments.
if (!skipLeadingComments) {
emitLeadingComments(pos, /*isEmittedNode*/ node.kind !== SyntaxKind.NotEmittedStatement);
emitLeadingComments(pos, end, /*isEmittedNode*/ node.kind !== SyntaxKind.NotEmittedStatement);
}

if (!skipLeadingComments || (pos >= 0 && (emitFlags & EmitFlags.NoLeadingComments) !== 0)) {
Expand Down Expand Up @@ -5543,7 +5544,7 @@ namespace ts {

enterComment();
if (!skipTrailingComments) {
emitLeadingComments(detachedRange.end, /*isEmittedNode*/ true);
emitLeadingComments(detachedRange.end, /*end*/ undefined, /*isEmittedNode*/ true);
if (hasWrittenComment && !writer.isAtStartOfLine()) {
writer.writeLine();
}
Expand Down Expand Up @@ -5576,15 +5577,15 @@ namespace ts {
return prevNodeIndex !== undefined && prevNodeIndex > -1 && parentNodeArray!.indexOf(nextNode) === prevNodeIndex + 1;
}

function emitLeadingComments(pos: number, isEmittedNode: boolean) {
function emitLeadingComments(pos: number, end: number | undefined, isEmittedNode: boolean) {
hasWrittenComment = false;

if (isEmittedNode) {
if (pos === 0 && currentSourceFile?.isDeclarationFile) {
forEachLeadingCommentToEmit(pos, emitNonTripleSlashLeadingComment);
forEachLeadingCommentToEmit(pos, end, emitNonTripleSlashLeadingComment);
}
else {
forEachLeadingCommentToEmit(pos, emitLeadingComment);
forEachLeadingCommentToEmit(pos, end, emitLeadingComment);
}
}
else if (pos === 0) {
Expand All @@ -5596,7 +5597,7 @@ namespace ts {
// /// <reference-path ...>
// interface F {}
// The first /// will NOT be removed while the second one will be removed even though both node will not be emitted
forEachLeadingCommentToEmit(pos, emitTripleSlashLeadingComment);
forEachLeadingCommentToEmit(pos, end, emitTripleSlashLeadingComment);
}
}

Expand Down Expand Up @@ -5644,7 +5645,7 @@ namespace ts {
return;
}

emitLeadingComments(pos, /*isEmittedNode*/ true);
emitLeadingComments(pos, /*end*/ undefined, /*isEmittedNode*/ true);
}

function emitTrailingComments(pos: number) {
Expand Down Expand Up @@ -5705,22 +5706,22 @@ namespace ts {
}
}

function forEachLeadingCommentToEmit(pos: number, cb: (commentPos: number, commentEnd: number, kind: SyntaxKind, hasTrailingNewLine: boolean, rangePos: number) => void) {
function forEachLeadingCommentToEmit(pos: number, end: number | undefined, cb: (commentPos: number, commentEnd: number, kind: SyntaxKind, hasTrailingNewLine: boolean, rangePos: number) => void) {
// Emit the leading comments only if the container's pos doesn't match because the container should take care of emitting these comments
if (currentSourceFile && (containerPos === -1 || pos !== containerPos)) {
if (hasDetachedComments(pos)) {
forEachLeadingCommentWithoutDetachedComments(cb);
}
else {
forEachLeadingCommentRange(currentSourceFile.text, pos, cb, /*state*/ pos);
forEachLeadingCommentRange(currentSourceFile.text, pos, end, cb, /*state*/ pos);
}
}
}

function forEachTrailingCommentToEmit(end: number, cb: (commentPos: number, commentEnd: number, kind: SyntaxKind, hasTrailingNewLine: boolean) => void) {
// Emit the trailing comments only if the container's end doesn't match because the container should take care of emitting these comments
if (currentSourceFile && (containerEnd === -1 || (end !== containerEnd && end !== declarationListContainerEnd))) {
forEachTrailingCommentRange(currentSourceFile.text, end, cb);
forEachTrailingCommentRange(currentSourceFile.text, end, /*end*/ undefined, cb);
}
}

Expand All @@ -5739,7 +5740,7 @@ namespace ts {
detachedCommentsInfo = undefined;
}

forEachLeadingCommentRange(currentSourceFile.text, pos, cb, /*state*/ pos);
forEachLeadingCommentRange(currentSourceFile.text, pos, /*end*/ undefined, cb, /*state*/ pos);
}

function emitDetachedCommentsAndUpdateCommentsInfo(range: TextRange) {
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/factory/nodeFactory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,8 @@ namespace ts {
get updateJSDocReadonlyTag() { return getJSDocSimpleTagUpdateFunction<JSDocReadonlyTag>(SyntaxKind.JSDocReadonlyTag); },
get createJSDocOverrideTag() { return getJSDocSimpleTagCreateFunction<JSDocOverrideTag>(SyntaxKind.JSDocOverrideTag); },
get updateJSDocOverrideTag() { return getJSDocSimpleTagUpdateFunction<JSDocOverrideTag>(SyntaxKind.JSDocOverrideTag); },
get createJSDocOverloadTag() { return getJSDocSimpleTagCreateFunction<JSDocOverloadTag>(SyntaxKind.JSDocOverloadTag); },
get updateJSDocOverloadTag() { return getJSDocSimpleTagUpdateFunction<JSDocOverloadTag>(SyntaxKind.JSDocOverloadTag); },
get createJSDocDeprecatedTag() { return getJSDocSimpleTagCreateFunction<JSDocDeprecatedTag>(SyntaxKind.JSDocDeprecatedTag); },
get updateJSDocDeprecatedTag() { return getJSDocSimpleTagUpdateFunction<JSDocDeprecatedTag>(SyntaxKind.JSDocDeprecatedTag); },
createJSDocUnknownTag,
Expand Down Expand Up @@ -4630,6 +4632,7 @@ namespace ts {
: node;
}


// @api
function createJSDocAugmentsTag(tagName: Identifier | undefined, className: JSDocAugmentsTag["class"], comment?: string | NodeArray<JSDocComment>): JSDocAugmentsTag {
const node = createBaseJSDocTag<JSDocAugmentsTag>(SyntaxKind.JSDocAugmentsTag, tagName ?? createIdentifier("augments"), comment);
Expand Down Expand Up @@ -6334,6 +6337,7 @@ namespace ts {
case SyntaxKind.JSDocProtectedTag: return "protected";
case SyntaxKind.JSDocReadonlyTag: return "readonly";
case SyntaxKind.JSDocOverrideTag: return "override";
case SyntaxKind.JSDocOverloadTag: return "overload";
case SyntaxKind.JSDocTemplateTag: return "template";
case SyntaxKind.JSDocTypedefTag: return "typedef";
case SyntaxKind.JSDocParameterTag: return "param";
Expand Down
4 changes: 4 additions & 0 deletions src/compiler/factory/nodeTests.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -898,6 +898,10 @@ namespace ts {
return node.kind === SyntaxKind.JSDocOverrideTag;
}

export function isJSDocOverloadTag(node: Node): node is JSDocOverloadTag {
return node.kind === SyntaxKind.JSDocOverloadTag;
}

export function isJSDocDeprecatedTag(node: Node): node is JSDocDeprecatedTag {
return node.kind === SyntaxKind.JSDocDeprecatedTag;
}
Expand Down
3 changes: 3 additions & 0 deletions src/compiler/parser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8359,6 +8359,9 @@ namespace ts {
case "override":
tag = parseSimpleTag(start, factory.createJSDocOverrideTag, tagName, margin, indentText);
break;
case "overload":
tag = parseSimpleTag(start, factory.createJSDocOverloadTag, tagName, margin, indentText);
break;
case "deprecated":
hasDeprecatedTag = true;
tag = parseSimpleTag(start, factory.createJSDocDeprecatedTag, tagName, margin, indentText);
Expand Down
Loading