Return early on intrinsics in couldContainTypeVariables #54377

jakebailey · 2023-05-24T20:40:01Z

The problem in the issue is that both source and target are the wildcard type, and so this recurses forever.

But, one thing that fixes this is to just bail early on intrinsics, as those can't actually contain type vars (which then does prevent the inference loop).

I could send a more targeted fix but I think this might actually help perf.

Needs a test.

jakebailey · 2023-05-24T20:40:15Z

@typescript-bot test this
@typescript-bot test top100
@typescript-bot user test this
@typescript-bot run dt
@typescript-bot perf test this faster
@typescript-bot pack this

typescript-bot · 2023-05-24T20:40:18Z

Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at b9aec28. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-05-24T20:40:18Z

Heya @jakebailey, I've started to run the extended test suite on this PR at b9aec28. You can monitor the build here.

typescript-bot · 2023-05-24T20:40:18Z

Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at b9aec28. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-05-24T20:40:18Z

Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at b9aec28. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-05-24T20:40:18Z

Heya @jakebailey, I've started to run the abridged perf test suite on this PR at b9aec28. You can monitor the build here.

Update: The results are in!

typescript-bot · 2023-05-24T20:40:19Z

Heya @jakebailey, I've started to run the tarball bundle task on this PR at b9aec28. You can monitor the build here.

typescript-bot · 2023-05-24T20:41:52Z

Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your package.json like so:

{
    "devDependencies": {
        "typescript": "https://typescript.visualstudio.com/cf7ac146-d525-443c-b23c-0d58337efebc/_apis/build/builds/154768/artifacts?artifactName=tgz&fileId=FDA1DBBE3E104BE4A8855C36123A62E2D06692464EB7D44A49F7841DBB25447602&fileName=/typescript-5.2.0-insiders.20230524.tgz"
    }
}

and then running npm install.

There is also a playground for this build and an npm module you can use via "typescript": "npm:@typescript-deploys/pr-build@5.2.0-pr-54377-7".;

typescript-bot · 2023-05-24T21:05:40Z

@jakebailey Here are the results of running the user test suite comparing main and refs/pull/54377/merge:

There were infrastructure failures potentially unrelated to your change:

1 instance of "Unknown failure"
1 instance of "Package install failed"

Otherwise...

Everything looks good!

typescript-bot · 2023-05-24T21:12:45Z

@jakebailey
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..54377

