JSDoc overload tag #51234

apendua · 2022-10-19T20:09:05Z

This PR replaces an earlier attempt: #50789

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.

apendua · 2022-10-19T20:14:56Z

@microsoft-github-policy-service agree

sandersn

This looks great. Clever re-use of existing infrastructure for @callback. Just a few questions and requests for additional tests. I also want to think about the design a bit more, but so far I think it's solid.

tests/baselines/reference/jsFileFunctionOverloads.types

sandersn · 2022-11-15T19:13:02Z

tests/baselines/reference/jsFileFunctionOverloads.symbols

+=== tests/cases/compiler/jsFileFunctionOverloads.js ===
+/**
+ * @overload
+ * @param {number} x


I'm not sure what happens if you find-all-ref or rename the overload parameters. In Typescript, they are not connected to the implementation signatures and don't rename.

Can you add two tests in cases/fourslash, one that renames an overload param and one that find-all-refs it, and shows that overload params are not connected to the implementation? There should be existing tests for @param that you can start from.

I only tried this in VSCode, and it seems like your assumptions are correct, i.e. the overload parameters are generally ignored on both find-all-ref and rename operations.

I would be happy to add those tests, but can you please point me to an example where a similar test was already implemented? I am fairly new to this codebase and navigating around (especially within tests) is still challenging 😄

I've added tests for find-all-ref and rename as requested.

sandersn · 2022-11-15T19:17:39Z

src/compiler/checker.ts

+                        if (node.tags) {
+                            for (const tag of node.tags) {
+                                if (isJSDocOverloadTag(tag)) {
+                                    result.push(getSignatureFromDeclaration(tag.typeExpression));


Operations that go from a signature back to a declaration might fail if the signature is a JSDocSignature. But (1) JSDocCallbackTag already exists, and has shaken out those bugs. (2) I can't think of any specific places; the only new ones would be ones that operate on symbols with multiple signatures.

Is there anything in particular here that you would like me to look into?

sandersn · 2022-11-15T19:17:51Z

tests/baselines/reference/jsFileFunctionOverloads.symbols

+ */
+/**


what happens if all the @overload tags are in one comment? Does that work too?

Yes, actually it does. At least the examples I checked locally seem to work properly. Do you think we should be adding this as a test case?

Just in case, I've also added another variant of tests, which tries to put all @overload tags in a single comment. It seems like things get tricky (unsurprisingly) if you also want to use @template tag, as you can only have a single "scope" for template parameters per JSDoc comment. This problem is not specific to @overload tag though, and I think it should be good enough for now if it's covered in documentation properly. Perhaps this could be improved in the future if we allow @template to be parsed as part of JSDocSignature?

Yes, I think it's good enough for now.

sandersn · 2022-11-15T19:18:44Z

tests/baselines/reference/jsFileMethodOverloads.types

+
+  /**
+   * @overload
+   * @this {Example<'number'>}


I don't see this in the type. Does this actually work?

Thank you for pointing this out! I forgot that JSDocSignature does not support @this tag, but it seems like @param {...} this works well. I've just pushed a fix.

sandersn · 2022-11-15T21:22:30Z

I did a quick grep of JS code (all the JS that gets installed when installing the dependencies of every package on Definitely Typed). This found one existing use of @overload, in the AWS SDK. The use is similar to this PR, except that it's used on functions with a single signature, and also on classes. Can you add some tests that reflect that use? They don't have to behave the way that the authors meant, they just need to not crash. =)

Here are the interesting cases I've found so far:

callable constructor function

  /**
   * @overload Endpoint(endpoint)
   *   Constructs a new endpoint given an endpoint URL. If the
   *   URL omits a protocol (http or https), the default protocol
   *   set in the global {AWS.config} will be used.
   *   @param endpoint [String] the URL to construct an endpoint from
   */
  constructor: function Endpoint(endpoint, config) {

callable constructor function with overloads

Note that the second overload has zero param tags, just @option and @example.

  /**
   * A credentials object can be created using positional arguments or an options
   * hash.
   *
   * @overload AWS.Credentials(accessKeyId, secretAccessKey, sessionToken=null)
   *   Creates a Credentials object with a given set of credential information
   *   as positional arguments.
   *   @param accessKeyId [String] the AWS access key ID
   *   @param secretAccessKey [String] the AWS secret access key
   *   @param sessionToken [String] the optional AWS session token
   *   @example Create a credentials object with AWS credentials
   *     var creds = new AWS.Credentials('akid', 'secret', 'session');
   * @overload AWS.Credentials(options)
   *   Creates a Credentials object with a given set of credential information
   *   as an options hash.
   *   @option options accessKeyId [String] the AWS access key ID
   *   @option options secretAccessKey [String] the AWS secret access key
   *   @option options sessionToken [String] the optional AWS session token
   *   @example Create a credentials object with AWS credentials
   *     var creds = new AWS.Credentials({
   *       accessKeyId: 'akid', secretAccessKey: 'secret', sessionToken: 'session'
   *     });
   */
  constructor: function Credentials() {

The associated hand-written overload set:

    constructor(options: CredentialsOptions);
    constructor(accessKeyId: string, secretAccessKey: string, sessionToken?: string);

single signature with split return types

There's a lot going on here that would be good to test:

function in object literal property assignment
double return tags; I think this is the 'overload' part
@callback inline type definition impllicitly covering the third parameter

Again, this doesn't need to have the correct semantics, it just needs to not crash.

    /**
     * @overload getSynthesizeSpeechUrl(params = {}, [expires = 3600], [callback])
     *   Generate a presigned url for {AWS.Polly.synthesizeSpeech}.
     *   @note You must ensure that you have static or previously resolved
     *     credentials if you call this method synchronously (with no callback),
     *     otherwise it may not properly sign the request. If you cannot guarantee
     *     this (you are using an asynchronous credential provider, i.e., EC2
     *     IAM roles), you should always call this method with an asynchronous
     *     callback.
     *   @param params [map] parameters to pass to the operation. See the {AWS.Polly.synthesizeSpeech}
     *     operation for the expected operation parameters.
     *   @param expires [Integer] (3600) the number of seconds to expire the pre-signed URL operation in.
     *     Defaults to 1 hour.
     *   @return [string] if called synchronously (with no callback), returns the signed URL.
     *   @return [null] nothing is returned if a callback is provided.
     *   @callback callback function (err, url)
     *     If a callback is supplied, it is called when a signed URL has been generated.
     *     @param err [Error] the error object returned from the presigner.
     *     @param url [String] the signed URL.
     *   @see AWS.Polly.synthesizeSpeech
     */
    getSynthesizeSpeechUrl: function getSynthesizeSpeechUrl(params, expires, callback) {

Here's the hand-created .d.ts overload set for comparison:

  getSynthesizeSpeechUrl(params: Polly.Types.SynthesizeSpeechInput, error: number, callback: (err: AWSError, url: string) => void): void;
  getSynthesizeSpeechUrl(params: Polly.Types.SynthesizeSpeechInput, callback: (err: AWSError, url: string) => void): void;
  getSynthesizeSpeechUrl(params: Polly.Types.SynthesizeSpeechInput, expires?: number): string;

apendua · 2022-11-16T05:50:46Z

@sandersn

I did a quick grep of JS code (all the JS that gets installed when installing the dependencies of every package on Definitely Typed).

Thank you for taking the effort to find these examples!

They don't have to behave the way that the authors meant, they just need to not crash. =)

Sure, I will try to add the tests for the use cases that you listed.

apendua · 2022-12-03T23:14:14Z

@sandersn I pushed some additional tests and also used this opportunity to resolve some conflicts. It would be great if you could have a second look 😄

sandersn · 2022-12-13T22:32:37Z

tests/cases/compiler/jsFileMethodOverloads2.ts

+// @declaration: true
+// @filename: jsFileMethodOverloads2.js
+
+// Also works if all @overload tags are combined in one comment.


sandersn · 2022-12-13T22:33:22Z

tests/cases/compiler/jsFileMethodOverloads.ts

+
+  /**
+   * @overload
+   * @param {Example<number>} this


huh. I had no idea this worked.

apendua · 2022-12-13T22:57:00Z

@sandersn Thank you for the review and for the positive feedback!

@overload

Also, make @overload work like other jsdoc tags: only the last jsdoc tag before a declaration is used. That means all overload tags need to be in one tag: ```js /** * @overload * @return {number} * * @overload * @return {string} */ ``` function f() { return "1" } This no longer works: ```js /** * @overload * @return {number} */ /** * @overload * @return {string} */ function f() { return "2" } ``` Fixes microsoft#51234

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

apendua mentioned this pull request Oct 19, 2022

Js doc overloads #50789

Closed

RyanCavanaugh requested a review from sandersn October 19, 2022 20:53

sandersn added this to Not started in PR Backlog Oct 27, 2022

sandersn self-assigned this Nov 15, 2022

sandersn moved this from Not started to Waiting on reviewers in PR Backlog Nov 15, 2022

sandersn requested changes Nov 15, 2022

View reviewed changes

PR Backlog automation moved this from Waiting on reviewers to Waiting on author Nov 15, 2022

apendua added 7 commits December 3, 2022 23:35

Add support for JSDocOverloadTag

7f31b7a

Use overload tag to determine function type

a6cb519

Update baselines

dd65e9a

Add new tests along with baselines

f659657

Add tests for all @overload tags in one comment

7db2335

Add tests for find-all-ref and rename operations

52a94ca

Add tests for alternative uses of @overload tag

f0ec97c

apendua force-pushed the js-doc-overload-tag branch from 3d0f56b to f0ec97c Compare December 3, 2022 22:42

apendua requested a review from sandersn December 3, 2022 23:14

sandersn reviewed Dec 13, 2022

View reviewed changes

tests/cases/compiler/jsFileMethodOverloads.ts

/**

* @overload

* @param {Example<number>} this

Copy link

Member

sandersn Dec 13, 2022

Choose a reason for hiding this comment

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

huh. I had no idea this worked.

sandersn approved these changes Dec 13, 2022

View reviewed changes

PR Backlog automation moved this from Waiting on author to Needs merge Dec 13, 2022

Merge branch 'main' into pr/apendua/51234

bf10df0

sandersn merged commit e4816ed into microsoft:main Dec 13, 2022

PR Backlog automation moved this from Needs merge to Done Dec 13, 2022

DanielRosenwasser mentioned this pull request Jan 23, 2023

@overload doesn't check implementation against overload signatures #52367

Closed

sandersn mentioned this pull request Jan 28, 2023

Check overload tag against implementation #52474

Merged

anonbergine mentioned this pull request Jan 28, 2023

@overload doesn't work for constructors #52477

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

JSDoc overload tag #51234

JSDoc overload tag #51234

apendua commented Oct 19, 2022

apendua commented Oct 19, 2022

sandersn left a comment

sandersn Nov 15, 2022

apendua Nov 16, 2022

apendua Dec 3, 2022

sandersn Nov 15, 2022

apendua Nov 16, 2022

sandersn Nov 15, 2022

apendua Nov 16, 2022

apendua Dec 3, 2022

sandersn Dec 13, 2022

sandersn Nov 15, 2022

apendua Nov 16, 2022

sandersn commented Nov 15, 2022

apendua commented Nov 16, 2022

apendua commented Dec 3, 2022

sandersn Dec 13, 2022

sandersn Dec 13, 2022

apendua commented Dec 13, 2022

JSDoc overload tag #51234

JSDoc overload tag #51234

Conversation

apendua commented Oct 19, 2022

apendua commented Oct 19, 2022

sandersn left a 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

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 commented Nov 15, 2022

callable constructor function

callable constructor function with overloads

single signature with split return types

apendua commented Nov 16, 2022

apendua commented Dec 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apendua commented Dec 13, 2022