-
Notifications
You must be signed in to change notification settings - Fork 12.6k
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
Propagate saved variance flags from cached comparisons #31688
Propagate saved variance flags from cached comparisons #31688
Conversation
@weswigham shall we port to 3.5? Sounds like React users might encounter this reasonably frequently |
@RyanCavanaugh probably - it's definitely a 3.5 regression and because it's caused by check order differences it's mega hard to debug what's going on - I wasn't even sure at first (since the type errors are kiiinda reasonable, especially if you don't know the subtleties of the "unreliable" variance markers) and had to debug for a few hours to track it back to this cause. @ahejlsberg I've cleaned this up and was even able to remove some now redundant suspect code from when I added the And now that I have a test and everything's in order, time for the extended tests: |
Heya @weswigham, I've started to run the extended test suite on this PR at 7b62fb1. You can monitor the build here. It should now contribute to this PR's status checks. |
Heya @weswigham, I've started to run the perf test suite on this PR at 7b62fb1. You can monitor the build here. It should now contribute to this PR's status checks. Update: The results are in! |
Heya @weswigham, I've started to run the parallelized Definitely Typed test suite on this PR at 7b62fb1. You can monitor the build here. It should now contribute to this PR's status checks. |
@weswigham Here they are:Comparison Report - master..31688
System
Hosts
Scenarios
|
Perf's unaffected by this change, |
31ca94d
to
25d30dd
Compare
1a05322
to
37fbb32
Compare
37fbb32
to
c96fee1
Compare
c96fee1
to
b36246d
Compare
@typescript-bot cherrypick this into release-3.5 again please thanks |
Hey @weswigham, I've opened #31707 for you. |
Fixes this issue reported by @JasonGore.
Watch mode vs batch mode was altering our check order and changing if we checked React's
WeakValidationMap
's variance beforeFunctionComponent
's or not. When we checked it first, we failed to markFunctionComponent
's variance as "unreliable", since the cached comparison of the marker-instantiatedWeakValidationMap
prevented us from actually doing the comparison and discovering the unreliable-ness. The fix is simply to propagate any unreliable-ness (already calculated, since the comparison we already know to be cached) from a cached comparison's variances when we return the cached result (alternatively, we could rerun the comparison to rediscover the reliability of the type, but I think that'd be more expensive).