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

Don't inherit jsdoc tags from overloaded signatures #43165

Merged
merged 1 commit into from
Mar 9, 2021

Conversation

sandersn
Copy link
Member

@sandersn sandersn commented Mar 9, 2021

Previously, when getting jsdoc for signatures, the services layer would get the jsdoc tags for the base symbol of a signature if it was
present. This is fine except when the base was overloaded. In that case, the multiple signatures of the overload would all contribute jsdoc, which is not correct.

A more correct fix would be to resolve overloads to the base, but the compiler doesn't have this capability and adding it or jury-rigging it seems like it would be complex, inappropriate for a fix to ship in a patch version.

Co-authored-by: Orta Therox [email protected]

Fixes #43053

Previously, when getting jsdoc for signatures, the services layer would
get the jsdoc tags for the base symbol of a signature if it was
present. This is fine except when the base was overloaded. In that case,
the multiple signatures of the overload would all contribute jsdoc,
which is not correct.

A more correct fix would be to resolve overloads to the base, but
the compiler doesn't have this capability and adding it or jury-rigging
it seems like it would be complex, inappropriate for a fix to ship in a
patch version.

Co-authored-by: Orta Therox <[email protected]>
@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Mar 9, 2021
"kind": "punctuation"
}
],
"documentation": []
Copy link
Member Author

Choose a reason for hiding this comment

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

this line is the assertion we're interested in

tags = [...inheritedTags, ...tags];
}
});
function getJsDocTagsOfSignature(declaration: Declaration, checker: TypeChecker): JSDocTagInfo[] {
Copy link
Member Author

Choose a reason for hiding this comment

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

This function is only called here, and is short, so I wanted to inline it, but it was non-trivial so I skipped it to keep the diff smaller. I couldn't help myself simplifying out the unused declarations: Declaration[] to declaration: Declaration, though.

@sandersn
Copy link
Member Author

sandersn commented Mar 9, 2021

Review reads better with whitespace ignored

@sandersn sandersn requested a review from orta March 9, 2021 18:03
@sandersn
Copy link
Member Author

sandersn commented Mar 9, 2021

@a-tarasyuk you might be interested in this. I'm curious whether you have any clever ideas for resolving a call to its base interface.

Copy link
Contributor

@orta orta left a comment

Choose a reason for hiding this comment

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

Looks like it only changes the behavior we want 👍🏻

@sandersn sandersn merged commit 3d53661 into master Mar 9, 2021
@sandersn sandersn deleted the dont-inherit-signature-jsdoc-from-overloads branch March 9, 2021 20:28
@johnjesse
Copy link

Is there a plan to release a version of 4.2 with this fix in it? Or just role this into 4.3?

Right now I'm attempting to upgrade our repo to webpack 5 -> which means I need ts 4.2 (for the lib changes) and that is causing the eslint-plugin-deprecation to flag up deprecations on function overloads that aren't deprecated. Whilst I could disable that rule - I'd rather not if you're expecting to release the fix soon

@orta
Copy link
Contributor

orta commented Mar 10, 2021

Because the TC39 meetings are currently happening it's a little chaotic for another day or two on our side, but I'd say this is very likely heading towards a 4.2 patch.

orta added a commit to orta/TypeScript that referenced this pull request Mar 10, 2021
Previously, when getting jsdoc for signatures, the services layer would
get the jsdoc tags for the base symbol of a signature if it was
present. This is fine except when the base was overloaded. In that case,
the multiple signatures of the overload would all contribute jsdoc,
which is not correct.

A more correct fix would be to resolve overloads to the base, but
the compiler doesn't have this capability and adding it or jury-rigging
it seems like it would be complex, inappropriate for a fix to ship in a
patch version.

Co-authored-by: Orta Therox <[email protected]>

Co-authored-by: Orta Therox <[email protected]>
orta pushed a commit that referenced this pull request Mar 24, 2021
* Don't inherit jsdoc tags from overloaded signatures (#43165)

Previously, when getting jsdoc for signatures, the services layer would
get the jsdoc tags for the base symbol of a signature if it was
present. This is fine except when the base was overloaded. In that case,
the multiple signatures of the overload would all contribute jsdoc,
which is not correct.

A more correct fix would be to resolve overloads to the base, but
the compiler doesn't have this capability and adding it or jury-rigging
it seems like it would be complex, inappropriate for a fix to ship in a
patch version.

Co-authored-by: Orta Therox <[email protected]>

Co-authored-by: Orta Therox <[email protected]>

* Update baseline

Co-authored-by: Nathan Shively-Sanders <[email protected]>
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 4.2.4 milestone Apr 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unexpected deprecation warning after upgrading to TypeScript v4.2.2
5 participants