_Metric	_main	₅₄₃₇₇	_Delta	_Best	_Worst	_p-value
_{Angular - node (v16.17.1, x64)}
_{Memory used}	_{365,254k (± 0.02%)}	_{365,178k (± 0.02%)}	_~	_365,096k	_365,255k	_{p=0.066 n=6}
_{Parse Time}	_{3.58s (± 0.38%)}	_{3.56s (± 0.48%)}	_{-0.02s (- 0.65%)}	_3.53s	_3.58s	_{p=0.021 n=6}
_{Bind Time}	_{1.18s (± 0.46%)}	_{1.18s (± 0.35%)}	_~	_1.17s	_1.18s	_{p=0.054 n=6}
_{Check Time}	_{9.62s (± 0.45%)}	_{9.57s (± 0.30%)}	_~	_9.54s	_9.62s	_{p=0.065 n=6}
_{Emit Time}	_{7.99s (± 1.35%)}	_{7.94s (± 0.77%)}	_~	_7.87s	_8.02s	_{p=0.469 n=6}
_{Total Time}	_{22.38s (± 0.60%)}	_{22.24s (± 0.30%)}	_~	_22.17s	_22.34s	_{p=0.065 n=6}
_{Compiler-Unions - node (v16.17.1, x64)}
_{Memory used}	_{193,481k (± 0.70%)}	_{193,468k (± 0.70%)}	_~	_192,799k	_196,242k	_{p=0.521 n=6}
_{Parse Time}	_{1.61s (± 0.52%)}	_{1.60s (± 1.96%)}	_~	_1.54s	_1.62s	_{p=0.196 n=6}
_{Bind Time}	_{0.83s (± 0.49%)}	_{0.83s (± 0.62%)}	_~	_0.82s	_0.83s	_{p=0.595 n=6}
_{Check Time}	_{10.17s (± 0.48%)}	_{10.19s (± 1.03%)}	_~	_10.07s	_10.35s	_{p=0.936 n=6}
_{Emit Time}	_{3.01s (± 0.45%)}	_{3.00s (± 1.02%)}	_~	_2.96s	_3.05s	_{p=0.416 n=6}
_{Total Time}	_{15.61s (± 0.32%)}	_{15.62s (± 0.68%)}	_~	_15.44s	_15.74s	_{p=0.688 n=6}
_{Monaco - node (v16.17.1, x64)}
_{Memory used}	_{345,880k (± 0.00%)}	_{345,882k (± 0.00%)}	_~	_345,865k	_345,900k	_{p=0.689 n=6}
_{Parse Time}	_{2.73s (± 0.68%)}	_{2.73s (± 0.44%)}	_~	_2.71s	_2.74s	_{p=0.806 n=6}
_{Bind Time}	_{1.09s (± 1.07%)}	_{1.09s (± 0.69%)}	_~	_1.08s	_1.10s	_{p=0.734 n=6}
_{Check Time}	_{7.83s (± 0.37%)}	_{7.83s (± 0.65%)}	_~	_7.76s	_7.89s	_{p=0.936 n=6}
_{Emit Time}	_{4.45s (± 0.39%)}	_{4.44s (± 0.93%)}	_~	_4.38s	_4.49s	_{p=0.808 n=6}
_{Total Time}	_{16.10s (± 0.08%)}	_{16.08s (± 0.57%)}	_~	_15.98s	_16.21s	_{p=0.627 n=6}
_{TFS - node (v16.17.1, x64)}
_{Memory used}	_{299,953k (± 0.00%)}	_{299,945k (± 0.01%)}	_~	_299,918k	_299,979k	_{p=0.572 n=6}
_{Parse Time}	_{2.17s (± 0.69%)}	_{2.15s (± 0.39%)}	_~	_2.15s	_2.17s	_{p=0.138 n=6}
_{Bind Time}	_{1.23s (± 0.51%)}	_{1.23s (± 0.84%)}	_~	_1.22s	_1.25s	_{p=0.654 n=6}
_{Check Time}	_{7.28s (± 0.34%)}	_{7.29s (± 0.51%)}	_~	_7.25s	_7.34s	_{p=0.418 n=6}
_{Emit Time}	_{4.36s (± 0.76%)}	_{4.33s (± 1.02%)}	_~	_4.27s	_4.39s	_{p=0.199 n=6}
_{Total Time}	_{15.04s (± 0.37%)}	_{15.01s (± 0.28%)}	_~	_14.96s	_15.08s	_{p=0.469 n=6}
_{material-ui - node (v16.17.1, x64)}
_{Memory used}	_{480,972k (± 0.01%)}	_{480,980k (± 0.00%)}	_~	_480,949k	_481,007k	_{p=0.575 n=6}
_{Parse Time}	_{3.26s (± 0.46%)}	_{3.26s (± 0.25%)}	_~	_3.25s	_3.27s	_{p=1.000 n=6}
_{Bind Time}	_{0.95s (± 0.86%)}	_{0.94s (± 0.55%)}	_~	_0.94s	_0.95s	_{p=0.523 n=6}
_{Check Time}	_{17.89s (± 0.97%)}	_{17.95s (± 1.58%)}	_~	_17.69s	_18.48s	_{p=0.936 n=6}
_{Emit Time}	_{0.00s (± 0.00%)}	_{0.00s (± 0.00%)}	_~	_0.00s	_0.00s	_{p=1.000 n=6}
_{Total Time}	_{22.09s (± 0.81%)}	_{22.15s (± 1.27%)}	_~	_21.89s	_22.67s	_{p=0.936 n=6}
_{xstate - node (v16.17.1, x64)}
_{Memory used}	_{560,641k (± 0.02%)}	_{560,700k (± 0.03%)}	_~	_560,521k	_560,971k	_{p=0.689 n=6}
_{Parse Time}	_{4.02s (± 0.53%)}	_{4.01s (± 0.34%)}	_~	_3.99s	_4.03s	_{p=0.369 n=6}
_{Bind Time}	_{1.77s (± 0.29%)}	_{1.76s (± 0.46%)}	_{-0.01s (- 0.56%)}	_1.75s	_1.77s	_{p=0.039 n=6}
_{Check Time}	_{3.06s (± 0.70%)}	_{3.07s (± 0.63%)}	_~	_3.05s	_3.10s	_{p=0.512 n=6}
_{Emit Time}	_{0.09s (± 0.00%)}	_{0.09s (± 0.00%)}	_~	_0.09s	_0.09s	_{p=1.000 n=6}
_{Total Time}	_{8.94s (± 0.40%)}	_{8.93s (± 0.28%)}	_~	_8.91s	_8.97s	_{p=0.870 n=6}

