Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
Include all properties from the mapped modifier type when calculating… #41976
+177
−1
Conversation
… index types for mapped types with name types
|
@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" |
andrewbranch
Jan 13, 2021
Member
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?
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?
andrewbranch
Jan 13, 2021
Member
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"
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" 
weswigham
Jan 13, 2021
•
Author
Member
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!
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!
andrewbranch
Jan 14, 2021
Member
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 }?
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 }?
weswigham
Jan 14, 2021
Author
Member
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.
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.
andrewbranch
Jan 14, 2021
Member
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?
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?
weswigham
Jan 27, 2021
Author
Member
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.
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.
weswigham
Jan 27, 2021
•
Author
Member
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.
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.
weswigham
merged commit 203a5ce
into
microsoft:master
9 of 10 checks passed
9 of 10 checks passed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
… 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