Improve reuse of nodes in signatures with type mapping

[Titian Cernicova-Dragomir (dragomirtitian)]

Improve reuse of nodes in signatures with type mapping

[TypeScript Bot (typescript-bot)]

This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.

[Jake Bailey (jakebailey)]

TypeScript Bot (@typescript-bot) test it

[TypeScript Bot (typescript-bot)]

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

[TypeScript Bot (typescript-bot)]

Hey Jake Bailey (@jakebailey), the results of running the DT tests are ready.

Everything looks the same!

You can check the log here.

[TypeScript Bot (typescript-bot)]

Jake Bailey (@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,154	62,154	~	~	~	p=1.000 n=6
Types	50,248	50,248	~	~	~	p=1.000 n=6
Memory used	192,881k (± 0.83%)	192,851k (± 0.78%)	~	192,208k	195,923k	p=0.936 n=6
Parse Time	1.55s (± 2.39%)	1.54s (± 1.81%)	~	1.50s	1.58s	p=0.686 n=6
Bind Time	0.87s (± 1.13%)	0.86s (± 0.60%)	~	0.86s	0.87s	p=0.417 n=6
Check Time	11.30s (± 0.38%)	11.33s (± 0.10%)	~	11.31s	11.34s	p=0.293 n=6
Emit Time	3.15s (± 1.29%)	3.16s (± 0.88%)	~	3.13s	3.21s	p=1.000 n=6
Total Time	16.87s (± 0.41%)	16.89s (± 0.23%)	~	16.83s	16.95s	p=0.520 n=6
angular-1 - node (v18.15.0, x64)
Errors	5	5	~	~	~	p=1.000 n=6
Symbols	944,110	944,110	~	~	~	p=1.000 n=6
Types	407,140	407,140	~	~	~	p=1.000 n=6
Memory used	1,222,108k (± 0.01%)	1,222,097k (± 0.01%)	~	1,221,969k	1,222,189k	p=1.000 n=6
Parse Time	8.09s (± 0.47%)	8.08s (± 0.68%)	~	8.02s	8.16s	p=0.748 n=6
Bind Time	2.22s (± 0.54%)	2.22s (± 0.70%)	~	2.20s	2.23s	p=0.797 n=6
Check Time	36.31s (± 0.17%)	36.31s (± 0.25%)	~	36.20s	36.40s	p=1.000 n=6
Emit Time	17.47s (± 0.91%)	17.38s (± 0.37%)	~	17.30s	17.49s	p=0.378 n=6
Total Time	64.09s (± 0.34%)	63.99s (± 0.16%)	~	63.83s	64.14s	p=0.470 n=6
mui-docs - node (v18.15.0, x64)
Errors	5	5	~	~	~	p=1.000 n=6
Symbols	1,964,176	1,964,176	~	~	~	p=1.000 n=6
Types	819,283	819,283	~	~	~	p=1.000 n=6
Memory used	1,849,670k (± 0.00%)	1,849,586k (± 0.00%)	-84k (- 0.00%)	1,849,564k	1,849,611k	p=0.005 n=6
Parse Time	6.79s (± 0.49%)	6.78s (± 0.30%)	~	6.75s	6.81s	p=0.325 n=6
Bind Time	2.31s (± 1.27%)	2.29s (± 1.22%)	~	2.28s	2.35s	p=0.155 n=6
Check Time	58.57s (± 0.32%)	58.54s (± 0.32%)	~	58.33s	58.80s	p=0.689 n=6
Emit Time	0.14s	0.14s	~	~	~	p=1.000 n=6
Total Time	67.80s (± 0.24%)	67.75s (± 0.32%)	~	67.53s	68.09s	p=0.688 n=6
self-build-src - node (v18.15.0, x64)
Errors	0	0	~	~	~	p=1.000 n=6
Symbols	1,221,221	1,221,227	+6 (+ 0.00%)	~	~	p=0.001 n=6
Types	259,523	259,525	+2 (+ 0.00%)	~	~	p=0.001 n=6
Memory used	2,337,728k (± 0.03%)	2,338,453k (± 0.03%)	~	2,337,374k	2,339,430k	p=0.173 n=6
Parse Time	4.98s (± 0.96%)	5.04s (± 0.67%)	+0.06s (+ 1.17%)	5.00s	5.10s	p=0.031 n=6
Bind Time	1.88s (± 0.45%)	1.89s (± 0.58%)	+0.01s (+ 0.80%)	1.87s	1.90s	p=0.042 n=6
Check Time	33.75s (± 0.32%)	33.83s (± 0.33%)	~	33.67s	34.00s	p=0.199 n=6
Emit Time	2.65s (± 1.61%)	2.62s (± 2.87%)	~	2.48s	2.69s	p=0.747 n=6
Total Time	43.26s (± 0.33%)	43.39s (± 0.41%)	~	43.11s	43.57s	p=0.229 n=6
self-build-src-public-api - node (v18.15.0, x64)
Errors	0	0	~	~	~	p=1.000 n=6
Symbols	1,221,221	1,221,227	+6 (+ 0.00%)	~	~	p=0.001 n=6
Types	259,523	259,525	+2 (+ 0.00%)	~	~	p=0.001 n=6
Memory used	2,413,607k (± 0.03%)	2,414,151k (± 0.04%)	~	2,413,029k	2,415,673k	p=0.471 n=6
Parse Time	6.25s (± 1.24%)	6.29s (± 1.46%)	~	6.11s	6.35s	p=0.378 n=6
Bind Time	2.04s (± 0.69%)	2.04s (± 1.25%)	~	2.01s	2.07s	p=0.809 n=6
Check Time	40.23s (± 0.38%)	40.38s (± 0.25%)	~	40.28s	40.54s	p=0.128 n=6
Emit Time	3.12s (± 2.90%)	3.18s (± 1.80%)	~	3.08s	3.24s	p=0.378 n=6
Total Time	51.65s (± 0.35%)	51.88s (± 0.38%)	+0.22s (+ 0.43%)	51.49s	52.03s	p=0.031 n=6
self-compiler - node (v18.15.0, x64)
Errors	0	0	~	~	~	p=1.000 n=6
Symbols	256,767	256,784	+17 (+ 0.01%)	~	~	p=0.001 n=6
Types	104,587	104,589	+2 (+ 0.00%)	~	~	p=0.001 n=6
Memory used	426,072k (± 0.01%)	426,218k (± 0.01%)	+146k (+ 0.03%)	426,142k	426,284k	p=0.005 n=6
Parse Time	2.79s (± 0.51%)	2.79s (± 0.42%)	~	2.78s	2.81s	p=0.934 n=6
Bind Time	1.11s (± 0.76%)	1.11s	~	~	~	p=0.176 n=6
Check Time	15.18s (± 0.49%)	15.19s (± 0.23%)	~	15.14s	15.22s	p=0.574 n=6
Emit Time	1.16s (± 1.22%)	1.13s (± 1.03%)	-0.03s (- 2.44%)	1.12s	1.15s	p=0.012 n=6
Total Time	20.23s (± 0.41%)	20.22s (± 0.20%)	~	20.17s	20.28s	p=1.000 n=6
ts-pre-modules - node (v18.15.0, x64)
Errors	35	35	~	~	~	p=1.000 n=6
Symbols	224,575	224,575	~	~	~	p=1.000 n=6
Types	93,785	93,785	~	~	~	p=1.000 n=6
Memory used	369,789k (± 0.01%)	369,907k (± 0.03%)	+118k (+ 0.03%)	369,776k	370,014k	p=0.045 n=6
Parse Time	3.50s (± 0.51%)	3.52s (± 0.39%)	~	3.50s	3.54s	p=0.139 n=6
Bind Time	1.93s (± 0.60%)	1.94s (± 1.51%)	~	1.92s	2.00s	p=0.615 n=6
Check Time	19.38s (± 0.35%)	19.35s (± 0.29%)	~	19.29s	19.44s	p=0.373 n=6
Emit Time	0.00s	0.00s	~	~	~	p=1.000 n=6
Total Time	24.81s (± 0.23%)	24.81s (± 0.30%)	~	24.73s	24.93s	p=0.872 n=6
vscode - node (v18.15.0, x64)
Errors	0	0	~	~	~	p=1.000 n=6
Symbols	2,823,941	2,823,941	~	~	~	p=1.000 n=6
Types	957,912	957,912	~	~	~	p=1.000 n=6
Memory used	2,996,220k (± 0.00%)	2,996,231k (± 0.00%)	~	2,996,158k	2,996,290k	p=0.689 n=6
Parse Time	13.81s (± 0.19%)	13.82s (± 0.31%)	~	13.75s	13.88s	p=0.459 n=6
Bind Time	4.15s (± 0.47%)	4.14s (± 0.36%)	~	4.13s	4.17s	p=0.742 n=6
Check Time	73.41s (± 0.23%)	73.41s (± 0.22%)	~	73.12s	73.61s	p=0.688 n=6
Emit Time	23.58s (± 1.01%)	23.46s (± 0.52%)	~	23.36s	23.62s	p=0.520 n=6
Total Time	114.95s (± 0.32%)	114.84s (± 0.18%)	~	114.48s	115.05s	p=0.936 n=6
webpack - node (v18.15.0, x64)
Errors	0	0	~	~	~	p=1.000 n=6
Symbols	265,866	265,866	~	~	~	p=1.000 n=6
Types	108,401	108,401	~	~	~	p=1.000 n=6
Memory used	410,600k (± 0.02%)	410,598k (± 0.02%)	~	410,508k	410,762k	p=0.936 n=6
Parse Time	4.75s (± 1.03%)	4.77s (± 1.12%)	~	4.68s	4.82s	p=0.422 n=6
Bind Time	2.05s (± 0.97%)	2.07s (± 0.71%)	~	2.04s	2.08s	p=0.155 n=6
Check Time	20.96s (± 0.39%)	20.93s (± 0.27%)	~	20.87s	21.01s	p=0.574 n=6
Emit Time	0.00s	0.00s	~	~	~	p=1.000 n=6
Total Time	27.76s (± 0.14%)	27.77s (± 0.25%)	~	27.69s	27.89s	p=1.000 n=6
xstate-main - node (v18.15.0, x64)
Errors	0	0	~	~	~	p=1.000 n=6
Symbols	524,639	524,639	~	~	~	p=1.000 n=6
Types	178,906	178,906	~	~	~	p=1.000 n=6
Memory used	462,663k (± 0.01%)	462,717k (± 0.01%)	~	462,667k	462,773k	p=0.128 n=6
Parse Time	3.12s (± 1.34%)	3.11s (± 0.89%)	~	3.09s	3.16s	p=0.934 n=6
Bind Time	1.16s (± 0.89%)	1.17s (± 0.84%)	~	1.16s	1.18s	p=0.437 n=6
Check Time	18.34s (± 0.61%)	18.27s (± 0.34%)	~	18.20s	18.38s	p=0.199 n=6
Emit Time	0.00s	0.00s	~	~	~	p=1.000 n=6
Total Time	22.62s (± 0.40%)	22.55s (± 0.26%)	~	22.51s	22.67s	p=0.230 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 (typescript-bot)]

