Skip to content
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

Consider enclosing declaration when serializing inferred return types #59170

Merged
merged 3 commits into from
Jul 9, 2024

Conversation

jakebailey
Copy link
Member

@jakebailey jakebailey commented Jul 8, 2024

In playing around with the repros from #58937 and Effect-TS/effect#3171, I tried to go through the changes in #58546 to narrow things down to what might have caused the problem with inferred/unannotated return types.

One thread to pull on is that removing the context.mapper stuff seemed to fix the problem, but cause other undesirable changes.

Another area of note was serializeReturnTypeForSignatureWorker; he doc for tryReuseExistingTypeNodeHelper says:

/**
 * Do you mean to call this directly? You probably should use `tryReuseExistingTypeNode` instead,
 * which performs sanity checking on the type before doing this.
 */

serializeReturnTypeForSignatureWorker suspiciously calls tryReuseExistingTypeNodeHelper directly. I just swapped tryReuseExistingTypeNode in with parameters that seemed to make sense, and the repro cases appeared to snap back to what they used to be. For example, the declaration emit in Effect-TS/effect#3171 original post looked like this in 5.4:

export declare const traverse: {
    <A, R, O, E, B>(f: (a: A) => import("effect/Option").Option<B>): <TR, TO, TE>(self: import("effect/Record").ReadonlyRecord<string, A>) => import("effect/Option").Option<import("effect/Record").ReadonlyRecord<string, B>>;
    <TR_1, TO_1, TE_1, A_1, R_1, O_1, E_1, B_1>(self: import("effect/Record").ReadonlyRecord<string, A_1>, f: (a: A_1) => import("effect/Option").Option<B_1>): import("effect/Option").Option<import("effect/Record").ReadonlyRecord<string, B_1>>;
};

But in 5.5 is:

export declare const traverse: {
    <A, R, O, E, B>(f: (a: A) => import("effect/HKT").Kind<import("effect/Option").OptionTypeLambda, R, O, E, B>): <TR, TO, TE>(self: import("effect/HKT").Kind<import("effect/Record").ReadonlyRecordTypeLambda<string>, TR, TO, TE, A>) => import("effect/HKT").Kind<import("effect/Option").OptionTypeLambda, R, O, E, import("effect/HKT").Kind<import("effect/Record").ReadonlyRecordTypeLambda<string>, TR, TO, TE, B>>;
    <TR, TO, TE, A, R, O, E, B>(self: import("effect/Record").ReadonlyRecord<string, A>, f: (a: A) => import("effect/HKT").Kind<import("effect/Option").OptionTypeLambda, R, O, E, B>): import("effect/HKT").Kind<import("effect/Option").OptionTypeLambda, R, O, E, import("effect/HKT").Kind<import("effect/Record").ReadonlyRecordTypeLambda<string>, TR, TO, TE, B>>;
};

Note that we copies Kind out of the call signature of traverse. With this PR, we go to:

export declare const traverse: {
    <A, R, O, E, B>(f: (a: A) => import("effect/Option").Option<B>): <TR, TO, TE>(self: import("effect/Record").ReadonlyRecord<string, A>) => import("effect/Option").Option<import("effect/Record").ReadonlyRecord<string, B>>;
    <TR, TO, TE, A, R, O, E, B>(self: import("effect/Record").ReadonlyRecord<string, A>, f: (a: A) => import("effect/Option").Option<B>): import("effect/Option").Option<import("effect/Record").ReadonlyRecord<string, B>>;
};

Which is the best of both, in that Kind has gone away, and we still kept the better type parameter renaming behavior.

A number of other baselines "regressed" in that they seem to have less node reuse, but some appear to be more reasonable in some ways. Notably, no dts emit in our tests changed, which is good, but indicates that we need more testing here.

This is extremely unscientific. If this PR looks wrong, it should at least provide someone else with some info that could track the issue down better. I'm not going to say that this is the fix to #58937 without a bit more consensus.

Fixes #58937

cc @weswigham @dragomirtitian

@typescript-bot typescript-bot added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jul 8, 2024
@jakebailey
Copy link
Member Author

