Improve relationship checking with keyof GenericMappedType as the source type
#56246
Conversation
…for their mapped types
typescript-bot
added
the
For Uncommitted Bug
src/compiler/checker.ts
Outdated
| @@ -17468,6 +17468,12 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| const modifiersType = getApparentType(getModifiersTypeFromMappedType(type)); // The 'T' in 'keyof T' | |||
| forEachMappedTypePropertyKeyTypeAndIndexSignatureKeyType(modifiersType, TypeFlags.StringOrNumberLiteralOrUnique, !!(indexFlags & IndexFlags.StringsOnly), addMemberForKeyType); | |||
| } | |||
| else if (isFilteringMappedType(type)) { | |||
| const modifiersIndex = getIndexType(getModifiersTypeFromMappedType(type), indexFlags, UnionReduction.None); | |||
There was a problem hiding this comment.
UnionReduction.None was added here to keep this assignment allowed.
I think that without it the union reduction was forgetting about 'str' since it's a subtype of string. string is a valid input K but it is meant to be filtered away by DistributiveNonIndex. So with the union reduction on, I have ended up removing 'str' entirely - breaking the test case.
There was a problem hiding this comment.
I'm somewhat unsure - but maybe the union reduction could still mess this up when another union gets created manually with that as one of its members? Maybe I should explore a different fix that would do the same~ instantiation but later on, when comparing some constraints?
There was a problem hiding this comment.
UnionReduction.None is, in some ways, a hack to make contextual typing work a little bit better (and avoid the perf overhead of subtype reduction we don't need to bother doing). The union members could, very easily, evaporate later on down the line.
IMO, it'd be better to leave this case as producing a generic keyof type (especially since this nameType could still just be filtering a homomorphic mapped type), and improve constraint-following logic inside the generic-keyof-over-mapped-type case in structuredTypeRelatedToWorker - specifically, we already have a generic-index-type-target case that does the right thing (tm) - that probably just needs to be brought up and situationally applied when the source is a generic index, too. Since our only special case for keyof on the source side right now is
else if (sourceFlags & TypeFlags.Index) {
if (result = isRelatedTo(keyofConstraintType, target, RecursionFlags.Source, reportErrors)) {
return result;
}
}
it could probably use some more specifics, since it's definitely true that generic keyofs (over mapped types) can have constraints more specific than string | number | symbol.
There was a problem hiding this comment.
Thank you for the advice! I now reused the mentioned logic for keyof GenericMappedType as the target type so now the whole fix is within structuredTypeRelatedToWorker
There was a problem hiding this comment.
Hah, I never linked it here, but after I looked over this, I was assigned another issue that tracked back to the same thing, so I ended up writing #56742, which basically scooped this change. Do you want to sync this and at least add the new test cases? In #56742 I used much more specific carve-outs of which keyof types this applies to than the very general check here (and actually use a slightly different source key type).
There was a problem hiding this comment.
The reason this wasn't fixed is that keyof { [K in keyof TActors as K & string]: { src: K; logic: TActors[K]; }; } wasn't treated as a deferred index type (although I'd argue that it clearly is deferred). So the logic added by #56742 couldn't kick in.
I recently changed that in #60528 and I think that essentially supersedes this PR so I just merged it in there now to keep the tests.
Andarist
changed the title
nameType of filtering mapped types when creating index for their mapped typesnameType of mapped types when creating their index types
typescript-bot
added
For Backlog Bug
src/compiler/checker.ts
Outdated
| @@ -17468,6 +17468,12 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| const modifiersType = getApparentType(getModifiersTypeFromMappedType(type)); // The 'T' in 'keyof T' | |||
| forEachMappedTypePropertyKeyTypeAndIndexSignatureKeyType(modifiersType, TypeFlags.StringOrNumberLiteralOrUnique, !!(indexFlags & IndexFlags.StringsOnly), addMemberForKeyType); | |||
| } | |||
| else if (isFilteringMappedType(type)) { | |||
| const modifiersIndex = getIndexType(getModifiersTypeFromMappedType(type), indexFlags, UnionReduction.None); | |||
There was a problem hiding this comment.
UnionReduction.None is, in some ways, a hack to make contextual typing work a little bit better (and avoid the perf overhead of subtype reduction we don't need to bother doing). The union members could, very easily, evaporate later on down the line.
IMO, it'd be better to leave this case as producing a generic keyof type (especially since this nameType could still just be filtering a homomorphic mapped type), and improve constraint-following logic inside the generic-keyof-over-mapped-type case in structuredTypeRelatedToWorker - specifically, we already have a generic-index-type-target case that does the right thing (tm) - that probably just needs to be brought up and situationally applied when the source is a generic index, too. Since our only special case for keyof on the source side right now is
else if (sourceFlags & TypeFlags.Index) {
if (result = isRelatedTo(keyofConstraintType, target, RecursionFlags.Source, reportErrors)) {
return result;
}
}
it could probably use some more specifics, since it's definitely true that generic keyofs (over mapped types) can have constraints more specific than string | number | symbol.
Andarist
changed the title
nameType of mapped types when creating their index typeskeyof GenericMappedType as the source type
…-of-filtering-mapped-types-for-keyof
Andarist
force-pushed
the
instantiate-name-type-of-filtering-mapped-types-for-keyof
branch
from
c618aff
to
3d1fe73
Compare
|
Included in #60528 |
fixes #56239