-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
Defer type comparability check for assertions #53261
Conversation
@typescript-bot perf test this |
Heya @gabritto, I'm starting to run the parallelized Definitely Typed test suite on this PR at 46c61b3. Hold tight - I'll update this comment with the log link once the build has been queued. Update: The results are in! |
Heya @gabritto, I'm starting to run the diff-based user code test suite on this PR at 46c61b3. Hold tight - I'll update this comment with the log link once the build has been queued. Update: The results are in! |
Heya @gabritto, I'm starting to run the diff-based top-repos suite on this PR at 46c61b3. Hold tight - I'll update this comment with the log link once the build has been queued. Update: The results are in! |
Heya @gabritto, I'm starting to run the perf test suite on this PR at 46c61b3. Hold tight - I'll update this comment with the log link once the build has been queued. Update: The results are in! |
@gabritto Here are the results of running the user test suite comparing Everything looks good! |
@gabritto Here they are:
CompilerComparison Report - main..53261
System
Hosts
Scenarios
TSServerComparison Report - main..53261
System
Hosts
Scenarios
StartupComparison Report - main..53261
System
Hosts
Scenarios
Developer Information: |
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
Hey @gabritto, the results of running the DT tests are ready. |
@typescript-bot perf test this faster |
Heya @gabritto, I've started to run the abridged perf test suite on this PR at 45124b1. You can monitor the build here. Update: The results are in! |
@gabritto Here they are:
Comparison Report - main..53261
System
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test this faster |
Heya @gabritto, I've started to run the abridged perf test suite on this PR at d409b7f. You can monitor the build here. Update: The results are in! |
@gabritto Here they are:Comparison Report - main..53261
System
Hosts
Scenarios
Developer Information: |
@typescript-bot perf test this faster |
Heya @gabritto, I've started to run the abridged perf test suite on this PR at 469f24d. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the diff-based top-repos suite (tsserver) on this PR at 7620056. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the perf test suite on this PR at 7620056. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the parallelized Definitely Typed test suite on this PR at 7620056. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the diff-based user code test suite on this PR at 7620056. You can monitor the build here. Update: The results are in! |
Heya @gabritto, I've started to run the diff-based top-repos suite on this PR at 7620056. You can monitor the build here. Update: The results are in! |
@gabritto Here are the results of running the user test suite comparing Everything looks good! |
@gabritto Here they are:
CompilerComparison Report - main..53261
System
Hosts
Scenarios
TSServerComparison Report - main..53261
System
Hosts
Scenarios
StartupComparison Report - main..53261
System
Hosts
Scenarios
Developer Information: |
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
Hey @gabritto, the results of running the DT tests are ready. |
@gabritto Here are the results of running the top-repos suite comparing Everything looks good! |
@typescript-bot perf test this faster |
Heya @gabritto, I've started to run the abridged perf test suite on this PR at fd7d37a. You can monitor the build here. Update: The results are in! |
@gabritto Here they are:Comparison Report - main..53261
System
Hosts
Scenarios
Developer Information: |
@@ -0,0 +1,16 @@ | |||
// @strict: true | |||
|
|||
// Issue #52813 |
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 is an example where this PR's approach doesn't solve the circularity problem.
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.
I assume you mean the other test with the function calls rather than expressions, since that's the one that still has an error. :) If you want, you should file an issue about the other form, so the issue can still get backlog'd, if you want to consider looking into a broader fix in the future after this PR marks the original issue as fixed.
@typescript-bot perf test this |
Heya @gabritto, I've started to run the perf test suite on this PR at fd7d37a. You can monitor the build here. Update: The results are in! |
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.
Looks good, just a small style suggestion to remove some duplication.
@@ -0,0 +1,16 @@ | |||
// @strict: true | |||
|
|||
// Issue #52813 |
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.
I assume you mean the other test with the function calls rather than expressions, since that's the one that still has an error. :) If you want, you should file an issue about the other form, so the issue can still get backlog'd, if you want to consider looking into a broader fix in the future after this PR marks the original issue as fixed.
@gabritto Here they are:
CompilerComparison Report - main..53261
System
Hosts
Scenarios
TSServerComparison Report - main..53261
System
Hosts
Scenarios
StartupComparison Report - main..53261
System
Hosts
Scenarios
Developer Information: |
It's kinda unfortunate that there seems to be a slight perf impact on check time, but I don't immediately see what could be done to improve this, since my previous attempts to either defer the call to |
This reverts commit 8947825.
Fixes #52813.
This is the fix that is implemented as deferring the type comparability check we do for assertions, and it fixes the circularity case in the original issue.
However, we can still trigger circularity errors when computing variance, as one of the new tests shows, because we can't always defer the check that makes us compute variances.
I'll attempt a different fix next that involves "resetting" the type resolution stack when we start to compute variances.