Improve apparent type of mapped types #57122
Conversation
|
@typescript-bot test top100 |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 14724a0. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 14724a0. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 14724a0. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the faster perf test suite on this PR at 14724a0. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @ahejlsberg, the results of running the DT tests are ready. |
|
@ahejlsberg Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
src/compiler/checker.ts
Outdated
| const target = (type.target ?? type) as MappedType; | ||
| const typeVariable = getHomomorphicTypeVariable(target); | ||
| if (typeVariable && !target.declaration.nameType) { | ||
| const constraint = getConstraintTypeFromMappedType(type); |
There was a problem hiding this comment.
All of this makes sense now... it is tricky that mapped types don't have constraints resolvable by getBaseConstraintOfType and such but we can reach for getConstraintTypeFromMappedType and use that.
src/compiler/checker.ts
Outdated
| const typeVariable = getHomomorphicTypeVariable(target); | ||
| if (typeVariable && !target.declaration.nameType) { | ||
| const constraint = getConstraintTypeFromMappedType(type); | ||
| if (constraint.flags & TypeFlags.Index) { |
There was a problem hiding this comment.
I wonder if this check shouldn't be replaced with an assert - it's quite a strong invariant that this branch can only be entered after getHomomorphicTypeVariable returns true and that requires the constraint to be an Index type.
There was a problem hiding this comment.
An assert here wouldn't be right. There many cases where an instantiation of a homomorphic mapped type doesn't have a keyof XXX constraint anymore. For example, the homomorphic Readonly<T> has the constraint type keyof T, but the instantiation Readonly<{ a: string, b: string }> has the constraint type "a" | "b". What we're looking for here are instantiations where the constraint type is still a keyof XXX. That's what wasn't handled before.
There was a problem hiding this comment.
Thank you for the explanation - it makes sense, getHomomorphicTypeVariable is called now with the target and not the type after all.
|
For what it's worth - I spent some sleepless nights on this last week so I built up some good context around this issue. LGTM (pending the resolution of #50034 ). |
|
@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 |
|
|
||
| type Curry<F extends ((...args: any) => any)> = | ||
| <T extends any[]>(...args: Tools.Cast<Tools.Cast<T, Gaps<Parameters<F>>>, any[]>) => | ||
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ |
There was a problem hiding this comment.
How is the change in the PR causing this error?
There was a problem hiding this comment.
I believe the underlying issue is the same as it was in my PRs. This is now resolved through layers and thus getResolvedApparentTypeOfMappedType sees the constraint of Parameters<F> - that once gets computed as unknown[] (by getDefaultConstraintOfConditionalType when restrictive instantiation bails out from getConstraintOfDistributiveConditionalType) and once as any (by getConstraintOfDistributiveConditionalType). So any can get returned and getResolvedApparentTypeOfMappedType can't recognize this as an array-like. So it ends up not d about it not being assignable to readonly any[] (that is being checked to determine if the type can be used as rest).
IIRC the other case (when unknown[]) is returned makes any[] (added by this Cast) eliminated from the substitution type since, at that point, it's determined that the baseType satisfies the added constraint - so the baseType gets returned.
There was a problem hiding this comment.
So the issue is mainly in the fact that previously the substitution type wasn't simplified at all here and that & any[] in it (coming from its added constraint) was preserved and made the comparison against readonly any[] OK.
There was a problem hiding this comment.
What do you mean by "this is now resolved through layers"?
How do we go from a call to getResolvedApparentTypeOfMappedType to calls to getDefaultConstraintOfConditionalType and getConstraintOfDistributiveConditionalType?
There was a problem hiding this comment.
Please note that I'm partially speaking based on what I remember about debugging this last week - so some details might be slightly off.
Gaps is a mapped type - so we enter getResolvedApparentTypeOfMappedType. With this PR we recognize that an instantiation of a homomorphic mapped type is received and we end up checking its constraint and if it's an index type. It turns out it is because it's something like keyof Parameters<...>. So we grab the .type and compute its base constraint. So now we are computing the constraint of a conditional type (through getConstraintFromConditionalType) and, as mentioned above, this once resolves through getDefaultConstraintOfConditionalType (for the restrictive instantiation case) and once through getConstraintOfDistributiveConditionalType (for a regular case :P).
There was a problem hiding this comment.
With latest commit this error is gone.
|
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 |
|
Hey @ahejlsberg, the results of running the DT tests are ready. Branch only errors:Package: lodash |
|
@ahejlsberg Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
The test runs reveal inconsistencies related to indexing arrays with numeric string literals (as in #50034): function bar<T extends string[], K extends number>() {
type T00 = string[]["0"]; // Ok
type T01 = string[]["0.0"]; // Error, as expected
type T02 = string[][K | "0"]; // Error, but shouldn't be
type T10 = T["0"]; // Error, but shouldn't be
type T11 = T["0.0"]; // Error, as expected
type T12 = T[K | "0"]; // Error, but shouldn't be
}The errors on |
typescript-bot
added
For Milestone Bug
|
@typescript-bot test top100 |
|
Heya @ahejlsberg, I've started to run the diff-based top-repos suite on this PR at 633bfc3. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 633bfc3. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the diff-based user code test suite on this PR at 633bfc3. You can monitor the build here. Update: The results are in! |
|
Heya @ahejlsberg, I've started to run the faster perf test suite on this PR at 633bfc3. You can monitor the build here. Update: The results are in! |
|
@ahejlsberg Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@ahejlsberg Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @ahejlsberg, the results of running the DT tests are ready. |
| function bar<T extends string[], K extends number>() { | ||
| type T00 = string[]["0"]; | ||
| type T01 = string[]["0.0"]; // Error | ||
| type T02 = string[][K | "0"]; | ||
| type T10 = T["0"]; | ||
| type T11 = T["0.0"]; // Error | ||
| type T12 = T[K | "0"]; | ||
| } |
There was a problem hiding this comment.
nit: those should likely be in a different test file since they don't have anything in common with this "test title" (assignmentToAnyArrayRestParameters)
|
@ahejlsberg Here are the results of running the top-repos suite comparing Everything looks good! |
|
Test and performance suites are all clean. This is ready to merge. |
This PR supersedes #56727 (which was reverted). The improvements in this PR don't exhibit the issue that caused us to revert #56727.
EDIT:
However, the PR does break one test because the improved resolution of apparent types for mapped types causes the issue reported in #29919 to surface in that test.I'm going to recommend we merge #50034, which fixes #29919 and thus the broken test in this PR.Fixes #29919.
Fixes #56726.