Support custom 'Symbol.hasInstance' methods when narrowing 'instanceof'#55052
Support custom 'Symbol.hasInstance' methods when narrowing 'instanceof'#55052
Conversation
…wing 'instanceof'
|
Looks like you're introducing a change to the public API surface area. If this includes breaking changes, please document them on our wiki's API Breaking Changes page. Also, please make sure @DanielRosenwasser and @RyanCavanaugh are aware of the changes, just as a heads up. |
|
@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 |
|
@typescript-bot perf test |
|
Heya @rbuckton, I've started to run the diff-based user code test suite on this PR at 6a4f838. You can monitor the build here. Update: The results are in! |
|
Heya @rbuckton, I've started to run the perf test suite on this PR at 6a4f838. You can monitor the build here. Update: The results are in! |
|
Heya @rbuckton, I've started to run the parallelized Definitely Typed test suite on this PR at 6a4f838. You can monitor the build here. Update: The results are in! |
|
@rbuckton 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
|
|
@rbuckton Here they are:
CompilerComparison Report - main..55052
System
Hosts
Scenarios
TSServerComparison Report - main..55052
System
Hosts
Scenarios
StartupComparison Report - main..55052
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @rbuckton, the results of running the DT tests are ready. |
|
@typescript-bot pack this |
|
Hey @rbuckton, 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 |
|
Leaving a random comment to test the new perf infra on this PR. Update: The results are in! |
ahejlsberg
left a comment
There was a problem hiding this comment.
This seems a lot more complex than I had expected. I'm not quite understanding why we need synthetic method calls and potentially overload resolution to figure this out.
|
The most recent commits address feedback from an offline discussion with @ahejlsberg. @ahejlsberg, can you take another look? |
ahejlsberg
left a comment
There was a problem hiding this comment.
Suggest simplifying the error reporting per comments, otherwise looks good.
|
@typescript-bot cherry-pick this to release-5.3 |
|
Hey @rbuckton, I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-5.3 manually. |
Component commits: 69f59da [WIP] Support custom 'Symbol.hasInstance' methods when checking/narrowing 'instanceof' 161d1f1 Add tests for instanceof and narrowing 6a4f838 Accept baseline, fix lint af6bb55 Check derived types when using type predicates with instanceof/hasInstance 6a83254 Small tweaks, lint fixes, and baseline updates 511c955 Add go-to-definition support on 'instanceof' keyword 9e3adb4 Merge branch 'main' into instanceof-Symbol.hasInstance 4b0eafc Fix format befa293 Address PR feedback 559047e Comment cleanup 8af777e Switch synthetic call to use use 'resolveSignature' flow f3e94f0 Merge branch 'main' into instanceof-Symbol.hasInstance 1d90af1 Run formatting b65f9bc Merge branch 'main' into instanceof-Symbol.hasInstance 0554f56 Remove branch for 'instanceof' error message reporting
|
I have no idea why the pick failed; I was going to say that it failed a the push stage but it got pushed above: typescript-bot@97bf30b Probably some weird GH outage :| (I am working on replacing this pick task anyway.) |
|
It doesn't matter too much in this case. Daniel said he'd just update from |
This makes our
instanceofnarrowing more closely align to Steps 2-3 of the InstanceofOperator algorithm in the ECMA-262 specification by allowing any object type in the right-hand side ofinstanceofas long as it has a valid[Symbol.hasInstance]method. If such a method exists and returns a type predicate, that predicate can then be used when narrowinginstanceof.For a
[Symbol.hasInstance]method to be considered duringinstanceofnarrowing, it must have a type predicate as its return type.In addition, when the left-hand side of
instanceofis considered as an argument to[Symbol.hasInstance], it must be assignable to the first parameter. This does not, however, weaken our current requirement that the left-hand side be an object type (or a union containing an object type), despite the fact this is not currently a requirement of ECMAScript.This is intended to address rather complicated workarounds employed by, for example,
engine262which uses[Symbol.hasInstance]as a runtime type guard, but needed to introduce a non-creatable abstract class so that TypeScript would understand the type guard: https://github.com/engine262/engine262/blob/435e533f8bb2f66961e3c16fe67150a0bba745bb/src/completion.mts#L70Fixes #17360
Fixes #39064
Related #32080
Related #52670
NOTE: This will need to wait for 5.3 at the earliest.