Instantiate earlier inferred constraints in conditional types #57362
Conversation
|
@typescript-bot test top200 @typescript-bot perf test this faster |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 1148bce. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 1148bce. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the faster perf test suite on this PR at 1148bce. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 1148bce. You can monitor the build here. Update: The results are in! |
| // * The original `mapper` used to create this conditional | ||
| // * The mapper that maps the old root type parameter to the clone (`freshMapper`) | ||
| // * The mapper that maps the clone to its inference result (`context.mapper`) | ||
| const freshParams = sameMap(root.inferTypeParameters, maybeCloneTypeParameter); |
There was a problem hiding this comment.
This PR - in its current form - removes the context of those fresh params.
They were introduced in this PR to address a type parameter leak. From what I understand, the leak was caused by non-instantiated constraints of infer type parameters, and those fresh params were introduced to "wire up" the mapper so it would partake in the instantiation of those constraints. Those instantiations are made using context.nonFixingMapper so I figured out that maybe this can be simplified by combining that with the mapper.
The problem with maybeCloneTypeParameter that was used to create those freshParams is that the check inside it is non-exhaustive:
isGenericObjectType(constraint) || isGenericIndexType(constraint)When it comes to the added test case, this check doesn't cover Klass<V> - that's neither of the tested things. (when the inferred Klass<number> gets checked against that constraint it fails the assignability check and the inferred result gets replaced by the constraint which in turn is instantiated by the combinedMapper)
So the alternative fix would be to extend this check to cover more situations. That can't be done exhaustively and cheaply. The closest check to that is couldContainTypeVariables and that's not supposed to be used in situations like this, it's just an optimization.
So given the fact above, one extra alternative solution comes to mind... freshParams could always be created. I verified that it would fix the issue and that it wouldn't break any existing case.
|
@jakebailey 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
|
|
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @jakebailey, the results of running the DT tests are ready. |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
There was a problem hiding this comment.
IIRC, I added the maybeCloneTypeParameters bit back when it was supposed that the type identities produced here could be bad for perf. Well, we have much better perf tests now, and they look fine to me, so I think this is pretty fine. Now, there may be some edge case that it effects in an outsized way, but hopefully if/when that comes up, we can come up with something nicer to make that work out, because this has always been a bit off-putting to me.
|
@typescript-bot pack this |
|
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 1148bce. You can monitor the build here. |
|
Hey @DanielRosenwasser, 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 |
Andarist
deleted the
fully-instantiated-constraints-for-infer-type-params
branch
fixes #57286 (comment)
some historical related context can be found in #31455 and #42747