Pr issue 57087 - fix for relating (e.g. satisfies) function overloads to intersection specifications #57395
Conversation
typescript-bot
added
the
For Uncommitted Bug
|
The TypeScript team hasn't accepted the linked issue #57087. If you can get it accepted, this PR will have a better chance of being reviewed. |
…then through typeRelatedToEachType for error messaging
craigphicks
changed the title
…edToIntersection.ts)
|
@typescript-bot test this |
|
Heya @RyanCavanaugh, I've started to run the tarball bundle task on this PR at 2d54894. You can monitor the build here. |
|
Heya @RyanCavanaugh, I've started to run the diff-based user code test suite (tsserver) on this PR at 2d54894. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the regular perf test suite on this PR at 2d54894. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the parallelized Definitely Typed test suite on this PR at 2d54894. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based top-repos suite (tsserver) on this PR at 2d54894. You can monitor the build here. Update: The results are in! |
|
Heya @RyanCavanaugh, I've started to run the diff-based user code test suite on this PR at 2d54894. You can monitor the build here. |
|
Hey @RyanCavanaugh, 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 |
|
@RyanCavanaugh Here are the results of running the user test suite comparing Everything looks good! |
|
@RyanCavanaugh Here they are:
tscComparison 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: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Hey @RyanCavanaugh, the results of running the DT tests are ready. |
|
@RyanCavanaugh Here are the results of running the top-repos suite comparing Something interesting changed - please have a look. DetailsServer exited prematurely with code unknown and signal SIGABRTAffected reposcalcom/cal.comRaw error text:RepoResults7/calcom.cal.com.rawError.txt in the artifact folder
Last few requests{"seq":760,"type":"request","command":"completionInfo","arguments":{"file":"@PROJECT_ROOT@/apps/storybook/postcss.config.js","line":11,"offset":30,"includeExternalModuleExports":false,"triggerKind":1}}
{"seq":761,"type":"request","command":"completionInfo","arguments":{"file":"@PROJECT_ROOT@/apps/storybook/postcss.config.js","line":11,"offset":39,"includeExternalModuleExports":false,"triggerKind":2,"triggerCharacter":" "}}
{"seq":762,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":["@PROJECT_ROOT@/apps/api/pages/api/webhooks/[id]/_patch.ts"],"openFiles":[]}}
{"seq":763,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/apps/swagger/lib/snippets.js","projectRootPath":"@PROJECT_ROOT@"}]}}
Repro steps
Server exited prematurely with code unknown and signal SIGABRTAffected reposbackstage/backstageRaw error text:RepoResults8/backstage.backstage.rawError.txt in the artifact folder
Last few requests{"seq":3321,"type":"request","command":"completionInfo","arguments":{"file":"@PROJECT_ROOT@/packages/create-app/src/lib/versions.ts","line":139,"offset":4,"includeExternalModuleExports":false,"triggerKind":1}}
{"seq":3322,"type":"request","command":"references","arguments":{"file":"@PROJECT_ROOT@/packages/create-app/src/lib/versions.ts","line":141,"offset":4}}
{"seq":3323,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":["@PROJECT_ROOT@/packages/core-plugin-api/src/apis/system/ApiRef.ts"],"openFiles":[]}}
{"seq":3324,"type":"request","command":"updateOpen","arguments":{"changedFiles":[],"closedFiles":[],"openFiles":[{"file":"@PROJECT_ROOT@/packages/create-app/templates/default-app/packages/app/e2e-tests/app.test.ts","projectRootPath":"@PROJECT_ROOT@"}]}}
Repro steps
|
|
@RyanCavanaugh - I guess this means I need to download the repos calcom/cal.com and/or backstate/backstage to confirm/recreate the problem - is that correct? |
|
Expand out the comment a few times and you'll get exact instructions on how to reproduce each. |
|
Re: the
This is the output from
In the non-failing case, looking at system memory usage, it rises nearly monotonically with time to increase by 4.4 GiB. I'll follow up by checking the other report too. |
|
Re: The
There are 4 Of those, differ from is the same. I have assumed that is the one to use.
|
|
@typescript-bot test top200 |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 2d54894. You can monitor the build here. Update: The results are in! |
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
@jakebailey - I am grateful and confused. I will look further into the SIGABRT examples. |
|
I would doubt that they are new in this PR; the server tests can be a little flaky. |
sandersn
requested review from
sandersn and
jakebailey
and removed request for
jakebailey
|
|
||
|
|
||
| /** | ||
| * The following function ft3 should fail. However, it currently does not |
There was a problem hiding this comment.
this is a half nit, but for comments in test files, I would avoid using language that includes "currently" or "new code", just because it'll end up out of date at some point. If there's a specific theory to these, then explaining them is alright, but the comments seem sorta speculative (like they should be review comment threads).
| ) { | ||
| if ( | ||
| source.flags & TypeFlags.Object && getSignaturesOfType(source, SignatureKind.Call).length > 1 | ||
| // && target.flags & TypeFlags.Object && getSignaturesOfType(target0, SignatureKind.Call).length > 1 // already known to be intersection type |
There was a problem hiding this comment.
This is commented out; I would not include commented out code unless it's explicitly trying to explain something (like in a comment above about why target isn't checked, or, assert it or something).
|
Didn't realize this wasn't accepted; will look if the feature is okay'd. |
|
This test case doesn't gets failed by the original code and doesn't get passed to the new function in the pull (which would pass it): So I have to check this before proceeding further. |
|
@craigphicks Is this PR worth keeping open while waiting for us to have time to discuss the original issue? It's out of date with main now, but it might still be useful if you want to run top400 or other tests. |
Usage Scenario:
Suppose an intersection type:
Current behavior
Without this pull, in order to write an implementation, the only reliable error-free overload implementation and declaration requires concatenating all the declarations of the intersection type as follows:
That has disadvantages:
New Behavior
With this pull the following declaration and implementation are allowed instead:
Implementation
Where
The function
checkOverloadsRelatedToIntersectionis called only when(target.flags & TypeFlags.Intersection) andtypeRelatedToEachType(source,target)returnedTernary.FalseTherefore, it only affect cases which were previously rejected.
Algorithm
Algorithm used within
checkOverloadsRelatedToIntersection:SourceDomainMatch checks that source domain maps into the target domain.
SourceRangeMatch checks that source range maps into the target range.
TargetDomainMatch checks that target domain maps into the source domain
Diff URL
Fixes #57112