Jake Bailey (@jakebailey) Here are the results of running the user tests comparing main and refs/pull/58546/merge:

Something interesting changed - please have a look.

Details

lodash

/mnt/ts_downloads/_/m/lodash/tsconfig.json

• [NEW] error TS2345: Argument of type '{ countBy: Function; each: (collection: any[] | any, iteratee?: Function | undefined) => any[] | any; eachRight: (collection: any[] | any, iteratee?: Function | undefined) => any[] | any; ... 24 more ...; sortBy: Function; }' is not assignable to parameter of type 'string'.
  • /mnt/ts_downloads/_/m/lodash/node_modules/lodash/fp/collection.js(2,26)
• [NEW] error TS2345: Argument of type '{ at: Function; chain: (value: any) => any; commit: () => any; lodash: typeof lodash; next: typeof wrapperNext; plant: (value: any) => any; reverse: () => any; ... 6 more ...; wrapperChain: () => any; }' is not assignable to parameter of type 'string'.
  • /mnt/ts_downloads/_/m/lodash/node_modules/lodash/fp/seq.js(2,26)
• [MISSING] error TS2345: Argument of type '{ countBy: Function; each: (collection: any[] | any, iteratee?: Function | undefined) => any; eachRight: (collection: any[] | any, iteratee?: Function | undefined) => any; ... 24 more ...; sortBy: Function; }' is not assignable to parameter of type 'string'.
  • /mnt/ts_downloads/_/m/lodash/node_modules/lodash/fp/collection.js(2,26)
