Track recursive homomorphic mapped types by the symbol of their target #55638
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
ahejlsberg
requested review from
DanielRosenwasser,
weswigham and
RyanCavanaugh
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the tsc-only perf test suite on this PR at cd21478. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at cd21478. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at cd21478. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at cd21478. You can monitor the build here. Update: The results are in! |
|
This is awesome, thank you! ❤️ |
|
@ahejlsberg Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@ahejlsberg Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
This is massive, thanks so much for the quick turnaround 😍 |
|
This looks great. Did not expect to see this picked up so quickly :) Thank you! |
|
@ahejlsberg Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
Hey @ahejlsberg, the results of running the DT tests are ready. Branch only errors:Package: raphael |
There was a problem hiding this comment.
Feels maybe a bit odd that Readonly<Foo>, Partial<Foo>, and WhateverMapper<Foo> all have the same recursion identity, but it's probably better than every Partial having the same recursion identity. Feels like it'd be better to use both the target symbol and the modifiers type symbol as an aggregate identity for mapped types like this, if possible - that way if you have mutually recursive mapped types mapping over the same type (eg, you have a self referential type that is deep mapped in a way that alternates between applying readonly and partial or the like), they all get the full 3-repition depth.
typescript-bot
added
For Milestone Bug
|
The DT run has new errors in the Also, two new errors in the |
|
(Or actually we just mark the package as 4.7+ which is okay though odd that variance is the fix) |
|
@typescript-bot pack this |
|
Heya @weswigham, I've started to run the tarball bundle task on this PR at cd21478. You can monitor the build here. |
|
Hey @weswigham, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
One thing from the design meeting notes - include a test case around something like type Expand<T> = {
[K in keyof T]: Expand<Expand<T[K]>>
}; |
|
Ok, a better fix for the new export type RaphaelPaperPluginRegistry<
TTechnology extends RaphaelTechnology = "SVG" | "VML",
T = RaphaelPaper<TTechnology>> = {
/**
* Either the paper plugin method or a new namespace with methods.
*/
[P in keyof T]: RaphaelPaperPluginMethodOrRegistry<TTechnology, T[P]>;
};
type RaphaelPaperPluginMethodOrRegistry<TTechnology extends RaphaelTechnology, T> =
T extends (...args: any) => any
? RaphaelPaperPluginMethod<TTechnology, Parameters<T>, ReturnType<T>>
: RaphaelPaperPluginRegistry<TTechnology, T>;Extracting the conditional type into a separate aliased type allows the compiler to compute variance information for the type arguments, which in turns makes it much cheaper to relate instantiations. This change is backwards compatible and has positive impact on type instantiation counts even before this PR. |
|
Is there anything blocking this? Just, sending the above to DT? |
Based on microsoft/TypeScript#55638 (comment) Extracts the conditional type into a separate type alias. Cheaper to compile and arguably easier to read.
Based on microsoft/TypeScript#55638 (comment) Extracts the conditional type into a separate type alias. Cheaper to compile and arguably easier to read.
Currently, ThomasAribart/json-schema-to-ts#96 and microsoft/TypeScript#55638 point to the fact that this can't easily be completed at runtime. I'm going to explore moving more to the generated types. Signed-off-by: Aramis <sennyeyaramis@gmail.com>
Currently, ThomasAribart/json-schema-to-ts#96 and microsoft/TypeScript#55638 point to the fact that this can't easily be completed at runtime. I'm going to explore moving more to the generated types. Signed-off-by: Aramis <sennyeyaramis@gmail.com>
With this PR, when a homomorphic mapped type is applied to a type with a symbol, we use the symbol of that type as the recursion identity. This is a better strategy than using the symbol of the mapped type, which doesn't work well for recursive mapped types because they end up being considered deeply nested after just three levels.
Note that this PR isn't a general solution to the problem of distinguishing finite and infinite types, rather it is a targeted fix for situations where mapped types are applied to object types that clearly originate in distinct declarations.
Fixes #55535.