-
Notifications
You must be signed in to change notification settings - Fork 768
Port implicit any type arguments in JavaScript
#1242
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
Port implicit any type arguments in JavaScript
#1242
Conversation
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.
Pull Request Overview
This PR restores JavaScript behavior by treating missing or default type arguments as any instead of unknown or {}, improving interoperability with existing JS code (e.g., new Set infers Set<any>). Key changes include:
- Extending
fillMissingTypeArgumentswith a JS file check to useanydefaults in JS. - Updating numerous baseline tests to reflect
anydefaults in JS contexts. - Adjusting calls throughout
internal/checkerto pass the newisJavaScriptflag.
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/checker/relater.go | Pass JS context flag to fillMissingTypeArguments in relation logic |
| internal/checker/inference.go | Pass JS context flag to fillMissingTypeArguments in inference |
| internal/checker/jsx.go | Update JSX instantiation calls to include JS context flag |
| internal/checker/checker.go | Widespread updates to signature instantiation, default constructors, and type argument logic to use JS flag |
| testdata/baselines/reference/submoduleAccepted/... | Baselines updated to expect any defaults and altered error counts |
Comments suppressed due to low confidence (1)
|
Minimal repro for the crash is // @filename: foo.js
export class A<T> {}
export class B extends A {
constructor() {
super();
}
}I haven't fixed it in the latest commits, but I was in the neighborhood and found a few places I had missed (or might as well have cleared out some diffs) |
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.
Based on the diffs, this sure seems correct to me. I'm not sure if this stuff was intentionally removed or not, though, but I feel like this should be here. Maybe @sandersn has an opinion, however.
(Also, baselines need a reup.)
One doesn't appear to exist today.
…rences, add "correct" example.
| +//// [DtsFileErrors] | ||
| + | ||
| + | ||
| +out/file.d.ts(4,23): error TS2314: Generic type 'Array<T>' requires 1 type argument(s). |
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.
Hm, this is exciting; wonder how easy it'd be to fix this?
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.
@weswigham can correct me, but I think this needs symbolTableToDeclarationStatements and others before we expect JS .d.ts to really work - see existingTypeNodeIsNotReferenceOrIsReferenceWithCompatibleTypeArgumentCount, canReuseTypeNode etc.
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.
Actually, not so - since we changed how JS code is parsed, we may never implement the symbolTableToDeclarationStatements stub and might just remove it (which is maybe a problem for expandable hover, since that also leverages it). Since JS is syntactically transformed into something the checker likes now, similarly, the declaration emitter aughta just have those special syntaxes handled directly in declaration emit rather than always falling back to a semantic printback - which basically just boils down to handling the new reparsed node structures and kinds in the transform itself. I added a draft of whatever js support was in while I was working on the initial declaration emitter, but it really does just need someone to go in and add the entirely new emit logic.
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.
In this case, the declaration emitter is picking up that we now just shove the type nodes directly on the .Type of the parameter and mostly just doing the right thing, but the declaration emitter is missing the logic to ensure minimum type argument counts are met that the node builder used to handle implicitly. That's a slightly semantic question ("how many type arguments should this reference have, at a minimum?"), so it'll probably need to get answered through a new emitResolver hook.
This restores the behavior of filling in missing type arguments (and default type arguments set to
{}andunknown) asanyin JavaScript files.It should fix the behavior in #1090, but and unbreaks JS code I've tested elsewhere where things like
new Setresult in aSet<unknown>instead of aSet<any>.Some of this is already partially controlled by flags like
noImplicitAny- however, I do wonder if we should consider adding another--strictflag for this behavior in JavaScript files. My intuition is that JS devs using checking and--strictdo want the same behavior as TypeScript, and would opt in for tighter checking. The use of JS is circumstantial around their build needs/preferences, not a signal of whether they want looser checking.Fixes #1090