TypeScript
TypeScript copied to clipboard
microsoft
Improve reuse of nodes in signatures with type mapping
This PR doesn't have any linked issues. Please open an issue that references this PR. From there we can discuss and prioritise.
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 |
Hey @jakebailey, the results of running the DT tests are ready.
Everything looks the same!
@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 |
- node (v18.15.0, x64)
- 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:
@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)
@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
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
undefinedornull, where the test hasstrictNullChecksdisabled (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
| undefinedon 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.tsand co - seems to be a very large instantiation count increase here.
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: @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:
- 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:
type Constructor<T = {}> = new (...args: any[]) => T;
function Mixin<TBase extends Constructor>(Base: TBase) {
return class extends Base {
};
}
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.
- Signatures that don't actually have a mapper that can get us from declaration to signature types. For example in
const createTransform = <I, O>(tr: (from: I) => O) => tr;
function withP2<P>(p: P) {
const m = <I,>(from: I) => ({ ...from, ...p });
return createTransform(m);
}
const addP2 = 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.
I investigated and the way the signature is printed is different. The types base-line prints the types using the
GenerateNamesForShadowedTypeParamsflag, 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
Tdefinitions come from source at the location, I don't really see how an inlined type would ever need to reference the outerTsince the outerTwas 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.
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
export const a = <T,>() => {
type U = T;
return <T extends U>() => null as any as T;
}
exist.
