-
Notifications
You must be signed in to change notification settings - Fork 12.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Instantiate earlier inferred constraints in conditional types #57362
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
fixes #57286 (comment)
some historical related context can be found in #31455 and #42747