-
Notifications
You must be signed in to change notification settings - Fork 12.9k
Remove reportErrors check in relateVariances #55222
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
base: main
Are you sure you want to change the base?
Conversation
@typescript-bot test this |
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at df695d9. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based user code test suite (tsserver) on this PR at df695d9. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the tarball bundle task on this PR at df695d9. You can monitor the build here. |
Heya @jakebailey, I've started to run the extended test suite on this PR at df695d9. You can monitor the build here. |
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at df695d9. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the perf test suite on this PR at df695d9. 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 df695d9. You can monitor the build here. Update: The results are in! |
Heya @jakebailey, I've started to run the diff-based top-repos suite (tsserver) on this PR at df695d9. You can monitor the build here. Update: The results are in! |
Hey @jakebailey, 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 |
src/compiler/checker.ts
Outdated
// if (entry !== undefined) { | ||
// // If the previous entry and the result disagree, then something has gone wrong. | ||
// Debug.assert(!!(entry & RelationComparisonResult.Succeeded) === (result !== Ternary.False), "Cached relationship does not match recalculated result"); | ||
// } |
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.
Probably this is a good thing to leave in, once the actual fix is in, that way we can observe this kind of bug faster.
src/compiler/checker.ts
Outdated
@@ -21951,7 +21956,7 @@ export function createTypeChecker(host: TypeCheckerHost): TypeChecker { | |||
// reveal the reason). | |||
// We can switch on `reportErrors` here, since varianceCheckFailed guarantees we return `False`, |
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 mentions the code I deleted, so I'd have to delete this if this fix is right.
@jakebailey Here are the results of running the user test suite comparing Everything looks good! |
@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:
CompilerComparison Report - main..55222
System
Hosts
Scenarios
TSServerComparison Report - main..55222
System
Hosts
Scenarios
StartupComparison Report - main..55222
System
Hosts
Scenarios
Developer Information: |
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details. |
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
7% check degradation in xstate, ouch @typescript-bot perf test this faster |
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 5902018. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:Comparison Report - main..55222
System
Hosts
Scenarios
Developer Information: |
This PR failed RWC but the diffs aren't correctly being pushed. Need to figure that out. |
@typescript-bot perf test this faster |
Heya @jakebailey, I've started to run the tsc-only perf test suite on this PR at 5902018. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test this faster |
Heya @jakebailey, I've started to run the tsc-only perf test suite on this PR at 2ef27bc. You can monitor the build here. Update: The results are in! |
@typescript-bot perf test this bun |
Heya @jakebailey, I've started to run the bun perf test suite on this PR at 2ef27bc. You can monitor the build here. Update: The results are in! |
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Seems like quite a few percent in xstate. Very odd but I'll have to profile it I guess. |
@jakebailey Here they are:
CompilerComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
StartupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
It's been awhile, but I'll chime in, since I've been working on variance stuff so much: the variance result and non-variance result for a comparison are supposed to be the same. If they're not, many strange things happen. Like this. It's why |
Just to be clear, are you saying this PR is "good", in that it removes a place where we can produce differing variance results? Or am I misunderstanding? (Generally I want to make progress on this PR though I have not figured out the perf problem yet...) |
@typescript-bot perf test this |
Heya @jakebailey, I've started to run the regular perf test suite on this PR at 26f1441. You can monitor the build here. Update: The results are in! |
@jakebailey 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: |
@typescript-bot perf test this |
@jakebailey 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: |
@typescript-bot perf test this faster |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Surely if I keep retrying it, it will eventually not break perf, right? @typescript-bot perf test this faster |
@jakebailey Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
Fixes #55217
As noted by @Andarist, this check is what causes things to differ between errors and non-error modes in relation, causing oddities when the same relation is observed in both ways but in different orders.
No doubt this is a bad idea but I wanted to run the test suite to see what happens.
Bunch of WIP code is left in the PR, I'll remove/refactor it if this code actually looks acceptable.