Add numeric constraints to type parameters of mapped types with narrowed down array constraints #55386
Conversation
…wed down array constraints
typescript-bot
added
the
For Uncommitted Bug
…-array-constraint-from-substitution
typescript-bot
added
For Backlog Bug
|
@typescript-bot test this |
|
Heya @weswigham, I've started to run the regular perf test suite on this PR at c741755. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the diff-based top-repos suite on this PR at c741755. You can monitor the build here. Update: The results are in! |
|
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at c741755. You can monitor the build here. Update: The results are in! |
|
@weswigham Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
|
@jakebailey could you build a playground for this one? |
|
Hey @weswigham, the results of running the DT tests are ready. |
|
@typescript-bot pack this |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at c741755. You can monitor the build here. |
|
Hey @jakebailey, 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 |
|
Got the repro case from Vue here. I will have to find time to investigate what exactly is happening here and how this could be remedied. |
|
@Andarist it sounds like this PR isn't ready to merge until after it works for Vue. Did I read the history right? |
|
Yes, I'll convert this to a temporary draft and ping you back when I manage to undraft it. I didn't yet have time to investigate this break. |
…-array-constraint-from-substitution
…-array-constraint-from-substitution
|
The above change is "correct" (well, it's consistent...). This is just a new situation in which this thing can happen today: #59260 |
|
@typescript-bot test it (been a bit) |
|
@jakebailey Here are the results of running the user tests with tsc comparing Everything looks good! |
|
Hey @jakebailey, the results of running the DT tests are ready. Everything looks the same! |
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
|
The above error is the same as was reported before. I think this code was just accidentally allowed in Vue types, see my previous comment here |
src/compiler/checker.ts
Outdated
| @@ -16633,7 +16633,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
| if (getTypeParameterFromMappedType(mappedType) === getActualTypeVariable(type)) { | |||
| const typeParameter = getHomomorphicTypeVariable(mappedType); | |||
| if (typeParameter) { | |||
| const constraint = getConstraintOfTypeParameter(typeParameter); | |||
| const constraint = getConstraintOfTypeParameter(getConditionalFlowTypeOfType(typeParameter, parent)); | |||
There was a problem hiding this comment.
I will note that this call is at least a bit cursed; getConditionalFlowTypeOfType returns Type, not TypeParameter.
There was a problem hiding this comment.
yeee, this is really cursed 😬 since the result of this function was accepted as a valid type to getConstraintOfTypeParameter... I didn't really recheck what's happening here.
I'm changing this to a draft as I also noticed now that it needs extra consideration as to how it interacts with getResolvedApparentTypeOfMappedType
There was a problem hiding this comment.
I'm not yet sure what to do about it, especially in light of #57801 that only treats mapped types ar arrays/tuples when every constituent of an intersection is array/tuple.
In this situation the constraint of the substitution type created by getConditionalFlowTypeOfType is smth like Schema & SchemaArray (so (SchemaArray | SchemaObject | LiteralType) & SchemaArray). The user's intention is clearly to treat the narrowed-down type as an array/tuple but this intersection type doesn't quite guarantee us that. Even though often an intersection like this is meant to "filter" it isn't exactly that.
I'll have to think about this one more but in the meantime I realized that there is a bug here that can be fixed separately: #60084
…-array-constraint-from-substitution
fixes #55383