-
Notifications
You must be signed in to change notification settings - Fork 299
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
Update to Checker Framework 3.39.0 #839
Conversation
/benchmark |
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## master #839 +/- ##
============================================
+ Coverage 86.78% 86.80% +0.01%
- Complexity 1877 1878 +1
============================================
Files 74 74
Lines 6191 6192 +1
Branches 1204 1204
============================================
+ Hits 5373 5375 +2
Misses 407 407
+ Partials 411 410 -1
☔ View full report in Codecov by Sentry. |
/benchmark |
Main Branch:
With This PR:
|
I've confirmed that the code coverage issue was due to an issue in Checker Framework; see typetools/checker-framework#6215. So this will be fixed once Checker Framework cuts another release and we update. In the meantime, I think we should go ahead and land this (and possibly cut a new release) to fix the #831 crasher on JDK 21. |
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.
Quick question/nit below, but otherwise LGTM
public TransferResult<Nullness, NullnessStore> visitDeconstructorPattern( | ||
DeconstructorPatternNode deconstructorPatternNode, | ||
TransferInput<Nullness, NullnessStore> input) { | ||
return noStoreChanges(NULLABLE, input); |
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.
Why is this marked as not covered? Because the codecov report is for an older JDK?
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.
It's due to typetools/checker-framework#6215, which is now fixed. Before, these nodes were not getting added properly to CFGs by Checker dataflow. Turns out checking code coverage is handy!
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.
We won't get the coverage until Checker Framework does another release and we update, but I don't think it's worth waiting for that to land this.
Fixes #831 as Checker Framework dataflow now has support for JDK 21 constructs