• [MISSING] error TS2345: Argument of type '{ at: Function; chain: (value: any) => any; commit: () => any; lodash: typeof lodash; next: typeof wrapperNext; plant: (value: any) => any; reverse: () => any; tap: (value: any, interceptor: Function) => any; ... 5 more ...; wrapperChain: () => any; }' is not assignable to parameter of type 'string'.
  • /mnt/ts_downloads/_/m/lodash/node_modules/lodash/fp/seq.js(2,26)

[TypeScript Bot (typescript-bot)]

Jake Bailey (@jakebailey) Here are the results of running the top 400 repos comparing main and refs/pull/58546/merge:

Something interesting changed - please have a look.

Details

tamagui/tamagui

9 of 118 projects failed to build with the old tsc and were ignored

packages/config/tsconfig.json

• error TS7056: The inferred type of this node exceeds the maximum length the compiler will serialize. An explicit type annotation is needed.
  • packages/config/src/v2-base.ts#L9 in packages/config/tsconfig.json
  • packages/config/src/v2-native.ts#L6 in packages/config/tsconfig.json
  • packages/config/src/v2-reanimated.ts#L6 in packages/config/tsconfig.json
  • packages/config/src/v2.native.ts#L6 in packages/config/tsconfig.json
  • packages/config/src/v2.ts#L6 in packages/config/tsconfig.json

