Allow type comparison when target is generic mapped type with key remapping #45700
Conversation
typescript-bot
added
the
For Milestone Bug
| // If the target type has shape `{ [P in Q as R]: T }`, then a property of the target has type `R`. | ||
| // If the target type has shape `{ [P in Q as R]?: T }`, then a property of the target has type `R`, | ||
| // but the property is optional, so we only want to compare properties `R` that are common between `keyof S` and `R`. | ||
| const indexingType = keysRemapped |
There was a problem hiding this comment.
I am not entirely sure this won't cause issues for the case where the target mapped type has an include optional modifier.
There was a problem hiding this comment.
So, to be clear here, in the keysRemapped case, we're skipping the getIntersectionType calls with the typeParameter, simply because the keys may or may not actually contain the remapping type parameter (eg, every key may just be mapped to string) - the targetKeys will already contain it if so. I think this is correct - it's just not necessarily clear. In the case where there's no as clause, we intersect the type parameter and the (filtered) keys so we get a generic property key - whereas when the keys are remapped, we don't need to do so as the remapping overrides that.
|
@typescript-bot perf test this |
|
Heya @andrewbranch, I'm starting to run the inline community code test suite on this PR at ca98987. Hold tight - I'll update this comment with the log link once the build has been queued. Update: The results are in! |
|
Heya @andrewbranch, I've started to run the parallelized Definitely Typed test suite on this PR at ca98987. You can monitor the build here. |
|
Heya @andrewbranch, I've started to run the perf test suite on this PR at ca98987. You can monitor the build here. Update: The results are in! |
|
|
||
| function fun2<T>(val: T) { | ||
| let x: FilterInclOpt<T> = val; // Ok | ||
| let y: ModifyInclOpt<T> = val; // Ok |
There was a problem hiding this comment.
My initial reaction to this one was that this is wrong because T might have some property that would conflict with the remapped property type, e.g. if T was instantiated with { foo: string, boolfoo: number }. However, it looks like that’s an unsoundness that already exists independent of mapped types entirely:
interface Foo {
x?: string;
y?: string;
z?: string;
}
function f<T>(x: T) { // What if `T` is `{ x: number }`?
const _: Foo = x;
}There was a problem hiding this comment.
Yeah, I tried to follow the already existing logic and reuse everything that's already there, so there might be inconsistencies that were already there or that result from the kind of types that are in key remapping clauses.
There was a problem hiding this comment.
Optional properties are a pretty big soundness hole, unfortunately. The unconstrained generic one is actually one I have a PR up to remove - #33570
There was a problem hiding this comment.
This exact discussed case has changed behavior in 51b346d#diff-25f367dc11e40edc9febb9d6f088e13f9c7c8086bb8b9f85aed321f0903bc46eR31-R33
One of my PRs bring back original behavior (somewhat accidentally, I wasn't aiming for it), see here
Which one is correct? 😅
|
@andrewbranch |
|
@andrewbranch Here they are:Comparison Report - main..45700
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| const indexingType = keysRemapped | ||
| ? (filteredByApplicability || targetKeys) | ||
| : filteredByApplicability | ||
| ? getIntersectionType([filteredByApplicability, typeParameter]) | ||
| : typeParameter; |
There was a problem hiding this comment.
| const indexingType = keysRemapped | |
| ? (filteredByApplicability || targetKeys) | |
| : filteredByApplicability | |
| ? getIntersectionType([filteredByApplicability, typeParameter]) | |
| : typeParameter; | |
| const indexingType = keysRemapped ? (filteredByApplicability || targetKeys) | |
| : filteredByApplicability ? getIntersectionType([filteredByApplicability, typeParameter]) | |
| : typeParameter; |
| // If the target type has shape `{ [P in Q as R]: T }`, then a property of the target has type `R`. | ||
| // If the target type has shape `{ [P in Q as R]?: T }`, then a property of the target has type `R`, | ||
| // but the property is optional, so we only want to compare properties `R` that are common between `keyof S` and `R`. | ||
| const indexingType = keysRemapped |
There was a problem hiding this comment.
So, to be clear here, in the keysRemapped case, we're skipping the getIntersectionType calls with the typeParameter, simply because the keys may or may not actually contain the remapping type parameter (eg, every key may just be mapped to string) - the targetKeys will already contain it if so. I think this is correct - it's just not necessarily clear. In the case where there's no as clause, we intersect the type parameter and the (filtered) keys so we get a generic property key - whereas when the keys are remapped, we don't need to do so as the remapping overrides that.
Fixes #45212