Js doc overloads #50789

apendua · 2022-09-15T14:32:48Z

Since there was no final consensus, I ended up using a new @overload tag to ensure backwards compatibility, e.g.

/**
 * @overload
 * @param {number} x
 * @returns {'number'}
 */
/**
 * @overload
 * @param {string} x
 * @returns {'string'}
 */
/**
 * @overload
 * @param {boolean} x
 * @returns {'boolean'}
 */
/**
 * @param {unknown} x
 * @returns {string}
 */
function typeName(x) {
  return typeof x;
}

I would be happy to modify the implementation if someone suggests a better approach.

ghost · 2022-09-15T14:33:00Z

All CLA requirements met.

ValeTheVioletMote · 2022-10-11T00:43:20Z

Looking forward to this!

sandersn · 2022-10-12T00:06:58Z

src/compiler/emitter.ts

@@ -3660,13 +3661,13 @@ namespace ts {

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


why are these additional ends needed?

@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.

sandersn · 2022-10-12T00:08:17Z

src/compiler/scanner.ts

@@ -747,7 +748,7 @@ namespace ts {
     * @returns If "reduce" is true, the accumulated value. If "reduce" is false, the first truthy
     *      return value of the callback.
     */
-    function iterateCommentRanges<T, U>(reduce: boolean, text: string, pos: number, trailing: boolean, cb: (pos: number, end: number, kind: CommentKind, hasTrailingNewLine: boolean, state: T, memo: U | undefined) => U, state: T, initial?: U): U | undefined {
+     function iterateCommentRanges<T, U>(reduce: boolean, text: string, pos: number, end: number | undefined, trailing: boolean, cb: (pos: number, end: number, kind: CommentKind, hasTrailingNewLine: boolean, state: T, memo: U | undefined) => U, state: T, initial?: U): U | undefined {


same question about end in the file

extra space typo:

Suggested change

function iterateCommentRanges<T, U>(reduce: boolean, text: string, pos: number, end: number | undefined, trailing: boolean, cb: (pos: number, end: number, kind: CommentKind, hasTrailingNewLine: boolean, state: T, memo: U | undefined) => U, state: T, initial?: U): U | undefined {

function iterateCommentRanges<T, U>(reduce: boolean, text: string, pos: number, end: number | undefined, trailing: boolean, cb: (pos: number, end: number, kind: CommentKind, hasTrailingNewLine: boolean, state: T, memo: U | undefined) => U, state: T, initial?: U): U | undefined {

sandersn · 2022-10-12T00:08:36Z

src/services/utilities.ts

@@ -2677,12 +2677,11 @@ namespace ts {
    }

    export function copyLeadingComments(sourceNode: Node, targetNode: Node, sourceFile: SourceFile, commentKind?: CommentKind, hasTrailingNewLine?: boolean) {
-        forEachLeadingCommentRange(sourceFile.text, sourceNode.pos, getAddCommentsFunction(targetNode, sourceFile, commentKind, hasTrailingNewLine, addSyntheticLeadingComment));
+        forEachLeadingCommentRange(sourceFile.text, sourceNode.pos, /*end*/ undefined, getAddCommentsFunction(targetNode, sourceFile, commentKind, hasTrailingNewLine, addSyntheticLeadingComment));


same question about end here

sandersn · 2022-10-12T00:08:58Z

tests/baselines/reference/api/tsserverlibrary.d.ts

@@ -14,7 +14,7 @@ and limitations under the License.
 ***************************************************************************** */

 declare namespace ts {
-    const versionMajorMinor = "4.9";
+    const versionMajorMinor = "4.8";


need to merge from main at some point

sandersn · 2022-10-12T00:10:28Z

tests/cases/compiler/jsFileMethodsOverloads.ts

@@ -0,0 +1,50 @@
+// @allowJs: true
+// @noEmit: true
+// @filename: 0.js


Suggested change

// @filename: 0.js

// @filename: jsFileMethodsOverloads.js

sandersn · 2022-10-12T00:10:42Z

tests/cases/compiler/jsFileFunctionOverloads.ts

@@ -0,0 +1,61 @@
+// @allowJs: true
+// @noEmit: true
+// @filename: 0.js


Suggested change

// @filename: 0.js

// @filename: jsFileFunctionOverloads.js

sandersn · 2022-10-13T20:46:18Z

tests/cases/compiler/jsFileMethodsOverloads.ts

@@ -0,0 +1,50 @@
+// @allowJs: true
+// @noEmit: true


please use // @outDir: dist/ and // @declaration: true to make sure that js->d.ts conversion works too.

@sandersn I tried this but didn't see any changes in the baseline files. What outcome I should be observing?

Did you remove @noEmit: true? You should see a baseline named jsFileMethodsOverloads.js and its last section should be a .d.ts file.

@sandersn Yup, that was it. Seems like it's working fine now. Thank you!

sandersn · 2022-10-13T20:52:01Z

src/compiler/binder.ts

+                const newParameters: ParameterDeclaration[] = [];
+                for (const tag of jsDoc.tags) {
+                    if (isJSDocParameterTag(tag) && tag.name.kind === SyntaxKind.Identifier) {
+                        const newParameter = factory.createParameterDeclaration(


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.

@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?

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.

@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!

apendua · 2022-10-19T20:13:25Z

Closing this PR in favor of: #51234

apendua added 4 commits September 15, 2022 15:19

Add support for JSDocOverloadTag

a51d682

Bind additional declarations from jsDoc

b578825

Update baseline for tests

e4b35c2

Add new tests along with baselines

9f26e98

typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Sep 15, 2022

sandersn requested changes Oct 13, 2022

View reviewed changes

sandersn self-assigned this Oct 17, 2022

sandersn requested a review from navya9singh October 17, 2022 22:39

apendua mentioned this pull request Oct 19, 2022

JSDoc overload tag #51234

Merged

apendua closed this Oct 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Js doc overloads #50789

Js doc overloads #50789

apendua commented Sep 15, 2022

ghost commented Sep 15, 2022 •

edited by ghost

Loading

ValeTheVioletMote commented Oct 11, 2022

sandersn Oct 12, 2022

apendua Oct 18, 2022

sandersn Oct 12, 2022

sandersn Oct 12, 2022

sandersn Oct 12, 2022

sandersn Oct 12, 2022

sandersn Oct 12, 2022

sandersn Oct 13, 2022

apendua Oct 18, 2022

sandersn Oct 18, 2022 •

edited

Loading

apendua Oct 19, 2022

sandersn Oct 13, 2022

apendua Oct 18, 2022

sandersn Oct 18, 2022

apendua Oct 19, 2022

apendua commented Oct 19, 2022

	function iterateCommentRanges<T, U>(reduce: boolean, text: string, pos: number, end: number \| undefined, trailing: boolean, cb: (pos: number, end: number, kind: CommentKind, hasTrailingNewLine: boolean, state: T, memo: U \| undefined) => U, state: T, initial?: U): U \| undefined {
	function iterateCommentRanges<T, U>(reduce: boolean, text: string, pos: number, end: number \| undefined, trailing: boolean, cb: (pos: number, end: number, kind: CommentKind, hasTrailingNewLine: boolean, state: T, memo: U \| undefined) => U, state: T, initial?: U): U \| undefined {

Js doc overloads #50789

Js doc overloads #50789

Conversation

apendua commented Sep 15, 2022

ghost commented Sep 15, 2022 • edited by ghost Loading

ValeTheVioletMote commented Oct 11, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandersn Oct 18, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apendua commented Oct 19, 2022

ghost commented Sep 15, 2022 •

edited by ghost

Loading

sandersn Oct 18, 2022 •

edited

Loading