[Jake Bailey (jakebailey)]

Used your tool to review all 201 significantly changed files. Some notes:

These are examples of notably good changes (good enough to point out how good they are):

• declarationEmitGlobalThisPreserved.ts
• declarationEmitMappedTypeTemplateTypeofSymbol.ts
• defaultDeclarationEmitShadowedNamedCorrectly.ts
• moduleAugmentationDuringSyntheticDefaultCheck.ts
• recursiveFunctionTypes.ts
• subtypingWithConstructSignatures.ts and co
• unionTypeWithRecursiveSubtypeReduction3.ts

Most changes seem like just good reuse updates, so that's good.

Most of the other changes seem to be a couple of categories:

• The preservation of undefined or null, where the test has strictNullChecks disabled (since it's not the default, ugh). Not totally sure how I feel about these, especially if they're going to show in tooltips? But who the heck has this flag off, why do we have this flag, agh
  • e.g. tests/cases/compiler/copyrightWithNewLine1.ts showing null as return for getElementById
• Objects losing | undefined on optional props. Continually worried about this but that's how it goes when copying...

Other stuff:

• constraintSatisfactionWithAny.ts - this looks like a bug in the old code, but why don't I see it in dts emit or hover?
• promisePermutations.ts and co - seems to be a very large instantiation count increase here.

[Titian Cernicova-Dragomir (dragomirtitian)]

constraintSatisfactionWithAny

I investigated and the way the signature is printed is different. The types base-line prints the types using the GenerateNamesForShadowedTypeParams flag, while the quick info does not use this flag so no type parameters will be renamed.

And it's not showing up in declarations, because the type reuse in the case of function declarations is actually not done by the checker printer, it's actually done in the declaration transform, which has its own ideas as to how things should be done (I really think we should unify these a lot more than they are today).

Something that would show the bug is const foo4 = null! as <T extends <T>(x: T) => void>(x: T) => T (Playground Link). In this case declaration emit falls back to type printing in the checker with GenerateNamesForShadowedTypeParams and we can see that the type parameters are not renamed appropriately.

On a related node. Why is renaming of shadowed parameters needed in this case? Since both T definitions come from source at the location, I don't really see how an inlined type would ever need to reference the outer T since the outer T was never accessible in the inner scope to being with. Is this a possible improvement or am I missing some case? (cc: Wesley Wigham (@weswigham))

promisePermutations

I do think most of these instantiations actually hit the instantiation cache, but they get counted anyway. So I don't think the performance impact should be too big.

The reason this happens though is because when printing the return type I need to instantiate the return type I extract from the declaration and check it against the resolved return type in the signature. This is because signature return types might be different to the declaration return type (even after mapping). Ideally we should change the cases where these two diverge and then we can remove the check. I found two cases where this is so:

1. Constructor inherited from the base class.

These constructors signatures actually have the declaration from their original declaration but the return type from their containing class. This is a problem, because if we look at the declaration we get a different return type than the signature. Try as I might I have not been able to find a way, just by looking at the signature, to determine if this is a constructor signature that is inherited (and thus unreliable) or not.

Ex:

typeConstructor<T={}>=new(...args: any[])=>T;functionMixin<TBaseextendsConstructor>(Base: TBase){returnclassextendsBase{};}

In the example the constructor signature of Base is actually tied to the declaration that comes from new (...args: any[]) => T, which will not give the correct return type.

2. Signatures that don't actually have a mapper that can get us from declaration to signature types. For example in

constcreateTransform=<I,O>(tr: (from: I)=>O)=>tr;functionwithP2<P>(p: P){constm=<I,>(from: I)=>({ ...from, ...p});returncreateTransform(m);}constaddP2=withP2({foo: 1});

The signature for addP2 actually comes from the declaration tr: (from: I) => O. But the mapper for the signature only contains I -> I, P -> { foo: number }. This mapper is incorrect, it does not actually have a mapping for O which it should for us to be able to correctly get the return type.

This is because in instantiateSignature when creating the new mapper we have:

freshTypeParameters=map(signature.typeParameters,cloneTypeParameter);mapper=combineTypeMappers(createTypeMapper(signature.typeParameters,freshTypeParameters),mapper);

But signature already has it's own mapper which contains a mapping from O -> I & P. I tried adding the signature mapper in the mix, but that seems to break a bunch of other things and it's an unrelated change I don't feel comfortable commingling in this PR.

[Jake Bailey (jakebailey)]

I investigated and the way the signature is printed is different. The types base-line prints the types using the GenerateNamesForShadowedTypeParams flag, while the quick info does not use this flag so no type parameters will be renamed.

Ah, right. I've wanted to make hover pass that flag, honestly. (related, #55821)

On a related node. Why is renaming of shadowed parameters needed in this case? Since both T definitions come from source at the location, I don't really see how an inlined type would ever need to reference the outer T since the outer T was never accessible in the inner scope to being with. Is this a possible improvement or am I missing some case?

We've iterated on the type parameter renaming code so much that I'm not sure why it doesn't work, honestly. I definitely had fixed some cases like this before, but we undid some of that, then redid some of that. So it does sound like an improvement, but that may be challenging because even though we introduce a new name T into the scope, it's possible that the inside bit could still refer to the outer T indirectly and be printed back. At least, that is my guess.

[Wesley Wigham (weswigham)]

On a related node. Why is renaming of shadowed parameters needed in this case?

It's a bit more pessimistic than it strictly has to be sometimes, but the renaming in general is needed because cases like this

exportconsta=<T,>()=>{typeU=T;return<TextendsU>()=>nullasanyasT;}

exist.

[Wesley Wigham (weswigham)]

I think I'm gonna merge this at this point so we have more time to work on top of it - I like the approach and the changes on the whole look good, and I think that if it doesn't fix some other issues reported with the new node reuse we're doing, it's a required stepping stone for them.