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.

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

JSDoc overload tag #51234

Merged
merged 8 commits into from
Dec 13, 2022
Merged

JSDoc overload tag #51234

merged 8 commits into from
Dec 13, 2022

Conversation

apendua
Copy link
Contributor

@apendua apendua commented Oct 19, 2022

Fixes #25590

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.

@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Oct 19, 2022
@apendua apendua mentioned this pull request Oct 19, 2022
@apendua
Copy link
Contributor Author

apendua commented Oct 19, 2022

@microsoft-github-policy-service agree

@sandersn sandersn self-assigned this Nov 15, 2022
Copy link
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

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/cases/compiler/jsFileFunctionOverloads.js ===
/**
* @overload
* @param {number} x
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Comment on lines +6 to +7
*/
/**
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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


/**
* @overload
* @this {Example<'number'>}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

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
Copy link
Contributor Author

apendua commented Nov 16, 2022

@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 apendua force-pushed the js-doc-overload-tag branch from 3d0f56b to f0ec97c Compare December 3, 2022 22:42
@apendua
Copy link
Contributor Author

apendua commented Dec 3, 2022

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

@apendua apendua requested a review from sandersn December 3, 2022 23:14
// @declaration: true
// @filename: jsFileMethodOverloads2.js

// Also works if all @overload tags are combined in one comment.
Copy link
Member

Choose a reason for hiding this comment

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

👍


/**
* @overload
* @param {Example<number>} this
Copy link
Member

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.

@apendua
Copy link
Contributor Author

apendua commented Dec 13, 2022

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

sandersn added a commit to sandersn/TypeScript that referenced this pull request Jan 28, 2023
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Backlog Bug PRs that fix a backlog bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

In JS, jsdoc should be able to declare functions as overloaded
3 participants