Skip to content

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

Conversation

DanielRosenwasser
Copy link
Member

@DanielRosenwasser DanielRosenwasser commented Jun 19, 2025

This restores the behavior of filling in missing type arguments (and default type arguments set to {} and unknown) as any in JavaScript files.

It should fix the behavior in #1090, but and unbreaks JS code I've tested elsewhere where things like new Set result in a Set<unknown> instead of a Set<any>.

Some of this is already partially controlled by flags like noImplicitAny - however, I do wonder if we should consider adding another --strict flag for this behavior in JavaScript files. My intuition is that JS devs using checking and --strict do 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

@Copilot Copilot AI review requested due to automatic review settings June 19, 2025 21:15
Copy link
Contributor

@Copilot Copilot AI left a 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 fillMissingTypeArguments with a JS file check to use any defaults in JS.
  • Updating numerous baseline tests to reflect any defaults in JS contexts.
  • Adjusting calls throughout internal/checker to pass the new isJavaScript flag.

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)

@DanielRosenwasser
Copy link
Member Author

DanielRosenwasser commented Jun 20, 2025

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)

Copy link
Member

@jakebailey jakebailey left a 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.)

+
+
+out/file.d.ts(4,23): error TS2314: Generic type 'Array<T>' requires 1 type argument(s).
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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.

@DanielRosenwasser DanielRosenwasser added this pull request to the merge queue Jun 24, 2025
Merged via the queue into microsoft:main with commit e00c12f Jun 24, 2025
22 checks passed
@DanielRosenwasser DanielRosenwasser deleted the implicitAnyTypeArgumentsInJavaScript branch June 24, 2025 03:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using a class based react component imported from a jsx files produce errors
3 participants