Skip to content

Fixes reverse mapped type members limiting constraint #56911

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Andarist wants to merge 3 commits into
base: main
Choose a base branch
from
+1,251 −843
Open

Fixes reverse mapped type members limiting constraint #56911

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
03848cf
Fixes reverse mapped type members limiting constraint
Andarist Dec 31, 2023
ab334b9
add more test cases
Andarist Jan 4, 2024
31839d2
intersect with the original type to avoid `T[K]` being instantiated i…
Andarist Jan 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 23 additions & 20 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2038,6 +2038,8 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
var unknownEmptyObjectType = createAnonymousType(/*symbol*/ undefined, emptySymbols, emptyArray, emptyArray, emptyArray);
var unknownUnionType = strictNullChecks ? getUnionType([undefinedType, nullType, unknownEmptyObjectType]) : unknownType;

var keyofConstraintObjectType = createAnonymousType(/*symbol*/ undefined, emptySymbols, emptyArray, emptyArray, [stringType, numberType, esSymbolType].map(t => createIndexInfo(t, unknownType, /*isReadonly*/ false))); // { [k: string | number | symbol]: unknown; }

var emptyGenericType = createAnonymousType(/*symbol*/ undefined, emptySymbols, emptyArray, emptyArray, emptyArray) as ObjectType as GenericType;
emptyGenericType.instantiations = new Map<string, TypeReference>();

Expand Down Expand Up @@ -13667,21 +13669,15 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
return instantiateType(instantiable, createTypeMapper([type.indexType, type.objectType], [getNumberLiteralType(0), createTupleType([replacement])]));
}

// If the original mapped type had an intersection constraint we extract its components,
// and we make an attempt to do so even if the intersection has been reduced to a union.
// This entire process allows us to possibly retrieve the filtering type literals.
// e.g. { [K in keyof U & ("a" | "b") ] } -> "a" | "b"
function getLimitedConstraint(type: ReverseMappedType) {
// If the original mapped type had an union/intersection constraint
// there is a chance that it includes an intersection that could limit what members are allowed
function getReverseMappedTypeMembersLimitingConstraint(type: ReverseMappedType) {
const constraint = getConstraintTypeFromMappedType(type.mappedType);
if (!(constraint.flags & TypeFlags.Union || constraint.flags & TypeFlags.Intersection)) {
return;
}
const origin = (constraint.flags & TypeFlags.Union) ? (constraint as UnionType).origin : (constraint as IntersectionType);
if (!origin || !(origin.flags & TypeFlags.Intersection)) {
if (constraint === type.constraintType) {
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, getIntersectionType([type.constraintType.type, keyofConstraintObjectType]));
Copy link
Contributor Author

@Andarist Andarist Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@weswigham weswigham Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Andarist Andarist Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yes! great idea with that intersection :) I pushed out this change

return getBaseConstraintOrType(instantiateType(constraint, mapper));
Copy link
Member

@weswigham weswigham Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a fan of getBaseConstraintOrType - it is almost never the right operation for the job.... why is it even needed here?

Copy link
Contributor Author

@Andarist Andarist Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@Andarist Andarist Jan 11, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }]

}

function resolveReverseMappedTypeMembers(type: ReverseMappedType) {
Expand All @@ -13691,14 +13687,21 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
const optionalMask = modifiers & MappedTypeModifiers.IncludeOptional ? 0 : SymbolFlags.Optional;
const indexInfos = indexInfo ? [createIndexInfo(stringType, inferReverseMappedType(indexInfo.type, type.mappedType, type.constraintType), readonlyMask && indexInfo.isReadonly)] : emptyArray;
const members = createSymbolTable();
const limitedConstraint = getLimitedConstraint(type);
const membersLimitingConstraint = getReverseMappedTypeMembersLimitingConstraint(type);
for (const prop of getPropertiesOfType(type.source)) {
// In case of a reverse mapped type with an intersection constraint, if we were able to
// extract the filtering type literals we skip those properties that are not assignable to them,
// because the extra properties wouldn't get through the application of the mapped type anyway
if (limitedConstraint) {
// we skip those properties that are not assignable to the limiting constraint
// the extra properties wouldn't get through the application of the mapped type anyway
// and their inferred type might not satisfy the type parameter's constraint
// which, in turn, could fail the check if the inferred type is assignable to its constraint
//
// inferring `{ a: number; b: string }` wouldn't satisfy T's constraint so b has to be skipped here
//
// declare function fn<T extends Record<string, number>>(arg: { [K in keyof T & "a"]: T[K] }): T
// const obj = { a: 1, b: '2' };
// fn(obj);
if (membersLimitingConstraint) {
const propertyNameType = getLiteralTypeFromProperty(prop, TypeFlags.StringOrNumberLiteralOrUnique);
if (!isTypeAssignableTo(propertyNameType, limitedConstraint)) {
if (!isTypeAssignableTo(propertyNameType, membersLimitingConstraint)) {
continue;
}
}
Expand Down Expand Up @@ -25749,9 +25752,9 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
}

function inferToMappedType(source: Type, target: MappedType, constraintType: Type): boolean {
if ((constraintType.flags & TypeFlags.Union) || (constraintType.flags & TypeFlags.Intersection)) {
if (constraintType.flags & TypeFlags.UnionOrIntersection) {
let result = false;
for (const type of (constraintType as (UnionType | IntersectionType)).types) {
for (const type of (constraintType as UnionOrIntersectionType).types) {
result = inferToMappedType(source, target, type) || result;
}
return result;
Expand Down
Loading