Skip to content

fix(45901): Error 1064 should apply to @callback definitions in the same way as @param, but doesn't #54625

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

Merged
merged 7 commits into from
Jul 25, 2023

Conversation

a-tarasyuk
Copy link
Contributor

Fixes #45901

…sdoc type related to async functions/methods
@typescript-bot typescript-bot added the For Backlog Bug PRs that fix a backlog bug label Jun 12, 2023
@sandersn
Copy link
Member

I think the basic approach is fine, and as a targetted fix, this just needs a little cleanup. Couple of points to follow up on, though:

  1. I would inline getReturnInfoOfSignatureDeclaration since it's only used once and it allocates (unless the VM optimises it).
  2. I thought there was a utility function that checks for a @type tag if getEffectiveReturnTypeNode returns undefined. Is that utility appropriate here? It probably doesn't return enough information though -- my memory is that it returns a Type instead of a Node. However, if you can find that utility, you should look at its callers to see if they could improve their error messages if they called getReturnInfoOfSignatureDeclaration instead.

I also need to think about the wording of the new error message. It's a little off but I haven't figured out an improvement.

@fatcerberus
Copy link

Shouldn't the error message be the same as other instances of this error? i.e.

The return type of an async function or method must be the global Promise<T> type.

I'm not sure why a new error message is even needed.

@a-tarasyuk
Copy link
Contributor Author

@sandersn Thanks for the feedback. There is a utility called getReturnTypeFromAnnotation that returns a Type. This utility handles cases with JSDoc type tags by utilizing getReturnTypeOfTypeTag, which also returns a Type. We can attempt to use the type instead of the TypeNode to perform all checks. However, we still need the return TypeNode for error messages.

I would inline getReturnInfoOfSignatureDeclaration since it's only used once and it allocates (unless the VM optimises it)

Are you suggesting moving the logic from getReturnInfoOfSignatureDeclaration directly into checkSignatureDeclarationDiagnostics or relocating getReturnInfoOfSignatureDeclaration to the scope of checkSignatureDeclarationDiagnostics?

@sandersn
Copy link
Member

Sorry I dropped this. Now that I look at it again:

  1. Move the logic directly into checkSignatureDeclarationDiagnostics.
  2. I now disagree with my past self: I don't think it's worthwhile to reuse getReturnTypeFromAnnotation.

@a-tarasyuk a-tarasyuk marked this pull request as ready for review July 24, 2023 21:18
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.

re @fatcerberus 's comment: the did-you-mean isn't appropriate since the change needs to be on the callback tag, but the rest of the error still applies.

@a-tarasyuk a-tarasyuk requested a review from sandersn July 25, 2023 08:01
@sandersn sandersn merged commit 9b4aa36 into microsoft:main Jul 25, 2023
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
None yet
Development

Successfully merging this pull request may close these issues.

Error 1064 should apply to @callback definitions in the same way as @param, but doesn't
4 participants