Preserve type aliases for union and intersection types #42149
Conversation
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at 237e9ca. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at 237e9ca. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the extended test suite on this PR at 237e9ca. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 237e9ca. You can monitor the build here. |
|
@ahejlsberg Here they are:Comparison Report - master..42149
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
@typescript-bot test this |
|
Heya @ahejlsberg, I've started to run the parallelized community code test suite on this PR at e0d4774. You can monitor the build here. |
|
Heya @ahejlsberg, I've started to run the perf test suite on this PR at e0d4774. You can monitor the build here. Update: The results are in! |
|
Meanwhile, once the test runs finish I will merge this PR. |
I have tested
I reviewed every emit change. It's spot on every single time, resulting in ideal emit, i.e. an expanded union is replaced by a reference to an identifier that is already in scope. As an example, the emit introduced by this now-merged PR... // dts emit from PR#42149
myColorProp: MakeEnum<{
readonly red: "red";
readonly green: "green";
readonly blue: "blue";
}>;...is now replaced with this... // dts emit from e794cb9 "Allow new alias to be associated with type alias instantiation"
myColorProp: Color;This makes the declaration files smaller and easier to read. And it helps prevent inter-project staleness issues. And it will help our documentation generation pipeline that reads the .d.ts files to generate correctly linked HTML. Your work on this is highly appreciated. |
* Create separate types for equivalent aliased unions * Accept new baselines * Preserve original types for union types * Accept new baselines * Preserve intersection origin for union types * Accept new baselines * Accept new baselines * Preserve aliases during relationship checks * Accept new baselines * Preserve aliases for intersection and indexed access types * Accept new baselines * Compute intersection-of-unions cross product without recursion * Accept new baselines * Use denormalized type objects for origin / support 'keyof' origins * Accept new baselines * Fix fourslash test * Recursively extract named union types * Accept new baselines * Map on union origin in mapType to better preserve aliases and origins * Remove redundant call * Accept new baselines * Revert back to declared type when branches produce equivalent union * Accept new baselines * Don't include denormal origin types in regular type statistics * Fix issue with unions not being marked primitive-only * Allow new alias to be associated with type alias instantiation * Accept new baselines * Revert "Accept new baselines" This reverts commit 4507270. * Revert "Allow new alias to be associated with type alias instantiation" This reverts commit 2c2d06d.
|
FYI: I'm upgrading some projects to use TypeScript 4.2 RC and this change is causing issues with Reduced test case (this worked in TS 4.1): type Compact<A> = A extends Function ? A : { [K in keyof A]: A[K] } & {};
type DistributiveOmit<T, K extends keyof T> = T extends unknown ? Omit<T, K> : never;
type Base<T> = T & { base: string };
type A = { a: string };
type B = { b: string };
type C = (Base<A> | Base<B>) & { toRemove: string };
// Given properties outside of the union, it should omit
// $ExpectType { a: string; base: string; } | { b: string; base: string; }
type Test1 = Compact<DistributiveOmit<C, 'toRemove'>>;… but it fails in TS 4.2: I'm not sure there is any way for me to rewrite this test? I think this is going to cause issues for tests in the DefinitelyTyped repo. |
|
@ahejlsberg @RyanCavanaugh @orta @sandersn, I think @OliverJAsh has a point. It looks like we still haven't provided API users with a way to opt out of printing origin types. This means we're probably unprepared for divergences in type display between 4.1 and 4.2+ in DefinitelyTyped. |
|
@DanielRosenwasser Now that 4.2 is out I guess we might start running into issues… |
|
I don't understand the problem well enough yet: does this only cause a problem for API users who have been using text equality on the output of type display? I feel like that's a bad idea anyway, and something that dtslint should stop eventually, switching instead to something like tsd. |
|
I mean, DT does type text comparison (heh), but it does support specifying multiple allowable representations, since we have textual representation changes somewhat regularly. (On DT, we can say |
I completely agree, but as it stands this is what DefinitelyTyped is using—so after TS 4.2, many tests in DefinitelyTyped will no longer function correctly, and there's no way to rewrite them without significantly changing the behaviour of the test. In my example above, we want to test the structure, not the name of the test. This was possible before because TS printed the whole structure, but now it does not. Until dtslint changes its testing infrastructure, I feel like we need an escape hatch so that these tests can continue to function. |
|
So far DT has taken the other option of adding synonyms, so I don't think an escape hatch is needed for dtslint at least [1]. A quick scan shows 667 uses of the [1] An interactive expansion of aliases in quickinfo would be nice though. |
I saw your PR which added a bunch of these (DefinitelyTyped/DefinitelyTyped#50566). I'm concerned that this could cause problems in the future. These synonyms are not testing the same thing, so in the future when the tests are only ran with TypeScript versions 4.2 and above, these tests could pass when they should fail. For example, I could modify my example test from above: type Compact<A> = A extends Function ? A : { [K in keyof A]: A[K] } & {};
-type DistributiveOmit<T, K extends keyof T> = T extends unknown ? Omit<T, K> : never;
+type DistributiveOmit<T, K extends keyof T> = T;
type Base<T> = T & { base: string };
type A = { a: string };
type B = { b: string };
type C = (Base<A> | Base<B>) & { toRemove: string };
// Given properties outside of the union, it should omit
-// $ExpectType { a: string; base: string; } | { b: string; base: string; }
+// $ExpectType { a: string; base: string; } | { b: string; base: string; } || Test1
type Test1 = Compact<DistributiveOmit<C, 'toRemove'>>;I've deliberately broken |
|
The reason I'm not worried about DefinitelyTyped/DefinitelyTyped#50566 is that those As you point out in your original example, type testing via type alias becomes tautological after this PR. |
|
@ahejlsberg Here's another situation where this behavior may be undesirable. Sometimes I'll use a type as a sort of lookup for related sub types (where I can't justify creating a named type for each). For example: I can then create a type that behaves like an enum of the different animals: In TS 4.1 if I hover my cursor over But in TS 4.2 it's now obviously going to become the less desirable: Fortunately when I type the following I still get autocompleted suggestions (which really is why I'm doing this in the first place): So some sort of opt out would be very valuable for this case too. This is the first idea off the top of my head. Could such a helper be created today? |
The following should do the trick: type Resolve<T> = T extends T ? T : never; |
|
There is might be a bug which was provided between TS 4.1.5 and 4.2.3. |
With this PR we improve preservation of type aliases for union and intersection types. Previously, an alias associated with a union type was lost when that type was made part of another union. For example:
Previously, the types of all but the
shapevariable would be displayed as fully expanded unions (see in playground). Now, the types are displayed as they were written. This makes diagnostics involving these types significantly easier to comprehend.This PR also makes it possible to have distinct aliases for the same union or intersection types. For example:
Previously we'd show the alias
StrOrNum1for bothsn1andsn2.The key implementation change in the PR is that we now include alias symbols in the key used to intern union, intersection, and indexed access types. Thus, aliased forms of these types get a separate type identity. We additionally include an "origin" description in union types when the originating declaration has fewer constituents than the fully expanded and normalized union. This origin may actually be an intersection, as is the case in the type
(A | B | C) & (D | E | F). A slight performance impact is expected because we now create more type identities for equivalent unions.Fixes #35654.