Fixes reverse mapped type members limiting constraint #56911
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
@jakebailey would you mind preparing a TS playground for this one? :) |
|
@typescript-bot test top200 @typescript-bot perf test this |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 03848cf. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 03848cf. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 03848cf. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the regular perf test suite on this PR at 03848cf. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 03848cf. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, 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 |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
Hey @jakebailey, the results of running the DT tests are ready. |
|
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
typescript-bot
added
For Backlog Bug
| return; | ||
| } | ||
| const limitedConstraint = getIntersectionType((origin as IntersectionType).types.filter(t => t !== type.constraintType)); | ||
| return limitedConstraint !== neverType ? limitedConstraint : undefined; | ||
| const mapper = appendTypeMapping(type.mappedType.mapper, type.constraintType.type, keyofConstraintObjectType); |
There was a problem hiding this comment.
@weswigham would you mind taking a look at this PR? It's a fix to the recently-ish merged #55811
I have a concern that using keyofConstraintObjectType here is not that great since it might change the meaning of T[K]. In reality, I'd like to just map keyof T... but type mappings only work on type parameters.
There was a problem hiding this comment.
Hm, swapping keyof Whatever out for string | number | symbol and getting the resulting type is the goal here, right? That definitely aughta result in the useful bound. It's definitely possible T[K] if it were used in a constraint could return weird stuff here by doing an instantiation in this way (which could be actually done because of some conditional type weirdness).... hmmm... Maybe instantiate with T with T & keyofConstraintObjectType so specific-key indexing still turns up the specific member types, but keyof returns string | number | symbol.
There was a problem hiding this comment.
ah yes! great idea with that intersection :) I pushed out this change
| const limitedConstraint = getIntersectionType((origin as IntersectionType).types.filter(t => t !== type.constraintType)); | ||
| return limitedConstraint !== neverType ? limitedConstraint : undefined; | ||
| const mapper = appendTypeMapping(type.mappedType.mapper, type.constraintType.type, keyofConstraintObjectType); | ||
| return getBaseConstraintOrType(instantiateType(constraint, mapper)); |
There was a problem hiding this comment.
Not a fan of getBaseConstraintOrType - it is almost never the right operation for the job.... why is it even needed here?
There was a problem hiding this comment.
Diff without getBaseConstraintOrType it:
git diff
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index bb3203b745..1752d5e162 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -13677,7 +13677,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return;
}
const mapper = appendTypeMapping(type.mappedType.mapper, type.constraintType.type, keyofConstraintObjectType);
- return getBaseConstraintOrType(instantiateType(constraint, mapper));
+ return instantiateType(constraint, mapper);
}
function resolveReverseMappedTypeMembers(type: ReverseMappedType) {
diff --git a/tests/baselines/reference/isomorphicMappedTypeInference.js b/tests/baselines/reference/isomorphicMappedTypeInference.js
index ee60e3717f..2971c8eb1e 100644
--- a/tests/baselines/reference/isomorphicMappedTypeInference.js
+++ b/tests/baselines/reference/isomorphicMappedTypeInference.js
@@ -372,26 +372,11 @@ declare function f21<T, K extends keyof T>(obj: Pick<T, K>): K;
declare function f22<T, K extends keyof T>(obj: Boxified<Pick<T, K>>): T;
declare function f23<T, U extends keyof T, K extends U>(obj: Pick<T, K>): T;
declare function f24<T, U, K extends keyof T | keyof U>(obj: Pick<T & U, K>): T & U;
-declare let x0: {
- foo: number;
- bar: string;
-};
-declare let x1: "foo" | "bar";
-declare let x2: {
- foo: number;
- bar: string;
-};
-declare let x3: {
- foo: number;
- bar: string;
-};
-declare let x4: {
- foo: number;
- bar: string;
-} & {
- foo: number;
- bar: string;
-};
+declare let x0: {};
+declare let x1: never;
+declare let x2: {};
+declare let x3: {};
+declare let x4: {};
declare function getProps<T, K extends keyof T>(obj: T, list: K[]): Pick<T, K>;
declare const myAny: any;
declare const o1: Pick<any, "foo" | "bar">;
diff --git a/tests/baselines/reference/isomorphicMappedTypeInference.types b/tests/baselines/reference/isomorphicMappedTypeInference.types
index 32b74150e5..919a72f9bf 100644
--- a/tests/baselines/reference/isomorphicMappedTypeInference.types
+++ b/tests/baselines/reference/isomorphicMappedTypeInference.types
@@ -533,8 +533,8 @@ declare function f24<T, U, K extends keyof T | keyof U>(obj: Pick<T & U, K>): T
>obj : Pick<T & U, K>
let x0 = f20({ foo: 42, bar: "hello" });
->x0 : { foo: number; bar: string; }
->f20({ foo: 42, bar: "hello" }) : { foo: number; bar: string; }
+>x0 : {}
+>f20({ foo: 42, bar: "hello" }) : {}
>f20 : <T, K extends keyof T>(obj: Pick<T, K>) => T
>{ foo: 42, bar: "hello" } : { foo: number; bar: string; }
>foo : number
@@ -543,8 +543,8 @@ let x0 = f20({ foo: 42, bar: "hello" });
>"hello" : "hello"
let x1 = f21({ foo: 42, bar: "hello" });
->x1 : "foo" | "bar"
->f21({ foo: 42, bar: "hello" }) : "foo" | "bar"
+>x1 : never
+>f21({ foo: 42, bar: "hello" }) : never
>f21 : <T, K extends keyof T>(obj: Pick<T, K>) => K
>{ foo: 42, bar: "hello" } : { foo: number; bar: string; }
>foo : number
@@ -553,8 +553,8 @@ let x1 = f21({ foo: 42, bar: "hello" });
>"hello" : "hello"
let x2 = f22({ foo: { value: 42} , bar: { value: "hello" } });
->x2 : { foo: number; bar: string; }
->f22({ foo: { value: 42} , bar: { value: "hello" } }) : { foo: number; bar: string; }
+>x2 : {}
+>f22({ foo: { value: 42} , bar: { value: "hello" } }) : {}
>f22 : <T, K extends keyof T>(obj: Boxified<Pick<T, K>>) => T
>{ foo: { value: 42} , bar: { value: "hello" } } : { foo: { value: number; }; bar: { value: string; }; }
>foo : { value: number; }
@@ -567,8 +567,8 @@ let x2 = f22({ foo: { value: 42} , bar: { value: "hello" } });
>"hello" : "hello"
let x3 = f23({ foo: 42, bar: "hello" });
->x3 : { foo: number; bar: string; }
->f23({ foo: 42, bar: "hello" }) : { foo: number; bar: string; }
+>x3 : {}
+>f23({ foo: 42, bar: "hello" }) : {}
>f23 : <T, U extends keyof T, K extends U>(obj: Pick<T, K>) => T
>{ foo: 42, bar: "hello" } : { foo: number; bar: string; }
>foo : number
@@ -577,8 +577,8 @@ let x3 = f23({ foo: 42, bar: "hello" });
>"hello" : "hello"
let x4 = f24({ foo: 42, bar: "hello" });
->x4 : { foo: number; bar: string; } & { foo: number; bar: string; }
->f24({ foo: 42, bar: "hello" }) : { foo: number; bar: string; } & { foo: number; bar: string; }
+>x4 : {}
+>f24({ foo: 42, bar: "hello" }) : {}
>f24 : <T, U, K extends keyof T | keyof U>(obj: Pick<T & U, K>) => T & U
>{ foo: 42, bar: "hello" } : { foo: number; bar: string; }
>foo : number
@@ -615,10 +615,10 @@ const o2: { foo: any; bar: any } = getProps(myAny, ['foo', 'bar']);
>o2 : { foo: any; bar: any; }
>foo : any
>bar : any
->getProps(myAny, ['foo', 'bar']) : Pick<any, "foo" | "bar">
+>getProps(myAny, ['foo', 'bar']) : Pick<any, string>
>getProps : <T, K extends keyof T>(obj: T, list: K[]) => Pick<T, K>
>myAny : any
->['foo', 'bar'] : ("foo" | "bar")[]
+>['foo', 'bar'] : string[]
>'foo' : "foo"
>'bar' : "bar"
diff --git a/tests/baselines/reference/reverseMappedTypeLimitedConstraintWithIntersection1.types b/tests/baselines/reference/reverseMappedTypeLimitedConstraintWithIntersection1.types
index e66efc74dd..215caee211 100644
--- a/tests/baselines/reference/reverseMappedTypeLimitedConstraintWithIntersection1.types
+++ b/tests/baselines/reference/reverseMappedTypeLimitedConstraintWithIntersection1.types
@@ -516,10 +516,10 @@ declare function fn3<T1, T2>(obj: {
}): [T1, T2];
const result3 = fn3({
->result3 : [{ a: string; b: boolean; }, { a: number; b: null; }]
->fn3({ a: { v1: "foo", v2: 100, }, b: { v1: true, v2: null, },}) : [{ a: string; b: boolean; }, { a: number; b: null; }]
+>result3 : [{}, {}]
+>fn3({ a: { v1: "foo", v2: 100, }, b: { v1: true, v2: null, },}) : [{}, {}]
>fn3 : <T1, T2>(obj: { [K in keyof T1 & keyof T2]: { v1: T1[K]; v2: T2[K]; }; }) => [T1, T2]
->{ a: { v1: "foo", v2: 100, }, b: { v1: true, v2: null, },} : { a: { v1: string; v2: number; }; b: { v1: true; v2: null; }; }
+>{ a: { v1: "foo", v2: 100, }, b: { v1: true, v2: null, },} : { a: { v1: string; v2: number; }; b: { v1: boolean; v2: null; }; }
a: {
>a : { v1: string; v2: number; }
@@ -535,11 +535,11 @@ const result3 = fn3({
},
b: {
->b : { v1: true; v2: null; }
->{ v1: true, v2: null, } : { v1: true; v2: null; }
+>b : { v1: boolean; v2: null; }
+>{ v1: true, v2: null, } : { v1: boolean; v2: null; }
v1: true,
->v1 : true
+>v1 : boolean
>true : true
v2: null,
@@ -571,14 +571,14 @@ declare function fn4<T, E extends Record<string, number>>(arg: {
}): [T, E];
const result4 = fn4({
->result4 : [{ a: string; b: boolean; }, { a: 404; b: 500; }]
->fn4({ a: { data: "foo", onSuccess: (dataArg) => { dataArg; }, error: 404, onError: (errorArg) => { errorArg; }, }, b: { data: true, onSuccess: (dataArg) => { dataArg; }, error: 500, onError: (errorArg) => { errorArg; }, },}) : [{ a: string; b: boolean; }, { a: 404; b: 500; }]
+>result4 : [{ a: string; b: boolean; }, {}]
+>fn4({ a: { data: "foo", onSuccess: (dataArg) => { dataArg; }, error: 404, onError: (errorArg) => { errorArg; }, }, b: { data: true, onSuccess: (dataArg) => { dataArg; }, error: 500, onError: (errorArg) => { errorArg; }, },}) : [{ a: string; b: boolean; }, {}]
>fn4 : <T, E extends Record<string, number>>(arg: { [K in keyof T & keyof E]: { data: T[K]; onSuccess: (data: T[K]) => void; error: E[K]; onError: (data: E[K]) => void; }; }) => [T, E]
->{ a: { data: "foo", onSuccess: (dataArg) => { dataArg; }, error: 404, onError: (errorArg) => { errorArg; }, }, b: { data: true, onSuccess: (dataArg) => { dataArg; }, error: 500, onError: (errorArg) => { errorArg; }, },} : { a: { data: string; onSuccess: (dataArg: string) => void; error: 404; onError: (errorArg: 404) => void; }; b: { data: true; onSuccess: (dataArg: boolean) => void; error: 500; onError: (errorArg: 500) => void; }; }
+>{ a: { data: "foo", onSuccess: (dataArg) => { dataArg; }, error: 404, onError: (errorArg) => { errorArg; }, }, b: { data: true, onSuccess: (dataArg) => { dataArg; }, error: 500, onError: (errorArg) => { errorArg; }, },} : { a: { data: string; onSuccess: (dataArg: string) => void; error: number; onError: (errorArg: unknown) => void; }; b: { data: boolean; onSuccess: (dataArg: boolean) => void; error: number; onError: (errorArg: unknown) => void; }; }
a: {
->a : { data: string; onSuccess: (dataArg: string) => void; error: 404; onError: (errorArg: 404) => void; }
->{ data: "foo", onSuccess: (dataArg) => { dataArg; }, error: 404, onError: (errorArg) => { errorArg; }, } : { data: string; onSuccess: (dataArg: string) => void; error: 404; onError: (errorArg: 404) => void; }
+>a : { data: string; onSuccess: (dataArg: string) => void; error: number; onError: (errorArg: unknown) => void; }
+>{ data: "foo", onSuccess: (dataArg) => { dataArg; }, error: 404, onError: (errorArg) => { errorArg; }, } : { data: string; onSuccess: (dataArg: string) => void; error: number; onError: (errorArg: unknown) => void; }
data: "foo",
>data : string
@@ -594,25 +594,25 @@ const result4 = fn4({
},
error: 404,
->error : 404
+>error : number
>404 : 404
onError: (errorArg) => {
->onError : (errorArg: 404) => void
->(errorArg) => { errorArg; } : (errorArg: 404) => void
->errorArg : 404
+>onError : (errorArg: unknown) => void
+>(errorArg) => { errorArg; } : (errorArg: unknown) => void
+>errorArg : unknown
errorArg;
->errorArg : 404
+>errorArg : unknown
},
},
b: {
->b : { data: true; onSuccess: (dataArg: boolean) => void; error: 500; onError: (errorArg: 500) => void; }
->{ data: true, onSuccess: (dataArg) => { dataArg; }, error: 500, onError: (errorArg) => { errorArg; }, } : { data: true; onSuccess: (dataArg: boolean) => void; error: 500; onError: (errorArg: 500) => void; }
+>b : { data: boolean; onSuccess: (dataArg: boolean) => void; error: number; onError: (errorArg: unknown) => void; }
+>{ data: true, onSuccess: (dataArg) => { dataArg; }, error: 500, onError: (errorArg) => { errorArg; }, } : { data: boolean; onSuccess: (dataArg: boolean) => void; error: number; onError: (errorArg: unknown) => void; }
data: true,
->data : true
+>data : boolean
>true : true
onSuccess: (dataArg) => {
@@ -625,16 +625,16 @@ const result4 = fn4({
},
error: 500,
->error : 500
+>error : number
>500 : 500
onError: (errorArg) => {
->onError : (errorArg: 500) => void
->(errorArg) => { errorArg; } : (errorArg: 500) => void
->errorArg : 500
+>onError : (errorArg: unknown) => void
+>(errorArg) => { errorArg; } : (errorArg: unknown) => void
+>errorArg : unknown
errorArg;
->errorArg : 500
+>errorArg : unknown
},
},Dissecting one of the failures:
declare function fn3<T1, T2>(obj: {
[K in keyof T1 & keyof T2]: {
v1: T1[K];
v2: T2[K];
};
}): [T1, T2];
const result3 = fn3({
a: {
v1: "foo",
v2: 100,
},
b: {
v1: true,
v2: null,
},
});When creating this limiting constraint for T1 I currently instantiate T1 in the constraint with keyofConstraintObjectType. That leaves us with the other part of this constraint since it's keyof T1 & keyof T2. So after instantiation we are left with (string | number | symbol) & keyof T2. This getBaseConstraintOrType tries to reduce it further to string | number | symbol.
Thinking about it now - what we want is to do the same thing for all reverse mapped types created based on the type.source object. In the example above, T2 is not a reverse mapped type though (it's a type parameter). And on top of that, even keyof T2 doesn't exactly have to end up being iterating over a reverse mapped type. If T2 appears somewhere naked or something then the reverse mapped type won't be the best inference candidate.
So I'm not sure how to properly deal with this here - since even keeping tabs on all potential reverse mapped types created from type.source could not work here (as we don't have a guarantee that other type params will actually end up being inferred as reverse mapped types). A failing test case for this would be something like this:
declare function fn5<T1, T2>(
obj1: {
[K in keyof T1 & keyof T2]: T1[K];
},
obj2: T2,
): [T1, T2];
const result5 = fn5(
{
a: "foo",
b: 100,
},
{
a: true,
},
);It works right now, in a sense that we get EPC on b but inferred T1 includes b property while it shouldn't. By including it we end up in square zero (it could lead to a different manifestation of #56910 ).
I think that perhaps it becomes even more crucial to make those reversed mapped types aware of the original inference context etc. To get the best results we should have access to the inferred types of other type params involved in all of this.
There was a problem hiding this comment.
It works right now, in a sense that we get EPC on b but inferred T1 includes b property while it shouldn't. By including it we end up in square zero (it could lead to a different manifestation of #56910 ).
To complete this with a test case:
declare function fn6<T1 extends Record<string, string>, T2>(
obj1: {
[K in keyof T1 & keyof T2]: T1[K];
},
obj2: T2,
): [T1, T2];
const obj1_6 = {
a: "foo",
b: 100,
};
const result6 = fn6(obj1_6, {
a: true,
});
result6;
// ^? [Record<string, string>, { a: boolean }]| return; | ||
| } | ||
| const limitedConstraint = getIntersectionType((origin as IntersectionType).types.filter(t => t !== type.constraintType)); | ||
| return limitedConstraint !== neverType ? limitedConstraint : undefined; | ||
| const mapper = appendTypeMapping(type.mappedType.mapper, type.constraintType.type, keyofConstraintObjectType); |
There was a problem hiding this comment.
Hm, swapping keyof Whatever out for string | number | symbol and getting the resulting type is the goal here, right? That definitely aughta result in the useful bound. It's definitely possible T[K] if it were used in a constraint could return weird stuff here by doing an instantiation in this way (which could be actually done because of some conditional type weirdness).... hmmm... Maybe instantiate with T with T & keyofConstraintObjectType so specific-key indexing still turns up the specific member types, but keyof returns string | number | symbol.
Those tests are not in the new test content but rather in the old content of that test file that I touched. I just rechecked it and both of them are there. They are almost identical - variable names involved don't include |
fixes #56910