-
Notifications
You must be signed in to change notification settings - Fork 12.8k
Infer instance type arguments as constraint #49863
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
Infer instance type arguments as constraint #49863
Conversation
The TypeScript team hasn't accepted the linked issue #17473. If you can get it accepted, this PR will have a better chance of being reviewed. |
957043a
to
0af9933
Compare
@typescript-bot pack this |
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 0af9933. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 0af9933. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 0af9933. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 0af9933. You can monitor the build here. Update: The results are in! |
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at 0af9933. You can monitor the build here. Update: The results are in! |
Heya @DanielRosenwasser, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
@DanielRosenwasser |
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 |
@DanielRosenwasser Here they are:
CompilerComparison Report - main..49863
System
Hosts
Scenarios
TSServerComparison Report - main..49863
System
Hosts
Scenarios
Developer Information: |
Here's a playground link that contains both code samples from the description, for convenience: 4.8.0-pr-49863-9 |
I've been thinking more about this issue, specifically in the context of type parameter variance when a custom For
We also then need to decide what happens in the absence of those annotations What does everyone think? |
This should be declare class HasAction<T> {
readonly fn: (x: T) => void;
} then you want the most restrictive instantiation of |
Ah,
And then for |
In this latest commit, I've updated the behavior such that the presence of an
|
e58a84d
to
b74922b
Compare
Rebased, but the |
f02417c
to
e10367e
Compare
@microsoft-github-policy-service agree company="Bloomberg" |
Seems like the CLA bot is broken 😢 EDIT: Or not? The |
e10367e
to
5f51450
Compare
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.
Honestly unsure if this should be its own flag - this looks a lot like an implicit any
we're removing to me, so I could see it as a justifiable breaking change under the noImplicitAny
flag. Depends on how breaky we wanna be, which I guess we'd have to discuss at a design meeting @DanielRosenwasser .
What about instantiation expressions? I think |
@typescript-bot pack this |
Heya @DanielRosenwasser, I've started to run the parallelized Definitely Typed test suite on this PR at 0106a18. You can monitor the build here. Update: The results are in! |
Heya @DanielRosenwasser, I've started to run the extended test suite on this PR at 0106a18. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the tarball bundle task on this PR at 0106a18. You can monitor the build here. |
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite (tsserver) on this PR at 0106a18. You can monitor the build here. Update: The results are in! |
Heya @DanielRosenwasser, I've started to run the perf test suite on this PR at 0106a18. You can monitor the build here. Update: The results are in! |
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite on this PR at 0106a18. You can monitor the build here. Update: The results are in! |
Heya @DanielRosenwasser, I've started to run the diff-based top-repos suite (tsserver) on this PR at 0106a18. You can monitor the build here. Update: The results are in! |
Heya @DanielRosenwasser, I've started to run the diff-based user code test suite on this PR at 0106a18. You can monitor the build here. Update: The results are in! |
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 |
@DanielRosenwasser Here are the results of running the user test suite comparing Everything looks good! |
1 similar comment
@DanielRosenwasser Here are the results of running the user test suite comparing Everything looks good! |
Heya @DanielRosenwasser, I've run the RWC suite on this PR - assuming you're on the TS core team, you can view the resulting diff here. |
@DanielRosenwasser Here they are:
CompilerComparison Report - main..49863
System
Hosts
Scenarios
TSServerComparison Report - main..49863
System
Hosts
Scenarios
StartupComparison Report - main..49863
System
Hosts
Scenarios
Developer Information: |
Hey @DanielRosenwasser, the results of running the DT tests are ready. Branch only errors:Package: regenerator-runtime
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. Details
|
@DanielRosenwasser Here are the results of running the top-repos suite comparing Everything looks good! |
Following up on this. I saw some of the failures from the top-repos tests, but not sure how those should be interpreted. Most of the errors feel like correctly replacing a loose If there's any conclusion that can be drawn from these results, or any further tests to be run, let me know if I can push this PR along in any way. Thanks! |
Tested this out some more, and there might be a potential issue when a type parameter constraint refers to some other local type parameters (playground link)
The new type of the class after the instanceof check is I'll treat this as a bug for now and attempt to implement a solution. |
@molisani @DanielRosenwasser @jakebailey It looks like this PR needs some fixes before it's ready. Do you want to keep working on it? Otherwise I like to close stale PRs after a few months. |
It does need a few changes to make it ready. This slipped between the cracks and I should be able to get it back into shape soon. |
@molisani, have you had a chance to come back to this PR? |
I haven't, but I'm still optimistic that I'll be able to tackle this at some point. For now I'll close it out and if I can pick it up again I'll open a new PR with everything refreshed. |
For anyone subscribed to this PR, I have recreated it with my latest comments resolved here: #58028 |
In JavaScript, there are several runtime checks that can determine if an object is an instance of a specific class. However, due to the fact that TypeScript type parameters only exist at the type level, there are limited options for how to handle these when building the class instance reference type. At the moment, there are two different approaches used by the two different kind of checks: one defaults to
any
and the other re-uses the type argument from the enclosing class. The first could be stricter and the second is possibly too strict in some cases.The proposed change is to modify the way TypeScript handles these checks such that the type arguments are inferred as the constraint from the corresponding type parameter. This would default to
unknown
if no constraint was specified as that is the "default constraint".Fixes #17473 - Infer constrained generic parameters after instanceof check
For
instanceof
checks, TypeScript mirrors the underlying behavior of JavaScript and looks at theClass.prototype
property. This property is computed dynamically in the checker and is then used to narrow the target of theinstanceof
expression. If the class in question has any type parameters, TypeScript returns a type reference where all of the type arguments areany
. This is documented as being part of the "TypeScript 1.0 spec" so it is very long-standing behavior. The proposed change in behavior would constitute a breaking change as it would narrow some types fromany
to the constraint type orunknown
.Fixes #46668 - Private field check narrows generic class too far
For
#brand in obj
checks, TypeScript can infer that if an object has a private identifier then it must be an instance of the enclosing class. If the class in question has any type parameters, TypeScript sets all of the type arguments of the inferred type to match the type arguments ofthis
(see thread for potential issues). This behavior was only introduced recently via ergonomic brand checks for private fields. The proposed change in behavior is technically a breaking change, but less so than theinstanceof
check as in some cases it would loosen type arguments to match their constraint.Breaking Changes
In order to deal with these breaking changes, this change should be gated behind a new strict compiler option
inferInstanceTypeArgumentsAsConstraint
. This would be similar to theuseUnknownInCatchVariables
option that narrowed catch variables fromany
tounknown
(#41013).For individual instances of this issue, it should be possible to re-widen the narrowed type with a cast:
Addendum on Name: I am aware that this new compiler option name is very verbose, and could potentially be improved. I considered something that referenced "runtime instance checks" but it's hard to include that verbiage in the name and not make it potentially confusing. (ex: Would someone see
strictRuntimeInstanceChecks
and then assume that it somehow changes runtime behavior?) Also considereduseConstraintAsDefaultInstanceTypeArgument
orstrictDefaultInstanceTypeArguments
.