Fixed an issue with apparent mapped type keys #57838
Conversation
typescript-bot
added
the
For Uncommitted Bug
src/compiler/checker.ts
Outdated
| t => void mappedKeys.push(instantiateType(nameType, appendTypeMapping(targetType.mapper, getTypeParameterFromMappedType(targetType), t))), | ||
| ); | ||
| return getUnionType(mappedKeys); | ||
| return mappedKeys.length ? getUnionType(mappedKeys) : stringNumberSymbolType; |
There was a problem hiding this comment.
The problem here is that when modifiersType is unknown (or {}) then keyof modifiersType is never. Since never is assignable to everything, the checker assumed that keyof { [P in keyof T as T[P] extends string ? P: never]: any } is assignable to string.
Then based on that information it managed to simplify the instantiated substitution type of Extract to just keyof { [P in keyof T as T[P] extends string ? P: never]: any } (its newBaseType). Then in turn getNarrowableTypeForReference concluded that it should substitute constraints. Those were substituted with stringNumberSymbolType (for the keyof ...) and that's not assignable to string.
Before #56742 , it didn't mistakenly assume that keyof { [P in keyof T as T[P] extends string ? P: never]: any } is assignable to string so the substitution type was instantiated as (keyof { [P in keyof T as T[P] extends string ? P: never]: any }) & string and that is assignable to a string.
…ype-keys-with-unknown-modifier
|
Hi! I appreciate that reviewers can be very busy but I discovered this PR when bisecting my own issue with a bit of an absurd reproduction that was erroring with a much more opaque error "Type instantiation is excessively deep and possibly infinite." so I was unable to find any workarounds before the process of bisecting. I'd really like to see this PR merged because when I built it locally it makes my code work! I wouldn't want to ask this of anyone else without being willing to do it myself so I did also review it and I can't find anything objectionable, though of course my opinion doesn't weigh as much given I'm not a maintainer. |
…ype-keys-with-unknown-modifier
|
@typescript-bot pack this |
|
Hey @gabritto, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@typescript-bot test it |
|
@gabritto Here are the results of running the user tests with tsc comparing Everything looks good! |
|
Hey @gabritto, the results of running the DT tests are ready. Everything looks the same! |
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gabritto Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
Andarist
force-pushed
the
fix/apparent-mapped-type-keys-with-unknown-modifier
branch
from
2f2b500 to
cad532a
Compare
…ype-keys-with-unknown-modifier
Andarist
force-pushed
the
fix/apparent-mapped-type-keys-with-unknown-modifier
branch
from
cf0355f to
21f4f7c
Compare
|
@typescript-bot test it |
|
Hey @gabritto, the results of running the DT tests are ready. Everything looks the same! |
|
@gabritto Here are the results of running the user tests with tsc comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Everything looks good! |
|
@gabritto Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@gabritto Here are the results of running the top 400 repos with tsc comparing Everything looks good! |
src/compiler/checker.ts
Outdated
| ); | ||
| return getUnionType(mappedKeys); | ||
| const apparentKeys = getUnionType(mappedKeys); | ||
| if (forSource && apparentKeys.flags & TypeFlags.Never) { |
There was a problem hiding this comment.
Sorry if I wasn't completely clear, I think moving this check to this function allows us to simply check if modifiersType is unknown, and then we return string | number | symbol instead of never. To me this would make more clear where the special case is coming from and when it should be applied. But maybe this should actually be applied whenever never is there result, regardless of what the modifiers type was? That's what I'm confused about still.
What I also find confusing about the existing use of getApparentMappedTypeKeys is that, if we're using it to compare a source type with a target type, I would imagine we want to be conservative and obtain the largest possible type the source type could be. But the logic of getApparentMappedTypeKeys seems to compute the smallest possible type the source type could be: if we have some generic mapped type M, its apparent type will be, say, M1, and M <: M1 (M``` is a subtype of M1). We then compute the keyof M1, but I think keyofis contravariant, and sokeyof M :> keyof M1, so we're effectively approximating keyof M by a subtype of it, whereas to be conservative on the side of a source type, we'd need to approximate keyof M by a supertype. @weswigham is my thinking wrong here?
If that all made sense, then this PR fixes the keyof approximation by inverting never (smallest type for a key) and string | number | symbol (largest type for a key), but then I still worry the cases in between could be wrong.
There was a problem hiding this comment.
You raise a good point here, I'll research this further and come back with some thoughts
There was a problem hiding this comment.
I think it's wrong:
type AcceptString<T extends string> = T;
type FilterByLiteralValue<T, V> = { [P in keyof T as T[P] extends V ? P : never]: any };
function test<T extends { a: 1; b: 2 }>(k: keyof FilterByLiteralValue<T, 1>) {
const s: string = k; // errors correctly
type Result = AcceptString<typeof k> // doesn't error but it should
}So yes, you are right - obtaining apparent keys on the source side isn't the right approach at all.
…ype-keys-with-unknown-modifier
Andarist
force-pushed
the
fix/apparent-mapped-type-keys-with-unknown-modifier
branch
from
7c57147 to
73d96e1
Compare
5 participants
fixes #57827