Skip to content

Make it a noImplicitAny error to fail to provide type arguments to a superclass via @augments #18778

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
7 commits merged into from
Oct 20, 2017

Conversation

ghost
Copy link

@ghost ghost commented Sep 26, 2017

Fixes #17073

Diagnostic message mentions @extends in anticipation of #18706

@ghost ghost requested a review from sandersn September 27, 2017 17:11
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.

How does this behave when the augments tag is itself missing type arguments?

@@ -3511,6 +3511,14 @@
"category": "Error",
"code": 8021
},
"Generic type '{0}' requires '{1}' type arguments; provide these with an '@augments' or '@extends' tag.": {
Copy link
Member

Choose a reason for hiding this comment

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

I'd choose one of @augments or @extends for the error, preferably the one that is in the source if it is already there.

Copy link
Author

Choose a reason for hiding this comment

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

There probably won't be one already there; we might just recommend @extends since that's the more intuitive name.

==== /b.js (1 errors) ====
class B extends A {}
~
!!! error TS8022: Generic type 'A<T>' requires '1' type arguments; provide these with an '@augments' or '@extends' tag.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a test for

/** @augments A */
class B { }

There should be an error on A on the first line, I think.

Copy link
Author

Choose a reason for hiding this comment

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

Requires getting #18775 in first.

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.

Approved after switching to the newer error wording, which also displays numTypeArguments to the user.

"category": "Error",
"code": 8025
},
"Generic type '{0}' requires between '{1}' and '{2}' type arguments; provide these with an '@extends' tag.": {
Copy link
Member

Choose a reason for hiding this comment

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

Basing it on the new error message is better: "Expected 1-2 type arguments, but got 3; provide them with an '@extends' tag."

I didn't notice when I added the new error message that there was an existing one. I filed #19374 to fix that.

@ghost ghost merged commit 8b7d859 into master Oct 20, 2017
@ghost ghost deleted the jsExtendsImplicitAny branch October 20, 2017 16:41
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant