Fix logic for determining whether to simplify keyof on mapped types #44042

ahejlsberg · 2021-05-11T21:27:15Z

weswigham · 2021-05-12T00:26:47Z

src/compiler/checker.ts

+                return type.flags & TypeFlags.TypeParameter ? type === typeVariable :
+                    type.flags & TypeFlags.Conditional ? (<ConditionalType>type).root.isDistributive && isDistributiveForTypeParameter((<ConditionalType>type).checkType) :
+                    type.flags & (TypeFlags.UnionOrIntersection | TypeFlags.TemplateLiteral) ? every((<UnionOrIntersectionType | TemplateLiteralType>type).types, isDistributiveForTypeParameter) :
+                    type.flags & TypeFlags.IndexedAccess ? isDistributiveForTypeParameter((<IndexedAccessType>type).indexType) :


Can't the object type also essentially pass thru distrubutivity? Since (A | B)["x"] is identical to A["x"] | B["x"] (for reading, at least)?

Isn't that exactly what this does? It completely ignores the object type.

With latest commit we now check both objectType and indexType.

weswigham · 2021-05-12T00:29:21Z

src/compiler/checker.ts

+                    type.flags & TypeFlags.Conditional ? (<ConditionalType>type).root.isDistributive && isDistributiveForTypeParameter((<ConditionalType>type).checkType) :
+                    type.flags & (TypeFlags.UnionOrIntersection | TypeFlags.TemplateLiteral) ? every((<UnionOrIntersectionType | TemplateLiteralType>type).types, isDistributiveForTypeParameter) :
+                    type.flags & TypeFlags.IndexedAccess ? isDistributiveForTypeParameter((<IndexedAccessType>type).indexType) :
+                    type.flags & TypeFlags.Substitution ? isDistributiveForTypeParameter((<SubstitutionType>type).substitute) :


The substitute may erase the original type parameter at the position (eg, because it mixed in any), so it may be pertinent to check for either the substitute or the base type containing a match.

Not necessary with the latest changes.

weswigham · 2021-05-12T00:32:59Z

src/compiler/checker.ts

+                    type.flags & TypeFlags.IndexedAccess ? isDistributiveForTypeParameter((<IndexedAccessType>type).indexType) :
+                    type.flags & TypeFlags.Substitution ? isDistributiveForTypeParameter((<SubstitutionType>type).substitute) :
+                    type.flags & TypeFlags.StringMapping ? isDistributiveForTypeParameter((<StringMappingType>type).type) :
+                    type.flags & TypeFlags.Index ? false :


What about, eg, keyof {[_ in T]: whatever}? That's still essentially distributive over T since it should just be T (T just has to be constrained to valid key types).

Isn't that handled by getNameTypeFromMappedType(mappedType) || typeVariable in the original call to isDistributiveForTypeParameter? If there's no NameType, typeVariable will immediately pass the check.

A keyof { [_ in T]: xxx } will, if possible, already have been simplified by getIndexType.

ahejlsberg · 2021-05-14T14:39:36Z

Some background on the change in this PR...

For a type keyof { [K in X as Y]: ... } we want to determine if it is correct to simplify to an instantiation of Y with X substituted for K. Let's say X is a union type 'a' | 'b' | 'c'. Without simplification, the keyof operation becomes a union of three instantiations of Y with 'a', 'b', and 'c' respectively substituted for K. We want to determine if it is correct to simplify to a single instantiation of Y with 'a' | 'b' | 'c' substituted for K, which it will be when Y distributes over union types in K. A complication is that we can't always inspect every constituent of Y because it might be circular or infinitely expanding. So, for some types we can't easily provide an answer. However, unless we can prove otherwise, it is reasonable to assume that Y depends on K in some way, or else the entire construct is just a needlessly complicated way of writing just Y.

With that in mind, the best approach is to not simplify unless we can definitely prove a type to be distributive. Originally the code was doing it the other way around (assuming distributive and attempting to prove otherwise), but that meant we'd get it wrong in cases where the analysis was incomplete.

markw65 · 2021-05-16T16:30:11Z

src/compiler/checker.ts

+            const typeVariable = getTypeParameterFromMappedType(mappedType);
+            return isDistributiveForTypeParameter(getNameTypeFromMappedType(mappedType) || typeVariable);
+            function isDistributiveForTypeParameter(type: Type): boolean {
+                return type.flags & TypeFlags.TypeParameter ? type === typeVariable :


Not a bug per se, but isn't this check too strict? eg it causes

type NotDistributive<T, U, V = string> = keyof { [ K in keyof T as V & (K extends U ? K : never)]: T[K]};

to fail the isDistributiveForTypeParameter, because V is rejected by the every call.

Maybe add a flag to isDistributiveForTypeParameter saying whether or not you're under a checkType, default it to false (for the initial call, and the every call), pass true for the checkType call, and pass it through for the other cases?

Agreed, fixed in the latest commit.

markw65 · 2021-05-16T16:31:17Z

src/compiler/checker.ts

+                return type.flags & TypeFlags.TypeParameter ? type === typeVariable :
+                    type.flags & TypeFlags.Conditional ? (<ConditionalType>type).root.isDistributive && isDistributiveForTypeParameter((<ConditionalType>type).checkType) :
+                    type.flags & (TypeFlags.UnionOrIntersection | TypeFlags.TemplateLiteral) ? every((<UnionOrIntersectionType | TemplateLiteralType>type).types, isDistributiveForTypeParameter) :
+                    type.flags & TypeFlags.IndexedAccess ? isDistributiveForTypeParameter((<IndexedAccessType>type).indexType) :


Isn't that exactly what this does? It completely ignores the object type.

markw65 · 2021-05-16T16:33:09Z

src/compiler/checker.ts

+                    type.flags & TypeFlags.IndexedAccess ? isDistributiveForTypeParameter((<IndexedAccessType>type).indexType) :
+                    type.flags & TypeFlags.Substitution ? isDistributiveForTypeParameter((<SubstitutionType>type).substitute) :
+                    type.flags & TypeFlags.StringMapping ? isDistributiveForTypeParameter((<StringMappingType>type).type) :
+                    type.flags & TypeFlags.Index ? false :


Isn't that handled by getNameTypeFromMappedType(mappedType) || typeVariable in the original call to isDistributiveForTypeParameter? If there's no NameType, typeVariable will immediately pass the check.

# Conflicts: # src/compiler/checker.ts

markw65 · 2021-05-18T20:30:39Z

src/compiler/checker.ts

+                    type.flags & TypeFlags.IndexedAccess ? isDistributive((type as IndexedAccessType).objectType) && isDistributive((type as IndexedAccessType).indexType) :
+                    type.flags & TypeFlags.Substitution ? isDistributive((type as SubstitutionType).substitute) :
+                    type.flags & TypeFlags.StringMapping ? isDistributive((type as StringMappingType).type) :
+                    true;


Your background comment says With that in mind, the best approach is to not simplify unless we can definitely prove a type to be distributive, so shouldn't this true be false?

In fact, another test case [edit: linked to the correct test case]. Again, it should reject the call to f, but fails to do so in the playground, and fails to do so after this fix.

Changing line 14349 to !(type.flags & TypeFlags.Index) fixes this particular test case, but if we're trying to be conservative false would be better. Or maybe !!(type.flags & TypeFlags.Primitive) to avoid being overly conservative [edit: more flags would need to be checked; eg at least TypeFlags.TypeParameter to avoid being way too conservative]

Sorry, not sure what's going on with the playground links - they seem to randomly link to other test cases I've been playing with. In case its behaving in a similar way for you, here's the code I'm trying to link to:

type KeyofMapped<T, U> = keyof { [ K in keyof T as keyof ({[P in K as T[P] extends U ? K : never]: true}) ]: T[K] }; interface M { a: boolean; b: number; } function f(x: KeyofMapped<M, boolean>) { return x; } f("b");

With latest commits now errors as expected.

markw65 · 2021-05-18T20:54:00Z

src/compiler/checker.ts

+            }
+            function isDistributiveCheckType(type: Type): boolean {
+                return !!(type.flags & TypeFlags.TypeParameter) && type === typeVariable ||
+                    !!(type.flags & TypeFlags.Conditional) && (type as ConditionalType).root.isDistributive && isDistributiveCheckType((type as ConditionalType).checkType);


Wouldn't this be problematic?

type Dist<U, V, W, X> = U extends V ? W : X; type KeyofMapped<T, U, V> = keyof { [ K in keyof T as Dist<K extends U ? T[K] : never, T[K], K, never> ]: T[K] };

Now, we hit this "inner, distributive conditional" case, and the inner conditional is distributive wrt K, so we do the transformation. When its finally instantiated, the inner conditional correctly expands to a union of those fields of T specified by U. So the checkType of the outer conditional is a union, and we distribute that too; but now we're distributing something that isn't actually K. So as long as at least one field was picked by the inner conditional, we end up choosing all the keys.

Here's a full test case.

It should reject the call to f, but accepts it instead. The playground accepts it, and this fix also accepts it. If I remove line 14353 from your fix, it correctly rejects it.

Similarly, here's the code I'm trying to link to for this example:

type Dist<U, V, W, X> = U extends V ? W : X; type KeyofMapped<T, U> = keyof { [ K in keyof T as Dist<K extends U ? T[K] : never, T[K], K, never> ]: T[K] }; interface M { a: boolean; b: number; } function f(x : KeyofMapped<M, "a">) { return x; } f("b");

Similarly, with latest commits now errors as expected.

markw65

I have a higher level question: why do this transformation at all?

If it's done correctly (and we seem to be seeing that that's actually quite hard), it won't change the behavior, so it doesn't appear to be necessary. Is it supposed to be speeding up the compilation step by simplifying the expression? In that case, its not clear to me that it's a win.

Typically I will have

type MakeMappedType<Args> = { ... };
type KeyofMappedType<Args> = keyof MakeMappedType<Args>;

And then I will always use them in pairs - ie for every set of Args that I instantiate KeyofMappedType, I will also instantiate MakeMappedType (or I won't even bother with KeyofMappedType, and just use keyof MakeMappedType directly). Assuming my usage model is typical, there doesn't seem to be a win, since without the transformation, keyof MakeMappedType would just read the property keys from the already-instantiated mapped type.

ahejlsberg · 2021-05-19T20:14:28Z

@markw65 Thanks for the latest tests which should now work as expected.

WRT why we do this transformation in the first place, the short answer is: In order to ensure types that are expected to be compatible actually are compatible. For example

type T1<T> = keyof T & string;
type T2<T> = keyof { [P in keyof T as P & string]: number };

You'd expect these two types to be compatible since they'll always have the same sets of properties, but they wouldn't be without the simplification.

markw65 · 2021-05-19T22:12:44Z

You'd expect these two types to be compatible since they'll always have the same sets of properties, but they wouldn't be without the simplification.

@ahejlsberg Sorry, would you mind clarifying that? Maybe with an example, or an explanation of what I'm doing wrong here:

type T1<T> = (keyof T) & string;
type T2<T> = keyof { [P in keyof T as P & string]: number };
//type T2<T> = keyof { [P in keyof T as [P] extends [P] ? P & string : never]: number };

interface M {
    a: number;
    b: number;
    [42]: number;
}

function f1(x: T1<M> extends T2<M> ? boolean : never) {
    return x;
}
function f2(x: T2<M> extends T1<M> ? boolean : never) {
    return x;
}
function f3(x: T1<M> extends T2<M> ? (T2<M> extends T1<M> ? boolean : never) : never) {
    return x;
}
f1(false);
f2(false);
f3(false);

In the above code, f1 and f2 both take boolean arguments, which seems to indicate that (as you said) T1 and T2 produce compatible types (each extends the other). But if I try to combine the tests, in f3 it doesn't take boolean (it also doesn't tell me what it does take).

If I change the definition of T2 so that the transformation doesn't happen (the commented out version of T2), I get exactly the same behavior (ie, I believe, based on f1 and f2, that the types are still compatible; but something odd happens in the f3 case).

ahejlsberg · 2021-05-19T22:47:43Z

@markw65 It works because T1<M> and T2<M> both resolve to the union type 'a' | 'b' in your example, so by the time we relate the types they're actually the same type. However, things don't work out if M is generic because the types are then preserved in their higher order form. So, adding to your example:

function foo1(x: T1<M>, y: T2<M>) {
    x = y;  // Ok
    y = x;  // Ok
}