@typescript-bot test it
@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 8, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
test top400 ✅ Started ✅ Results
user test this ✅ Started ✅ Results
run dt ✅ Started ✅ Results
perf test this faster ✅ Started 👀 Results
pack this ✅ Started ✅ Results
@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 8, 2024

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/162588/artifacts?artifactName=tgz&fileId=18C37DD5E439FF86EF8CC21F21AE94E5A03D9A893A11631A44B691130EF45D4602&fileName=/typescript-5.6.0-insiders.20240708.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.6.0-pr-59170-2".;

@@ -79,7 +79,7 @@ class C extends B {
>A : typeof A
> : ^^^^^^^^
>then : <S extends A>(cb: (x: A) => S) => Chain<S>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In examples like these, I think it's incredibly weird that the old behavior had us not reusing effectively anything but the return type annotation.

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

Comment on lines 58 to +59
>fun : () => import("bbb").INode<T>
> : ^^^^^^ ^^^^^ ^^^^^ ^
> : ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old reuse here seems dubious to me; create is imported, so how are we reusing nodes from another file? Similarly to a lot of what changes in this PR.

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user tests with tsc comparing main and refs/pull/59170/merge:

Everything looks good!

@jakebailey jakebailey changed the title Ensure we consider enclosing declaration when attempting to reuse nodes in return types Jul 8, 2024
@jakebailey
Copy link
Member Author

@typescript-bot test it
@typescript-bot pack this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 8, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
test top400 ✅ Started ✅ Results
user test this ✅ Started ✅ Results
run dt ✅ Started ✅ Results
perf test this faster ✅ Started 👀 Results
pack this ✅ Started ✅ Results
@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 8, 2024

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/162594/artifacts?artifactName=tgz&fileId=6B705B58AF6F6B24FC52A47A0AE0718118A689B306FE5B1C0CC81F06C358639402&fileName=/typescript-5.6.0-insiders.20240708.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.6.0-pr-59170-7".;

@typescript-bot
Copy link
Collaborator

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

Here they are:

tsc

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-Unions - node (v18.15.0, x64)
Errors 30 30 ~ ~ ~ p=1.000 n=6
Symbols 62,153 62,153 ~ ~ ~ p=1.000 n=6
Types 50,242 50,242 ~ ~ ~ p=1.000 n=6
Memory used 193,361k (± 0.95%) 192,848k (± 0.76%) ~ 192,149k 195,844k p=0.873 n=6
Parse Time 1.31s (± 0.31%) 1.30s (± 1.49%) ~ 1.26s 1.31s p=0.248 n=6
Bind Time 0.71s 0.71s ~ ~ ~ p=1.000 n=6
Check Time 9.44s (± 0.24%) 9.44s (± 0.20%) ~ 9.42s 9.47s p=0.622 n=6
Emit Time 2.76s (± 0.60%) 2.74s (± 0.23%) ~ 2.73s 2.75s p=0.070 n=6
Total Time 14.21s (± 0.27%) 14.19s (± 0.22%) ~ 14.14s 14.23s p=0.687 n=6
angular-1 - node (v18.15.0, x64)
Errors 5 5 ~ ~ ~ p=1.000 n=6
Symbols 944,114 944,114 ~ ~ ~ p=1.000 n=6
Types 407,050 407,050 ~ ~ ~ p=1.000 n=6
Memory used 1,218,369k (± 0.00%) 1,218,396k (± 0.00%) ~ 1,218,328k 1,218,446k p=0.471 n=6
Parse Time 6.64s (± 0.48%) 6.61s (± 0.35%) ~ 6.58s 6.64s p=0.120 n=6
Bind Time 1.86s (± 0.53%) 1.86s (± 0.65%) ~ 1.85s 1.88s p=0.865 n=6
Check Time 30.59s (± 0.41%) 30.67s (± 0.41%) ~ 30.51s 30.78s p=0.228 n=6
Emit Time 13.53s (± 0.90%) 13.57s (± 0.24%) ~ 13.52s 13.60s p=0.872 n=6
Total Time 52.63s (± 0.29%) 52.71s (± 0.23%) ~ 52.56s 52.85s p=0.470 n=6
mui-docs - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,130,140 2,130,140 ~ ~ ~ p=1.000 n=6
Types 927,634 927,634 ~ ~ ~ p=1.000 n=6
Memory used 2,106,185k (± 0.01%) 2,106,181k (± 0.00%) ~ 2,106,029k 2,106,307k p=1.000 n=6
Parse Time 6.59s (± 0.36%) 6.60s (± 0.20%) ~ 6.59s 6.62s p=0.460 n=6
Bind Time 2.33s (± 1.07%) 2.31s (± 1.15%) ~ 2.27s 2.35s p=0.462 n=6
Check Time 70.40s (± 1.40%) 70.84s (± 0.30%) ~ 70.66s 71.23s p=0.689 n=6
Emit Time 0.28s (±128.30%) 0.13s (± 3.10%) ~ 0.13s 0.14s p=0.248 n=6
Total Time 79.60s (± 1.18%) 79.89s (± 0.25%) ~ 79.73s 80.27s p=0.748 n=6
self-build-src - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,225,497 1,225,508 +11 (+ 0.00%) ~ ~ p=0.001 n=6
Types 261,459 261,459 ~ ~ ~ p=1.000 n=6
Memory used 2,340,809k (± 0.03%) 2,340,478k (± 0.02%) ~ 2,339,908k 2,341,142k p=0.471 n=6
Parse Time 5.01s (± 1.93%) 5.08s (± 1.14%) ~ 5.03s 5.18s p=0.297 n=6
Bind Time 1.92s (± 0.61%) 1.91s (± 0.27%) ~ 1.91s 1.92s p=0.191 n=6
Check Time 34.29s (± 0.65%) 34.18s (± 0.44%) ~ 34.04s 34.47s p=0.378 n=6
Emit Time 2.61s (± 2.84%) 2.67s (± 3.88%) ~ 2.59s 2.87s p=0.199 n=6
Total Time 43.86s (± 0.69%) 43.86s (± 0.49%) ~ 43.66s 44.25s p=1.000 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,225,497 1,225,508 +11 (+ 0.00%) ~ ~ p=0.001 n=6
Types 261,459 261,459 ~ ~ ~ p=1.000 n=6
Memory used 2,415,521k (± 0.02%) 2,414,908k (± 0.05%) ~ 2,413,387k 2,416,195k p=0.471 n=6
Parse Time 6.24s (± 0.67%) 6.27s (± 0.56%) ~ 6.24s 6.33s p=0.332 n=6
Bind Time 2.03s (± 1.05%) 2.02s (± 0.41%) ~ 2.01s 2.03s p=0.806 n=6
Check Time 40.76s (± 0.12%) 40.82s (± 0.58%) ~ 40.61s 41.27s p=0.936 n=6
Emit Time 3.10s (± 1.21%) 3.17s (± 3.31%) ~ 3.06s 3.33s p=0.378 n=6
Total Time 52.14s (± 0.23%) 52.30s (± 0.63%) ~ 51.98s 52.83s p=0.572 n=6
self-compiler - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 258,195 258,195 ~ ~ ~ p=1.000 n=6
Types 104,737 104,737 ~ ~ ~ p=1.000 n=6
Memory used 427,482k (± 0.01%) 427,445k (± 0.03%) ~ 427,367k 427,658k p=0.173 n=6
Parse Time 4.07s (± 0.36%) 4.08s (± 0.26%) ~ 4.07s 4.10s p=0.073 n=6
Bind Time 1.61s (± 1.32%) 1.62s (± 1.68%) ~ 1.57s 1.65s p=0.681 n=6
Check Time 21.99s (± 0.31%) 21.99s (± 0.30%) ~ 21.89s 22.06s p=0.686 n=6
Emit Time 1.55s (± 0.97%) 1.57s (± 1.18%) +0.03s (+ 1.72%) 1.55s 1.59s p=0.037 n=6
Total Time 29.21s (± 0.26%) 29.27s (± 0.22%) ~ 29.17s 29.35s p=0.377 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors 35 35 ~ ~ ~ p=1.000 n=6
Symbols 224,565 224,565 ~ ~ ~ p=1.000 n=6
Types 93,734 93,734 ~ ~ ~ p=1.000 n=6
Memory used 369,486k (± 0.03%) 369,609k (± 0.05%) ~ 369,432k 369,818k p=0.298 n=6
Parse Time 3.45s (± 1.08%) 3.45s (± 0.80%) ~ 3.41s 3.48s p=0.808 n=6
Bind Time 1.93s (± 1.20%) 1.93s (± 0.39%) ~ 1.92s 1.94s p=0.870 n=6
Check Time 19.14s (± 0.41%) 19.13s (± 0.53%) ~ 18.98s 19.23s p=0.873 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 24.52s (± 0.49%) 24.51s (± 0.48%) ~ 24.36s 24.64s p=0.688 n=6
vscode - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,882,609 2,882,609 ~ ~ ~ p=1.000 n=6
Types 975,856 975,856 ~ ~ ~ p=1.000 n=6
Memory used 3,045,852k (± 0.00%) 3,045,892k (± 0.00%) ~ 3,045,818k 3,046,048k p=0.471 n=6
Parse Time 16.85s (± 0.21%) 16.88s (± 0.27%) ~ 16.84s 16.96s p=0.413 n=6
Bind Time 5.04s (± 0.39%) 5.09s (± 1.79%) ~ 5.01s 5.27s p=0.168 n=6
Check Time 89.49s (± 2.05%) 89.57s (± 1.10%) ~ 88.84s 91.51s p=0.298 n=6
Emit Time 28.25s (± 6.43%) 28.61s (± 3.51%) ~ 26.57s 29.15s p=0.575 n=6
Total Time 139.64s (± 0.41%) 140.15s (± 0.26%) ~ 139.66s 140.57s p=0.173 n=6
webpack - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 267,117 267,117 ~ ~ ~ p=1.000 n=6
Types 108,775 108,775 ~ ~ ~ p=1.000 n=6
Memory used 411,626k (± 0.03%) 411,592k (± 0.02%) ~ 411,499k 411,756k p=0.575 n=6
Parse Time 4.71s (± 0.25%) 4.72s (± 0.80%) ~ 4.67s 4.76s p=0.935 n=6
Bind Time 2.09s (± 0.58%) 2.09s (± 1.08%) ~ 2.05s 2.11s p=0.806 n=6
Check Time 20.78s (± 0.24%) 20.79s (± 0.57%) ~ 20.64s 20.96s p=0.810 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 27.58s (± 0.15%) 27.60s (± 0.49%) ~ 27.41s 27.77s p=0.872 n=6
xstate-main - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 525,251 525,251 ~ ~ ~ p=1.000 n=6
Types 178,574 178,574 ~ ~ ~ p=1.000 n=6
Memory used 462,587k (± 0.08%) 462,604k (± 0.08%) ~ 462,315k 463,145k p=0.575 n=6
Parse Time 3.17s (± 0.73%) 3.18s (± 0.62%) ~ 3.15s 3.20s p=0.746 n=6
Bind Time 1.17s 1.17s ~ ~ ~ p=1.000 n=6
Check Time 17.93s (± 0.40%) 17.93s (± 0.34%) ~ 17.86s 18.01s p=0.872 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 22.27s (± 0.24%) 22.28s (± 0.26%) ~ 22.20s 22.37s p=0.809 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Compiler-Unions - node (v18.15.0, x64)
  • angular-1 - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-build-src-public-api - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • ts-pre-modules - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate-main - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

Hey @jakebailey, the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the user tests with tsc comparing main and refs/pull/59170/merge:

Everything looks good!

@typescript-bot
Copy link
Collaborator

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

Here they are:

tsc

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-Unions - node (v18.15.0, x64)
Errors 30 30 ~ ~ ~ p=1.000 n=6
Symbols 62,153 62,153 ~ ~ ~ p=1.000 n=6
Types 50,242 50,242 ~ ~ ~ p=1.000 n=6
Memory used 194,584k (± 0.96%) 192,214k (± 0.07%) -2,370k (- 1.22%) 192,081k 192,479k p=0.045 n=6
Parse Time 1.30s (± 1.19%) 1.30s (± 0.75%) ~ 1.29s 1.31s p=0.933 n=6
Bind Time 0.71s 0.71s ~ ~ ~ p=1.000 n=6
Check Time 9.45s (± 0.22%) 9.46s (± 0.55%) ~ 9.38s 9.53s p=0.514 n=6
Emit Time 2.75s (± 0.91%) 2.74s (± 0.88%) ~ 2.71s 2.78s p=1.000 n=6
Total Time 14.21s (± 0.14%) 14.22s (± 0.35%) ~ 14.18s 14.31s p=0.454 n=6
angular-1 - node (v18.15.0, x64)
Errors 5 5 ~ ~ ~ p=1.000 n=6
Symbols 944,114 944,114 ~ ~ ~ p=1.000 n=6
Types 407,050 407,050 ~ ~ ~ p=1.000 n=6
Memory used 1,218,391k (± 0.00%) 1,218,360k (± 0.00%) ~ 1,218,284k 1,218,427k p=0.378 n=6
Parse Time 6.66s (± 0.86%) 6.67s (± 0.74%) ~ 6.62s 6.75s p=0.688 n=6
Bind Time 1.86s (± 0.68%) 1.86s (± 0.63%) ~ 1.84s 1.87s p=1.000 n=6
Check Time 30.68s (± 0.36%) 30.52s (± 0.23%) -0.17s (- 0.55%) 30.44s 30.62s p=0.030 n=6
Emit Time 13.54s (± 0.72%) 13.58s (± 0.47%) ~ 13.49s 13.68s p=0.571 n=6
Total Time 52.74s (± 0.40%) 52.63s (± 0.25%) ~ 52.47s 52.78s p=0.336 n=6
mui-docs - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,130,140 2,130,140 ~ ~ ~ p=1.000 n=6
Types 927,634 927,634 ~ ~ ~ p=1.000 n=6
Memory used 2,106,213k (± 0.01%) 2,106,137k (± 0.00%) ~ 2,106,095k 2,106,197k p=0.230 n=6
Parse Time 6.59s (± 0.37%) 6.62s (± 0.17%) ~ 6.61s 6.64s p=0.052 n=6
Bind Time 2.31s (± 0.75%) 2.31s (± 0.51%) ~ 2.30s 2.33s p=0.803 n=6
Check Time 70.61s (± 0.24%) 70.51s (± 0.29%) ~ 70.18s 70.77s p=0.471 n=6
Emit Time 0.13s (± 3.10%) 0.14s (± 2.95%) 🔻+0.01s (+ 5.06%) 0.13s 0.14s p=0.034 n=6
Total Time 79.65s (± 0.23%) 79.58s (± 0.26%) ~ 79.24s 79.83s p=0.689 n=6
self-build-src - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,225,497 1,225,508 +11 (+ 0.00%) ~ ~ p=0.001 n=6
Types 261,459 261,459 ~ ~ ~ p=1.000 n=6
Memory used 2,342,148k (± 0.04%) 2,341,379k (± 0.03%) ~ 2,340,112k 2,342,216k p=0.128 n=6
Parse Time 6.04s (± 0.62%) 6.04s (± 0.69%) ~ 6.00s 6.11s p=0.688 n=6
Bind Time 2.26s (± 0.63%) 2.26s (± 0.60%) ~ 2.24s 2.27s p=0.807 n=6
Check Time 40.21s (± 0.35%) 40.18s (± 0.30%) ~ 40.05s 40.36s p=0.575 n=6
Emit Time 3.02s (± 4.31%) 3.16s (± 5.81%) ~ 2.88s 3.40s p=0.230 n=6
Total Time 51.55s (± 0.46%) 51.65s (± 0.58%) ~ 51.18s 52.01s p=0.575 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 1,225,497 1,225,508 +11 (+ 0.00%) ~ ~ p=0.001 n=6
Types 261,459 261,459 ~ ~ ~ p=1.000 n=6
Memory used 2,414,873k (± 0.02%) 2,414,090k (± 0.03%) ~ 2,412,971k 2,414,813k p=0.066 n=6
Parse Time 5.20s (± 0.49%) 5.23s (± 1.12%) ~ 5.15s 5.30s p=0.470 n=6
Bind Time 1.70s (± 0.94%) 1.69s (± 1.02%) ~ 1.67s 1.72s p=0.514 n=6
Check Time 34.72s (± 0.52%) 34.73s (± 0.71%) ~ 34.55s 35.21s p=0.810 n=6
Emit Time 2.69s (± 2.66%) 2.70s (± 1.85%) ~ 2.62s 2.77s p=0.810 n=6
Total Time 44.30s (± 0.55%) 44.34s (± 0.74%) ~ 44.10s 44.97s p=0.810 n=6
self-compiler - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 258,195 258,195 ~ ~ ~ p=1.000 n=6
Types 104,737 104,737 ~ ~ ~ p=1.000 n=6
Memory used 427,454k (± 0.01%) 427,427k (± 0.01%) ~ 427,388k 427,458k p=0.229 n=6
Parse Time 4.08s (± 0.10%) 4.09s (± 0.72%) ~ 4.06s 4.13s p=0.451 n=6
Bind Time 1.63s (± 0.78%) 1.62s (± 1.21%) ~ 1.59s 1.65s p=0.567 n=6
Check Time 21.97s (± 0.39%) 22.05s (± 0.33%) ~ 21.92s 22.13s p=0.229 n=6
Emit Time 1.55s (± 1.94%) 1.59s (± 0.73%) +0.04s (+ 2.47%) 1.57s 1.60s p=0.022 n=6
Total Time 29.23s (± 0.30%) 29.35s (± 0.31%) ~ 29.22s 29.45s p=0.066 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors 35 35 ~ ~ ~ p=1.000 n=6
Symbols 224,565 224,565 ~ ~ ~ p=1.000 n=6
Types 93,734 93,734 ~ ~ ~ p=1.000 n=6
Memory used 369,505k (± 0.02%) 369,490k (± 0.04%) ~ 369,359k 369,777k p=0.378 n=6
Parse Time 2.78s (± 0.60%) 2.78s (± 1.06%) ~ 2.73s 2.80s p=0.743 n=6
Bind Time 1.60s (± 1.81%) 1.59s (± 1.17%) ~ 1.57s 1.61s p=0.459 n=6
Check Time 15.52s (± 0.39%) 15.51s (± 0.28%) ~ 15.46s 15.58s p=0.628 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 19.89s (± 0.35%) 19.87s (± 0.28%) ~ 19.83s 19.98s p=0.572 n=6
vscode - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 2,882,609 2,882,609 ~ ~ ~ p=1.000 n=6
Types 975,856 975,856 ~ ~ ~ p=1.000 n=6
Memory used 3,045,880k (± 0.00%) 3,045,928k (± 0.00%) ~ 3,045,818k 3,046,009k p=0.199 n=6
Parse Time 16.89s (± 0.32%) 16.92s (± 0.46%) ~ 16.84s 17.02s p=0.575 n=6
Bind Time 5.13s (± 2.08%) 5.13s (± 1.90%) ~ 5.08s 5.33s p=0.807 n=6
Check Time 90.81s (± 2.61%) 90.95s (± 2.40%) ~ 89.30s 94.65s p=0.575 n=6
Emit Time 27.70s (± 8.64%) 27.80s (± 7.23%) ~ 24.67s 29.25s p=0.936 n=6
Total Time 140.53s (± 0.10%) 140.80s (± 0.23%) ~ 140.50s 141.29s p=0.128 n=6
webpack - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 267,117 267,117 ~ ~ ~ p=1.000 n=6
Types 108,775 108,775 ~ ~ ~ p=1.000 n=6
Memory used 411,590k (± 0.01%) 411,584k (± 0.01%) ~ 411,553k 411,631k p=0.810 n=6
Parse Time 3.81s (± 0.61%) 3.82s (± 0.81%) ~ 3.78s 3.87s p=1.000 n=6
Bind Time 1.68s (± 0.32%) 1.68s (± 0.72%) ~ 1.66s 1.69s p=0.855 n=6
Check Time 16.71s (± 0.38%) 16.78s (± 0.46%) ~ 16.66s 16.86s p=0.170 n=6
Emit Time 0.00s 0.00s ~ ~ ~ p=1.000 n=6
Total Time 22.21s (± 0.32%) 22.28s (± 0.29%) ~ 22.16s 22.34s p=0.149 n=6
xstate-main - node (v18.15.0, x64)
Errors 0 0 ~ ~ ~ p=1.000 n=6
Symbols 525,251 525,251 ~ ~ ~ p=1.000 n=6
Types 178,574 178,574 ~ ~ ~ p=1.000 n=6
Memory used 462,917k (± 0.05%) 462,609k (± 0.06%) ~ 462,410k 463,035k p=0.066 n=6
Parse Time 3.95s (± 0.59%) 3.95s (± 0.34%) ~ 3.93s 3.96s p=0.676 n=6
Bind Time 1.46s (± 1.42%) 1.46s (± 0.67%) ~ 1.45s 1.47s p=0.565 n=6
Check Time 22.25s (± 0.43%) 22.19s (± 0.42%) ~ 22.03s 22.31s p=0.747 n=6
Emit Time 0.00s 0.00s (±244.70%) ~ 0.00s 0.01s p=0.405 n=6
Total Time 27.65s (± 0.39%) 27.61s (± 0.35%) ~ 27.44s 27.73s p=1.000 n=6
System info unknown
Hosts
  • node (v18.15.0, x64)
Scenarios
  • Compiler-Unions - node (v18.15.0, x64)
  • angular-1 - node (v18.15.0, x64)
  • mui-docs - node (v18.15.0, x64)
  • self-build-src - node (v18.15.0, x64)
  • self-build-src-public-api - node (v18.15.0, x64)
  • self-compiler - node (v18.15.0, x64)
  • ts-pre-modules - node (v18.15.0, x64)
  • vscode - node (v18.15.0, x64)
  • webpack - node (v18.15.0, x64)
  • xstate-main - node (v18.15.0, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top 400 repos with tsc comparing main and refs/pull/59170/merge:

Everything looks good!

1 similar comment
@typescript-bot
Copy link
Collaborator

@jakebailey Here are the results of running the top 400 repos with tsc comparing main and refs/pull/59170/merge:

Everything looks good!

// To ensure we don't serialize the wrong type we check that that return type of the signature corresponds to the declaration return type signature
if (annotation && getTypeFromTypeNode(context, annotation) === type) {
const result = tryReuseExistingTypeNodeHelper(context, annotation);
const annotation = getNonlocalEffectiveReturnTypeAnnotationNode(signature.declaration);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure removing this check getTypeFromTypeNode(context, annotation) === type is a good idea? I think there are cases where the type of the annotation might differ from the type of the signature.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what tryReuseExistingTypeNode does internally and is how it differs from tryReuseExistingTypeNodeHelper

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right; removing this didn't change anything except reducing some type instantiations. Sorry, I rebased that particular thing away because I felt pretty confident that I could remove the equality here.

@jakebailey
Copy link
Member Author

Interestingly, this is not the fix for the type display issue from DefinitelyTyped/DefinitelyTyped#69838.

@typescript-bot typescript-bot added For Milestone Bug PRs that fix a bug with a specific milestone and removed For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jul 8, 2024
@gcanti
Copy link

gcanti commented Jul 9, 2024

I can confirm that installing the npm package referenced here #59170 (comment) fixes the type display issue reported in Effect-TS/effect#3171

@jakebailey
Copy link
Member Author

Thanks for confirming; I'm going to make one last attempt to make a repro that exhibits the "cannot be named" error before merging/backporting.

@jakebailey
Copy link
Member Author

Okay, I give up trying to produce a test case for the cannot be named error.

@jakebailey
Copy link
Member Author

@typescript-bot cherry-pick this to release-5.5

@typescript-bot
Copy link
Collaborator

typescript-bot commented Jul 9, 2024

Starting jobs; this comment will be updated as builds start and complete.

Command Status Results
cherry-pick this to release-5.5 ✅ Started ❌ Results
@typescript-bot
Copy link
Collaborator

Hey, @jakebailey! I was unable to cherry-pick this PR.

Check the logs at: https://github.com/microsoft/TypeScript/actions/runs/9863981289

@jakebailey
Copy link
Member Author

Okay, guess I'll do it manually

@jakebailey jakebailey merged commit 533acb5 into microsoft:main Jul 9, 2024
29 checks passed
@jakebailey jakebailey deleted the fix-58937 branch July 9, 2024 21:35
jakebailey added a commit to jakebailey/TypeScript that referenced this pull request Jul 9, 2024
@DanielRosenwasser DanielRosenwasser added this to the TypeScript 5.5.4 milestone Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
6 participants