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

Check overload tag against implementation #52474

Merged

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Jan 28, 2023

Edit: It turns out that the semantics below only work for overloads that specify a return type. Otherwise the parser doesn't know where the last @overload ends if it's missing @return. While noImplicitAny prompts to add @return, it's off in most JS scenarios.

I'm still not happy that we're encouraging people to split their tags, but I don't see a good way around it.

Nope, Below Idea Doesn't Work

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:

/**
 * @overload
 * @return {number}
 *
 * @overload
 * @return {string}
 */

function f() { return "1" }

This no longer works:

/**
 * @overload
 * @return {number}
 */
/**
 * @overload
 * @return {string}
 */
function f() { return "2" }

@DanielRosenwasser this is a break from what is described in the beta blog post, but I really want @overload to work the same way as the rest of our jsdoc tags. At the time I changed the existing tag's behaviour, I found that almost nobody split their tags out this way, so I don't want to encourage people to start.

Fixes #52367
Followup to #51234

Check @overload signatures and implementation signatures.
checkFunctionOrConstructorSymbolWorker doesn't recognise functions with @overload tags as overloaded because it's using a simple syntactic check based on TS syntax. This PR adds a check for @overload tags as well.
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
@DanielRosenwasser
Copy link
Member

Looks like you'll need to merge from main due to the noImplicitAny changes.

Copy link
Member

@DanielRosenwasser DanielRosenwasser left a comment

Choose a reason for hiding this comment

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

Looks good - though the placement of the test is weird.

@sandersn sandersn merged commit 4b534dc into microsoft:main Feb 16, 2023
@sandersn sandersn deleted the check-overload-tag-against-implementation branch February 16, 2023 22:42
@jakebailey
Copy link
Member

jakebailey commented Feb 16, 2023

Main is failing after this has been merged; some conflict?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

@overload doesn't check implementation against overload signatures
4 participants