Skip to content
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

Fixed crash related to index type deferral on generic mapped types with name types #60528

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Andarist
Copy link
Contributor

fixes #60476

@typescript-bot typescript-bot added the For Uncommitted Bug PR for untriaged, rejected, closed or missing bug label Nov 17, 2024
Comment on lines +67 to +68
!!! error TS2322: Type 'Mapped6<K>[`_${K}`]' is not assignable to type '`_${string}`'.
!!! error TS2322: Type 'Mapped6<K>[`_${string}`]' is not assignable to type '`_${string}`'.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe this change is a fix. The new error matches what was reported by TS 5.0: TS playground

Comment on lines +49 to +55
type Mapped7<K extends string> = {
[P in K as [P] extends [`_${string}`] ? P : never]: P;
};

function f7<K extends string>(obj: Mapped7<K>, key: keyof Mapped7<K>) {
let s: `_${string}` = obj[key];
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test case is an extra thing that gets fixed here. This is based on Mapped5+f5 from this file. The only difference between them is that this one is using [P] extends [...] and not P extends ....

This didn't work because the index type deferral was previously dependent on hasDistributiveNameType. I don't quite see a reason why those 2 would type check differently though. Effectively P is always instantiated with a non-union type.

Comment on lines -42089 to -42092
// skip index type deferral on remapping mapped types
const objectIndexType = isGenericMappedType(objectType) && getMappedTypeNameTypeKind(objectType) === MappedTypeNameTypeKind.Remapping
? getIndexTypeForMappedType(objectType, IndexFlags.None)
: getIndexType(objectType, IndexFlags.None);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This reverts my own change from #55140 . I think now the check wasn't exhaustive anyway and this is now better handled by getSimplifiedIndexType

@Andarist
Copy link
Contributor Author

@jakebailey could u run the extended test suite here? :)

@@ -18350,7 +18356,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker {
function shouldDeferIndexType(type: Type, indexFlags = IndexFlags.None) {
return !!(type.flags & TypeFlags.InstantiableNonPrimitive ||
isGenericTupleType(type) ||
isGenericMappedType(type) && (!hasDistributiveNameType(type) || getMappedTypeNameTypeKind(type) === MappedTypeNameTypeKind.Remapping) ||
isGenericMappedType(type) && getNameTypeFromMappedType(type) ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

As mentioned in the comment here:

Effectively P is always instantiated with a non-union type.

Because of that, it's not safe to skip index type deferral for any remapping mapped type as P could be used multiple times by the mapping (and the compiler shouldn't allow for a cross-product of `${P}_${P}`). Checking getMappedTypeNameTypeKind can run into an infinite loop here and it seems the easiest to just avoid checking that and assume that all generic mapped types with name types have to have deferred index types. I compensate for that by simplifying them in relationship checking etc.

// want to perform the reduction when the name type of a mapped type is distributive with respect to the type variable
// introduced by the 'in' clause of the mapped type. Note that non-generic types are considered to be distributive because
// they're the same type regardless of what's being distributed over.
function hasDistributiveNameType(mappedType: MappedType) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternatively, maybe this could be kept but repurposed slightly. It would have to check if used template literal types don't refer to the type variable introduced by 'in' clause more than once. I don't think this is something the compiler would usually do in any other place so I'm hesitant to say that this would be a better solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Uncommitted Bug PR for untriaged, rejected, closed or missing bug
Projects
Status: Not started
2 participants