function foo2<M>(x: T1<M>, y: T2<M>) {
    x = y;  // Error in non-simplified case
    y = x;  // Error in non-simplified case
}

markw65 · 2021-05-20T02:11:19Z

@ahejlsberg Thanks. That makes sense. But in that case, it seems like its not really ok to be conservative about when to do the transformation? eg right now it would reject

type NotTransformed<T, U, V, W> = keyof { [K in keyof T as U extends V ? (K extends W ? K : never) : K]: T[K] };

even though it is distributive wrt K.

[edit: never mind about the last part, it is a bug, and it's fixed in the nightly. Sorry should have checked that first]. Can you explain what's going on with f3's parameter type? We know that T1<M> extends T2<M> and that T2<M> extends T1<M>, so the nested conditional should evaluate to boolean, but it doesn't seem to. Is that a bug? Or am I doing something stupid here? The really weird thing is that if I change it to T1<M> extends T2<M> ? (T2<M> extends T1<M> ? true : false) : 42, then f3 doesn't accept true, false or 42 as its parameter:

foo.ts:32:4 - error TS2345: Argument of type 'boolean' is not assignable to parameter of type '"a" | "b" extends T1<M> ? true : false'.

ahejlsberg added 2 commits May 11, 2021 14:13

Fix logic for determining whether to simplify keyof on mapped types

9f721e4

Add regression test

d1a7108

typescript-bot assigned ahejlsberg May 11, 2021

typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels May 11, 2021

ahejlsberg requested review from RyanCavanaugh and weswigham May 11, 2021 21:28

weswigham reviewed May 12, 2021

View reviewed changes

markw65 reviewed May 16, 2021

View reviewed changes

ahejlsberg added 3 commits May 18, 2021 07:10

Merge branch 'master' into fix44019

2486dad

# Conflicts: # src/compiler/checker.ts

Improve hasDistributiveNameType check

879753c

Add more tests

772e716

markw65 reviewed May 18, 2021

View reviewed changes

markw65 reviewed May 19, 2021

View reviewed changes

ahejlsberg added 2 commits May 19, 2021 13:07

Address code review feedback

007940c

Add more tests

a9a00e8

RyanCavanaugh approved these changes May 19, 2021

View reviewed changes

ahejlsberg merged commit 73736d9 into master May 19, 2021

ahejlsberg deleted the fix44019 branch May 19, 2021 20:43

DanielRosenwasser mentioned this pull request May 25, 2021

Design Meeting Notes, 5/19/2021 #44256

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix logic for determining whether to simplify keyof on mapped types #44042

Fix logic for determining whether to simplify keyof on mapped types #44042

ahejlsberg commented May 11, 2021

weswigham May 12, 2021

markw65 May 16, 2021

ahejlsberg May 18, 2021

weswigham May 12, 2021

ahejlsberg May 18, 2021

weswigham May 12, 2021

markw65 May 16, 2021

ahejlsberg May 18, 2021

ahejlsberg commented May 14, 2021

markw65 May 16, 2021

ahejlsberg May 18, 2021

markw65 May 16, 2021

markw65 May 16, 2021

markw65 May 18, 2021

markw65 May 19, 2021 •

edited

Loading

markw65 May 19, 2021

ahejlsberg May 19, 2021

markw65 May 18, 2021

markw65 May 18, 2021

markw65 May 19, 2021

ahejlsberg May 19, 2021

markw65 left a comment

ahejlsberg commented May 19, 2021

markw65 commented May 19, 2021 •

edited

Loading

ahejlsberg commented May 19, 2021

markw65 commented May 20, 2021 •

edited

Loading

Fix logic for determining whether to simplify keyof on mapped types #44042

Fix logic for determining whether to simplify keyof on mapped types #44042

Conversation

ahejlsberg commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ahejlsberg commented May 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markw65 May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

markw65 left a comment

Choose a reason for hiding this comment

ahejlsberg commented May 19, 2021

markw65 commented May 19, 2021 • edited Loading

ahejlsberg commented May 19, 2021

markw65 commented May 20, 2021 • edited Loading

markw65 May 19, 2021 •

edited

Loading

markw65 commented May 19, 2021 •

edited

Loading

markw65 commented May 20, 2021 •

edited

Loading