Include all properties from the mapped modifier type when calculating… #41976
Conversation
… index types for mapped types with name types
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot user test this |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 597a9c6. You can monitor the build here. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at 597a9c6. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the parallelized community code test suite on this PR at 597a9c6. You can monitor the build here. |
|
@typescript-bot pack this |
|
Heya @RyanCavanaugh, I've started to run the tarball bundle task on this PR at 597a9c6. You can monitor the build here. |
|
@weswigham Here they are:Comparison Report - master..41976
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
The user suite test run you requested has finished and failed. I've opened a PR with the baseline diff from master. |
|
DT and the user tests look OK, perf seems fine. |
| >WithIndexKey : string | number | ||
|
|
||
| type WithoutIndexKey = keyof WithoutIndex; // number <-- Expected: "foo" | "bar" | ||
| >WithoutIndexKey : number | "foo" | "bar" |
There was a problem hiding this comment.
Is this right? WithoutIndex appears, at least in quick info, not to have an index signature at all (not sure if it’s represented differently under the hood), so it seems like you shouldn’t be allowed to index into it with number?
There was a problem hiding this comment.
Slight simplification of the test case, FWIW:
type OmitIndex<T> = {
[K in keyof T as (string extends K ? never : K)]: T[K];
};
type WithIndex = { [x: string]: unknown, foo: "hello", bar: "world" };
type WithoutIndex = OmitIndex<WithIndex>; // { foo: "hello"; bar: "world"; } <-- OK
type WithIndexKey = keyof WithIndex;
type WithoutIndexKey = keyof WithoutIndex; // number <-- Expected: "foo" | "bar" There was a problem hiding this comment.
Arguable - the OmitIndex type is only omitting the string key, however, a string index signature adds both a string and number key to the keyof of the type, since a string index signature implies and allows indexing by number!
There was a problem hiding this comment.
That makes sense, but two follow up questions:
- Why doesn’t quick info show the number index signature in
WithoutIndex? - How should the type
{ [K in keyof { [key: string]: any }]: K }be understood? Why isn’t it{ [x: string]: string | number }or{ [x: string]: string & number }or{ [x: string]: string, [x: number]: number }?
There was a problem hiding this comment.
For 1: The keyof logic includes number in the key type, however mapped type mappings iterate over the set of properties/indexes in the original modifiersType directly if available (and homomorphic), leading to the discrepancy.
As for 2: We have no principled answer and our behaviors are arbitrarily chosen. I think I went over the inconsistency at a design meeting just after key mappings were introduced and we decided to stick with all the inconsistencies so we didn't break anyone.
There was a problem hiding this comment.
It feels troubling that
declare let withoutIndex: WithoutIndex;
withoutIndex[3];is an error even though keyof typeof withoutIndex includes number. Is there any way to observe the numberyness of the keys besides keyof?
There was a problem hiding this comment.
type WithIndex = { [x: string]: unknown, foo: "hello", bar: "world" };
type StringOrNumber = keyof WithIndex;is already how keyof already works, so, we allow
declare const val: WithIndex;
val[12];
val[null as any as number];The number-yness of string indexers is pretty visible everywhere it can be shown, IMO. I can't think of somewhere we actually forbid using a number on a string index signature... such a location would probably constitute a bug.
There was a problem hiding this comment.
is an error even though keyof typeof withoutIndex includes number.
You can blame this on how we construct mapped types. While the keyof result excludes string, mapped type construction actually doesn't use keyof at all for homomorphic mapped types (heh), and instead iterates over the slots visible on the modifiers type. If the string indexer has been filtered out it doesn't check "oh, well, maybe a number indexer is still appropriate given the filter applied to the modifier", it just drops it.
In any case, number is present in the keys is true both before and after this change, so I'm thinking probably doesn't have too much bearing on this change as-is? That could be a separate enhancement/change to how we create mapped types with as clauses.
… index types for mapped types with name types. Previously, when only inspecting the constraint, we'd miss the mapping for any properties which subtype reduced with the constraint. When we return the constraint as the index, this is fine, however when we perform a mapping over the keys, we really need to start with the full list of keys, before any subtype reduction takes place. By looking at the list of properties like this, we ensure we can include the result of their mapping in the resulting key type.
Fixes #41966