Allow assignability of non-empty object to generic mapped type#32071
Conversation
…hen deciding assignability
| const hasOptionalUnionKeys = modifiers & MappedTypeModifiers.IncludeOptional && targetConstraint.flags & TypeFlags.Union; | ||
| const filteredByApplicability = hasOptionalUnionKeys ? filterType(targetConstraint, t => !!isRelatedTo(t, sourceKeys)) : undefined; | ||
| const includeOptional = modifiers & MappedTypeModifiers.IncludeOptional; | ||
| const filteredByApplicability = includeOptional ? filterType(getLiteralTypeFromProperties(target, TypeFlags.StringOrNumberLiteralOrUnique), t => !!isRelatedTo(t, sourceKeys)) : undefined; |
There was a problem hiding this comment.
My expectation was that when a mapped type target was instantiated with some T with a constraint like { x: number }, either getConstraintTypeFromMappedType(target) or getIndexType(target) would essentially give me keyof { x: number }. But instead, they both give me a fairly unhelpful index type for a generic object.
So then I thought, “ah, maybe if I use the apparent type of the mapped type,” but in the test case I added, getApparentType(target) === target.
My true intent was just to get a plain object type for that instantiation of that generic mapped type, at which point I think I could just call isRelatedTo(source, plainObjectType).
There was a problem hiding this comment.
If the constraint is keyof U | "a" | "b", getLiteralTypeFromProperties is going to ignore the U-ness of the keyof U part, while the targetConstraint will keep it. We'd like to keep as a keyof (T extends Entity) is different from a keyof (U extends Entity) and needs to be recognized as such. Rather than the filtering using filterType to calculate filteredByApplicability as we're doing now, it's actually probably best to use intersectTypes(targetConstraint, sourceKeys) since the result is the overlapping keys (be they generic or not). Making the indexed access below with that intersection should then work dandily.
In fact, it seems like this is all the changes needed to fix the issue (from master):
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 64bf09d8b3..4476214e71 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -13200,8 +13200,8 @@ namespace ts {
if (!isGenericMappedType(source)) {
const targetConstraint = getConstraintTypeFromMappedType(target);
const sourceKeys = getIndexType(source, /*stringsOnly*/ undefined, /*noIndexSignatures*/ true);
- const hasOptionalUnionKeys = modifiers & MappedTypeModifiers.IncludeOptional && targetConstraint.flags & TypeFlags.Union;
- const filteredByApplicability = hasOptionalUnionKeys ? filterType(targetConstraint, t => !!isRelatedTo(t, sourceKeys)) : undefined;
+ const hasOptionalUnionKeys = modifiers & MappedTypeModifiers.IncludeOptional;
+ const filteredByApplicability = hasOptionalUnionKeys ? intersectTypes(targetConstraint, sourceKeys) : undefined;
// A source type T is related to a target type { [P in Q]: X } if Q is related to keyof T and T[Q] is related to X.
// A source type T is related to a target type { [P in Q]?: X } if some constituent Q' of Q is related to keyof T and T[Q'] is related to X.
if (hasOptionalUnionKeysThere was a problem hiding this comment.
Oh, fantastic, that’s so much better 🙌
| const hasOptionalUnionKeys = modifiers & MappedTypeModifiers.IncludeOptional && targetConstraint.flags & TypeFlags.Union; | ||
| const filteredByApplicability = hasOptionalUnionKeys ? filterType(targetConstraint, t => !!isRelatedTo(t, sourceKeys)) : undefined; | ||
| const includeOptional = modifiers & MappedTypeModifiers.IncludeOptional; | ||
| const filteredByApplicability = includeOptional ? filterType(getLiteralTypeFromProperties(target, TypeFlags.StringOrNumberLiteralOrUnique), t => !!isRelatedTo(t, sourceKeys)) : undefined; |
There was a problem hiding this comment.
If the constraint is keyof U | "a" | "b", getLiteralTypeFromProperties is going to ignore the U-ness of the keyof U part, while the targetConstraint will keep it. We'd like to keep as a keyof (T extends Entity) is different from a keyof (U extends Entity) and needs to be recognized as such. Rather than the filtering using filterType to calculate filteredByApplicability as we're doing now, it's actually probably best to use intersectTypes(targetConstraint, sourceKeys) since the result is the overlapping keys (be they generic or not). Making the indexed access below with that intersection should then work dandily.
In fact, it seems like this is all the changes needed to fix the issue (from master):
diff --git a/src/compiler/checker.ts b/src/compiler/checker.ts
index 64bf09d8b3..4476214e71 100644
--- a/src/compiler/checker.ts
+++ b/src/compiler/checker.ts
@@ -13200,8 +13200,8 @@ namespace ts {
if (!isGenericMappedType(source)) {
const targetConstraint = getConstraintTypeFromMappedType(target);
const sourceKeys = getIndexType(source, /*stringsOnly*/ undefined, /*noIndexSignatures*/ true);
- const hasOptionalUnionKeys = modifiers & MappedTypeModifiers.IncludeOptional && targetConstraint.flags & TypeFlags.Union;
- const filteredByApplicability = hasOptionalUnionKeys ? filterType(targetConstraint, t => !!isRelatedTo(t, sourceKeys)) : undefined;
+ const hasOptionalUnionKeys = modifiers & MappedTypeModifiers.IncludeOptional;
+ const filteredByApplicability = hasOptionalUnionKeys ? intersectTypes(targetConstraint, sourceKeys) : undefined;
// A source type T is related to a target type { [P in Q]: X } if Q is related to keyof T and T[Q] is related to X.
// A source type T is related to a target type { [P in Q]?: X } if some constituent Q' of Q is related to keyof T and T[Q'] is related to X.
if (hasOptionalUnionKeys|
@typescript-bot run dt |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 27b8c45. You can monitor the build here. It should now contribute to this PR's status checks. |
|
Heya @weswigham, I've started to run the extended test suite on this PR at 27b8c45. You can monitor the build here. It should now contribute to this PR's status checks. |
|
I think the RWC diff is the same as |
Fixes #31070
The particulars of how I’m doing this feel fairly wrong, but the results seem correct. Not sure why a couple error baselines elaborate way more, but wanted to publish this draft before looking at it to see how off-base this is 😬Apparently I can't mark this as a draft after the fact