-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Distribute index over mapped types when normalizing types #55130
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.
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
base: main
Are you sure you want to change the base?
Distribute index over mapped types when normalizing types #55130
Conversation
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
src/compiler/checker.ts
Outdated
@@ -17633,6 +17633,13 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
// For example, for an index access { [P in K]: Box<T[P]> }[X], we construct the type Box<T[X]>. | |||
if (isGenericMappedType(objectType)) { | |||
if (!getNameTypeFromMappedType(objectType) || isFilteringMappedType(objectType)) { | |||
if (distributeIndexOverMappedType && !writing) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an attempt to fix this issue, a conversation starter to establish the proper fix 😉
The core of the issue is that with this code:
function run<T extends Table>(pointer: Pointer<T>) {
const x = something(pointer);
}
the indexed mapped type gets substituted and thus source
in isRelatedTo
quickly becomes { table: T; id: string; }
because T
isn't a union (so It's not handled by distributeIndexOverObjectType
), it just has a union constraint.
On the other hand, this version has no real semantic difference and yet it's handled correctly:
function run<T extends Table>(pointer: Pointer<T & Table>) {
const x = something(pointer);
}
This is because this added intersection creates a union here so distributeIndexOverObjectType
kicks in and we end up with: { table: T & "a"; id: string; } | { table: T & "b"; id: string; } | { table: T & "c"; id: string; }
.
And because this is a union, ' eachTypeRelatedToType' can relate this with Pointer<keyof TableToRecord>
(also a union). The broken case though can't be related through typeRelatedToSomeType
because its table
property is of type T
with a constraint of 'a' | 'b' | 'c'
where we have 3 targets with the table
property respectively set to 'a'
, 'b'
and 'c'
.
I've tried a couple of things, deferring this indexed access type, etc. It seems though that during inference we actually want to perform this substitution, we don't necessarily want to do it when relating types. So if this is meant to be fixed this function here has to conditionally do different things with those indexed mapped types - based on the usage type.
…-access-on-mapped-type
dddb105
to
35be879
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, not a huge fan of a conditional check like this in getSimplifiedType
- this may be a case where this should basically be a new relationship rule, since it's both only being done inside relationship checking, and only being done on the source side type. All that seems to indicate this equivalency would be better recognized as a new branch when comparing types. We might need to look at the pre-simplified types to make it happen, though.
@@ -6555,6 +6555,7 @@ export interface IndexedAccessType extends InstantiableType { | |||
constraint?: Type; | |||
simplifiedForReading?: Type; | |||
simplifiedForWriting?: Type; | |||
simplifiedForReadingD?: Type; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe D
-> Distributive
here, if this gets kept.
fixes #54892