System

_{Machine Name}	_ts-ci-ubuntu
_Platform	_{linux 5.4.0-148-generic}
_Architecture	_x64
_{Available Memory}	_{16 GB}
_{Available Memory}	_{15 GB}
_CPUs	_{4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz}

Hosts

node (v16.17.1, x64)

Scenarios

Angular - node (v16.17.1, x64)
Compiler-Unions - node (v16.17.1, x64)
Monaco - node (v16.17.1, x64)
TFS - node (v16.17.1, x64)
material-ui - node (v16.17.1, x64)
xstate - node (v16.17.1, x64)

_Benchmark	_Name	_Iterations
_Current	₅₄₃₇₇	₆
_Baseline	_main	₆

Developer Information:

Download Benchmark

typescript-bot · 2023-05-24T22:15:43Z

@jakebailey Here are the results of running the top-repos suite comparing main and refs/pull/54377/merge:

Everything looks good!

typescript-bot · 2023-05-24T22:30:43Z

Hey @jakebailey, the results of running the DT tests are ready.
Everything looks the same!
You can check the log here.

ahejlsberg · 2023-05-25T16:03:47Z

src/compiler/checker.ts

@@ -24004,6 +24004,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
    // we perform type inference (i.e. a type parameter of a generic function). We cache
    // results for union and intersection types for performance reasons.
    function couldContainTypeVariables(type: Type): boolean {
+        if (type.flags & TypeFlags.Intrinsic) return false;


I'm not understanding how the function would previously return true for the wildcard type (which is a TypeFlags.Any)? What am I missing?

The place I was referring to was inferTypes, which starts with a wildcard check, but if both sides are a wildcard we loop forever.

But, it checks for "could contain type var" before the wildcard logic.

Right, but assuming both sides are wildcard types, how is it that we loop forever? That would require couldContainTypeVariables to return true for a TypeFlags.Any, and I'm not seeing how that happens.

You're totally right that couldContainTypeVariables should already be returning false for wildcardType, and yet, in my example it appeared to be returning true in the debugger!

But now, on main, my test doesn't fail... reverse bisecting says that it was "fixed" by changing lib.d.ts. So, I think there's some really odd corruption happening. Sounds like a fun debugging challenge.

Just to note the scary thing I'm seeing:

Traced this down to the new code in #53246, but it's absolutely bizarre. The flag is clearly there that says that "could contain type vars" is computed, and yet it still proceeds to overwrite it for no reason.

Figured it out, see #54348 (comment)

capaj · 2023-06-02T07:52:15Z

I have tested this on our codebase for @official-love-guru, where both 5.0.4 and 5.1.3 are crashing with

RangeError: Maximum call stack size exceeded
    at inferFromTypes (/home/capaj/work-repos/redacted/node_modules/typescript/lib/tsc.js:63010:28)
    at inferFromTypes (/home/capaj/work-repos/redacted/node_modules/typescript/lib/tsc.js:63017:9)
    at inferFromTypes (/home/capaj/work-repos/redacted/node_modules/typescript/lib/tsc.js:63017:9)
    at inferFromTypes (/home/capaj/work-repos/redacted/node_modules/typescript/lib/tsc.js:63017:9)
    at inferFromTypes (/home/capaj/work-repos/redacted/node_modules/typescript/lib/tsc.js:63017:9)
    at inferFromTypes (/home/capaj/work-repos/redacted/node_modules/typescript/lib/tsc.js:63017:9)
    at inferFromTypes (/home/capaj/work-repos/redacted/node_modules/typescript/lib/tsc.js:63017:9)
    at inferFromTypes (/home/capaj/work-repos/redacted/node_modules/typescript/lib/tsc.js:63017:9)
    at inferFromTypes (/home/capaj/work-repos/redacted/node_modules/typescript/lib/tsc.js:63017:9)
    at inferFromTypes (/home/capaj/work-repos/redacted/node_modules/typescript/lib/tsc.js:63017:9)

and this PR fixes it!

Please merge this.

jakebailey · 2023-06-02T17:09:47Z

The real fix for this problem is definitely not going to be this PR, per above conversation. I am looking into the actual problem.

Return early on intrinsics in couldContainTypeVariables

b9aec28

typescript-bot assigned jakebailey May 24, 2023

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels May 24, 2023

ahejlsberg reviewed May 25, 2023

View reviewed changes

jakebailey closed this Jun 2, 2023

jakebailey deleted the fix-54348 branch March 15, 2024 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Return early on intrinsics in couldContainTypeVariables #54377

Return early on intrinsics in couldContainTypeVariables #54377

jakebailey commented May 24, 2023

jakebailey commented May 24, 2023

typescript-bot commented May 24, 2023 •

edited

typescript-bot commented May 24, 2023 •

edited

typescript-bot commented May 24, 2023 •

edited

typescript-bot commented May 24, 2023 •

edited

typescript-bot commented May 24, 2023 •

edited

typescript-bot commented May 24, 2023 •

edited

typescript-bot commented May 24, 2023 •

edited

typescript-bot commented May 24, 2023

typescript-bot commented May 24, 2023

typescript-bot commented May 24, 2023

typescript-bot commented May 24, 2023

ahejlsberg May 25, 2023

jakebailey May 25, 2023

ahejlsberg May 25, 2023

jakebailey May 25, 2023

This comment was marked as spam.

jakebailey Jun 2, 2023

jakebailey Jun 2, 2023

jakebailey Jun 2, 2023

capaj commented Jun 2, 2023

jakebailey commented Jun 2, 2023

Return early on intrinsics in couldContainTypeVariables #54377

Return early on intrinsics in couldContainTypeVariables #54377

Conversation

jakebailey commented May 24, 2023

jakebailey commented May 24, 2023

typescript-bot commented May 24, 2023 • edited

typescript-bot commented May 24, 2023 • edited

typescript-bot commented May 24, 2023 • edited

typescript-bot commented May 24, 2023 • edited

typescript-bot commented May 24, 2023 • edited

typescript-bot commented May 24, 2023 • edited

typescript-bot commented May 24, 2023 • edited

typescript-bot commented May 24, 2023

typescript-bot commented May 24, 2023

typescript-bot commented May 24, 2023

typescript-bot commented May 24, 2023

ahejlsberg May 25, 2023

Choose a reason for hiding this comment

jakebailey May 25, 2023

Choose a reason for hiding this comment

ahejlsberg May 25, 2023

Choose a reason for hiding this comment

jakebailey May 25, 2023

Choose a reason for hiding this comment

This comment was marked as spam.

jakebailey Jun 2, 2023

Choose a reason for hiding this comment

jakebailey Jun 2, 2023

Choose a reason for hiding this comment

jakebailey Jun 2, 2023

Choose a reason for hiding this comment

capaj commented Jun 2, 2023

jakebailey commented Jun 2, 2023

typescript-bot commented May 24, 2023 •

edited

typescript-bot commented May 24, 2023 •

edited

typescript-bot commented May 24, 2023 •

edited

typescript-bot commented May 24, 2023 •

edited

typescript-bot commented May 24, 2023 •

edited

typescript-bot commented May 24, 2023 •

edited

typescript-bot commented May 24, 2023 •

edited