-
Notifications
You must be signed in to change notification settings - Fork 12.8k
JSDoc: make it a noImplicitAny error to leave out Array or Promise type argument #32766
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
Comments
@sandersn can you collect data on this when you have a chance? |
A few things:
@brendankenny for (1) and (2) |
The any-filling behaviour in JS probably means that It might need to interact with its parent, getTypeFromTypeReference, to correctly instantiate arrays and promises, making it a bit more complicated. |
I might not have been precise enough with my suggestion. I was only suggesting fixing the explicit-jsdoc-type case, e.g. // @noImplicitAny: true
/** @type {Array} */ // Array<any>, no error
const arr = []; which is already an error for most parameterized types, like // @noImplicitAny: true
/** @type {Set} */ // error: Generic type 'Set<T>' requires 1 type argument(s)
const set = new Set(); un-annotated code is happy to go with // @noImplicitAny: true
const set = new Set(); // Happily Set<any> with no error which it seems might be what you're referring to here? That would be amazing to start erroring (I guess with a similar suggestion in an error message as the edit: actually I see now that
Hopefully this reference makes more sense after limiting the scope above. It looks like it was a side effect of adding the extends check in #18778? It's from the fallback condition in |
for reference, I was looking at something as simple as https://github.com/microsoft/TypeScript/compare/master...brendankenny:e00d71f?expand=1 (#32829) That commit also removes the no-argument That |
@brendankenny I think you were precise enough, I just mis-remembered the rules for jsdoc types. =) This seems like a good idea -- ideally Array and Promise would behave the same as other types, we just need to make them lenient in the default case. Removing the Now that I understand the bug, I think the main interesting data collection question is how many tsconfigs in the world have both |
Would the next step be to open a PR and decide the question there or to decide it first before opening a change? |
Either one is fine with me. I don't think the number is very high, but I haven't come up with a way to estimate the number yet either. |
I sampled some npm packages and looked at their github repos for tsconfig or jsconfig at the root. Of almost 5000 repos, this turned up 250 tsconfigs and 5 jsconfigs. Of those, half had strict or noImplicitAny on, 13 had allowJs on, and 3 had checkJS on. Only 5 had allowJs+strict/noImplicitAny and none of the 3 checkJs projects had strict on. I'm still sampling, but I think that means we can do whatever we want with strict+checkJs since so few people are using it right now. |
Search Terms
javascript array promise any default type argument
Suggestion
If
--noImplicitAny
is on, make it an error to leave out the type argument forArray
orPromise
in JSDoc annotations.#14284 added support for capitalization aliases commonly found in jsdoc annotations (
Number
when they meantnumber
) and also made/** @type {Array}
an alias for/** @type {Array<any>}
and/** @type {Promise}
an alias for/** @type {Promise<any>}
. From a lot of the jsdoc I've seen, this is probably still the right call (and often exactly what the author meant) when not in a--strict
checking environment.#18778 made it an error to leave out the type argument for generics in JS if
noImplicitAny
is set, but that doesn't catchArray
andPromise
because their type is set beforegetTypeReferenceType
can be called and the error found.It would be great to finish that and always have the type argument warning when
noImplicitAny
is set.If this idea sounds ok, I can put up a PR since it looks like a pretty simple change (just fall through
getIntendedTypeFromJSDocTypeReference
for these types ifnoImplicitAny
is set).Use Cases
Better alerting of the stealth
any
s introduced by/** @type {Array} */
and/** @type {Promise} */
to match how e.g./** @type {Set} */
gives'Generic type 'Set<T>' requires 1 type argument(s)'
.Checklist
My suggestion meets these guidelines:
I guess this may technically be a breaking change (the current behavior was intended by #14284), but since it would be limited to
noImplicitAny
/strict
users and it aligns with #18778's change for all other types, it seems ok?The text was updated successfully, but these errors were encountered: