-
Notifications
You must be signed in to change notification settings - Fork 12.8k
No error on unmatchable @param
tags
#22510
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
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
0dd9a99
No errr on unmatchable `@param` tags
sandersn 2d3440c
Do not require dummy param for JS uses of arguments
sandersn 43e20d0
Check for array type instead of syntactic `...`
sandersn b252e1c
Merge branch 'master' into no-error-on-unmatchable-param-tags
sandersn 24e014d
Address PR comments
sandersn 70af333
Update baselines
sandersn File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
14 changes: 14 additions & 0 deletions
14
tests/baselines/reference/paramTagOnCallExpression.symbols
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
=== tests/cases/conformance/jsdoc/decls.d.ts === | ||
declare function factory(type: string): {}; | ||
>factory : Symbol(factory, Decl(decls.d.ts, 0, 0)) | ||
>type : Symbol(type, Decl(decls.d.ts, 0, 25)) | ||
|
||
=== tests/cases/conformance/jsdoc/a.js === | ||
// from util | ||
/** @param {function} ctor - A big long explanation follows */ | ||
exports.inherits = factory('inherits') | ||
>exports.inherits : Symbol(inherits, Decl(a.js, 0, 0)) | ||
>exports : Symbol(inherits, Decl(a.js, 0, 0)) | ||
>inherits : Symbol(inherits, Decl(a.js, 0, 0)) | ||
>factory : Symbol(factory, Decl(decls.d.ts, 0, 0)) | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
=== tests/cases/conformance/jsdoc/decls.d.ts === | ||
declare function factory(type: string): {}; | ||
>factory : (type: string) => {} | ||
>type : string | ||
|
||
=== tests/cases/conformance/jsdoc/a.js === | ||
// from util | ||
/** @param {function} ctor - A big long explanation follows */ | ||
exports.inherits = factory('inherits') | ||
>exports.inherits = factory('inherits') : {} | ||
>exports.inherits : {} | ||
>exports : typeof "tests/cases/conformance/jsdoc/a" | ||
>inherits : {} | ||
>factory('inherits') : {} | ||
>factory : (type: string) => {} | ||
>'inherits' : "inherits" | ||
|
31 changes: 31 additions & 0 deletions
31
tests/baselines/reference/paramTagOnFunctionUsingArguments.errors.txt
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
tests/cases/conformance/jsdoc/a.js(2,20): error TS8029: The last @param tag of a function that uses 'arguments' must have an array type. | ||
tests/cases/conformance/jsdoc/a.js(19,9): error TS2345: Argument of type '1' is not assignable to parameter of type 'string'. | ||
|
||
|
||
==== tests/cases/conformance/jsdoc/decls.d.ts (0 errors) ==== | ||
declare function factory(type: string): {}; | ||
==== tests/cases/conformance/jsdoc/a.js (2 errors) ==== | ||
/** | ||
* @param {string} first | ||
~~~~~ | ||
!!! error TS8029: The last @param tag of a function that uses 'arguments' must have an array type. | ||
*/ | ||
function concat(/* first, second, ... */) { | ||
var s = '' | ||
for (var i = 0, l = arguments.length; i < l; i++) { | ||
s += arguments[i] | ||
} | ||
return s | ||
} | ||
|
||
/** | ||
* @param {...string} strings | ||
*/ | ||
function correct() { | ||
arguments | ||
} | ||
|
||
correct(1,2,3) // oh no | ||
~ | ||
!!! error TS2345: Argument of type '1' is not assignable to parameter of type 'string'. | ||
|
47 changes: 47 additions & 0 deletions
47
tests/baselines/reference/paramTagOnFunctionUsingArguments.symbols
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
=== tests/cases/conformance/jsdoc/decls.d.ts === | ||
declare function factory(type: string): {}; | ||
>factory : Symbol(factory, Decl(decls.d.ts, 0, 0)) | ||
>type : Symbol(type, Decl(decls.d.ts, 0, 25)) | ||
|
||
=== tests/cases/conformance/jsdoc/a.js === | ||
/** | ||
* @param {string} first | ||
*/ | ||
function concat(/* first, second, ... */) { | ||
>concat : Symbol(concat, Decl(a.js, 0, 0)) | ||
|
||
var s = '' | ||
>s : Symbol(s, Decl(a.js, 4, 5)) | ||
|
||
for (var i = 0, l = arguments.length; i < l; i++) { | ||
>i : Symbol(i, Decl(a.js, 5, 10)) | ||
>l : Symbol(l, Decl(a.js, 5, 17)) | ||
>arguments.length : Symbol(IArguments.length, Decl(lib.d.ts, --, --)) | ||
>arguments : Symbol(arguments) | ||
>length : Symbol(IArguments.length, Decl(lib.d.ts, --, --)) | ||
>i : Symbol(i, Decl(a.js, 5, 10)) | ||
>l : Symbol(l, Decl(a.js, 5, 17)) | ||
>i : Symbol(i, Decl(a.js, 5, 10)) | ||
|
||
s += arguments[i] | ||
>s : Symbol(s, Decl(a.js, 4, 5)) | ||
>arguments : Symbol(arguments) | ||
>i : Symbol(i, Decl(a.js, 5, 10)) | ||
} | ||
return s | ||
>s : Symbol(s, Decl(a.js, 4, 5)) | ||
} | ||
|
||
/** | ||
* @param {...string} strings | ||
*/ | ||
function correct() { | ||
>correct : Symbol(correct, Decl(a.js, 9, 1)) | ||
|
||
arguments | ||
>arguments : Symbol(arguments) | ||
} | ||
|
||
correct(1,2,3) // oh no | ||
>correct : Symbol(correct, Decl(a.js, 9, 1)) | ||
|
57 changes: 57 additions & 0 deletions
57
tests/baselines/reference/paramTagOnFunctionUsingArguments.types
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
=== tests/cases/conformance/jsdoc/decls.d.ts === | ||
declare function factory(type: string): {}; | ||
>factory : (type: string) => {} | ||
>type : string | ||
|
||
=== tests/cases/conformance/jsdoc/a.js === | ||
/** | ||
* @param {string} first | ||
*/ | ||
function concat(/* first, second, ... */) { | ||
>concat : (...args: any[]) => string | ||
|
||
var s = '' | ||
>s : string | ||
>'' : "" | ||
|
||
for (var i = 0, l = arguments.length; i < l; i++) { | ||
>i : number | ||
>0 : 0 | ||
>l : number | ||
>arguments.length : number | ||
>arguments : IArguments | ||
>length : number | ||
>i < l : boolean | ||
>i : number | ||
>l : number | ||
>i++ : number | ||
>i : number | ||
|
||
s += arguments[i] | ||
>s += arguments[i] : string | ||
>s : string | ||
>arguments[i] : any | ||
>arguments : IArguments | ||
>i : number | ||
} | ||
return s | ||
>s : string | ||
} | ||
|
||
/** | ||
* @param {...string} strings | ||
*/ | ||
function correct() { | ||
>correct : (...args: string[]) => void | ||
|
||
arguments | ||
>arguments : IArguments | ||
} | ||
|
||
correct(1,2,3) // oh no | ||
>correct(1,2,3) : void | ||
>correct : (...args: string[]) => void | ||
>1 : 1 | ||
>2 : 2 | ||
>3 : 3 | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
// @noEmit: true | ||
// @allowJs: true | ||
// @checkJs: true | ||
// @Filename: decls.d.ts | ||
declare function factory(type: string): {}; | ||
// @Filename: a.js | ||
|
||
// from util | ||
/** @param {function} ctor - A big long explanation follows */ | ||
exports.inherits = factory('inherits') |
27 changes: 27 additions & 0 deletions
27
tests/cases/conformance/jsdoc/paramTagOnFunctionUsingArguments.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
// @noEmit: true | ||
// @allowJs: true | ||
// @checkJs: true | ||
// @strict: true | ||
// @Filename: decls.d.ts | ||
declare function factory(type: string): {}; | ||
// @Filename: a.js | ||
|
||
/** | ||
* @param {string} first | ||
*/ | ||
function concat(/* first, second, ... */) { | ||
var s = '' | ||
for (var i = 0, l = arguments.length; i < l; i++) { | ||
s += arguments[i] | ||
} | ||
return s | ||
} | ||
|
||
/** | ||
* @param {...string} strings | ||
*/ | ||
function correct() { | ||
arguments | ||
} | ||
|
||
correct(1,2,3) // oh no |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This case I'm not so sure about supporting since people will think they're getting typing, when we just throw the type definition away without telling them and accept
any
. Instead we could add a better message telling them to use a rest parameter instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rest parameters don't exist in ES5, so the error would only apply to ES2015 and later. And we don't usually know whether a .js file targets ES2015 or later.
As for throwing away typing, I don't think there's much we can do -- maybe a noImplicitAny error instead?
I think it would be easier to error (to info-error?) whenever there's a reference to
arguments
. That error would be a lot easier to write, and would apply to both ES2015+ and TS, and should come with an associated quick fix. I'll open a separate issue for that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently we require the user to place a dummy-parameter and type it as a rest parameter:
We could make a codefix to convert
@param {string} first
to@param {...string} args
and add the dummy parameter -- we could also stop requiring the dumy parameter.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that. I'll see how hard it is to get the error for the
{string} x
case and then remove it when it's changed to{...string} x
. We can do the code fix later, since @mhegazy wants to get this into 2.8.1.