Repository navigation
Conversation
Checking purity annotations is the entire purpose of the Purity Checker, so running it no longer requires also supplying -AcheckPurityAnnotations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Now that the Purity Checker implies -AcheckPurityAnnotations, `checkPurity` verifies purity annotations rather than trusting them, and javacutil reported about 100 errors. It now reports none. Most are fixed by writing the annotation that was missing: * @SideEffectFree on BugInCF's constructors and @pure on getLocation(). This alone fixed 14 errors, because `throw new BugInCF(...)` appears in many methods that are declared pure. * @pure on ten TreeUtils methods, each of which reads a tree or symbol field. * @pure on CheckerFrameworkAnnotationValue.getValue(), and @SideEffectFree on DefaultAnnotationFormatter's constructor. The rest are suppressed, and the suppressions are concentrated in one structural limitation: the Purity Checker has no escape analysis. It therefore cannot verify: * a @deterministic method that iterates a collection, because Collection.iterator() and Map.keySet()/entrySet() allocate a fresh iterator or view -- so they are @SideEffectFree but not @deterministic -- and Iterator.next() mutates the iterator; * a @SideEffectFree method that mutates a locally allocated StringBuilder, StringJoiner, or TreeSet, or that memoizes its result in a field; * a @deterministic method containing a catch block. Because java.util.Map and java.util.Set declare containsKey, get, contains, equals, and hashCode as @pure, and purity annotations are inherited, every collection implementation inherits an obligation that it cannot discharge. AnnotationMirrorMap takes a class-level suppression for that reason, since all of its inherited pure methods are affected; elsewhere the suppressions are per-method. This is concretely the "high false positive rate" that the manual cites for not enabling -AcheckPurityAnnotations by default, and it is what the new purity analysis promised there would need to address. Two annotations are retained with a suppression rather than removed, because each is load-bearing for type refinement in callers: AnnotationUtils.annotationName (Name.toString() returns a fresh but always-equal String) and AbstractTypeProcessor.getCompilerLog (Log.instance() registers the Log in the compiler Context on its first call). Dropping @SideEffectFree from the latter costs three nullness errors in CFGProcessor, whose fields lose their refinement across the call. `typecheck-part1` runs `checkPurity` only on javacutil; the other subprojects do not pass it yet. Each still has its own `checkPurity` task to run directly. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hecker This clears 21 errors across 6 files, and brings the number of subprojects that pass `checkPurity` from one to five: checker-qual-android needed no edit, because its 14 errors were in generated copies of the two checker-qual files fixed here. ConversionCategory.toString() was declared @pure, which contradicts the rule that the annotated JDK records in a @CFComment on Object.toString(): a toString() that builds its result returns a string that differs according to ==, and @deterministic requires that two calls return == results. It is now @SideEffectFree. That was a wrong annotation rather than a false positive. The other errors are the structural limitation described in the previous commit: the Purity Checker has no escape analysis, so it cannot verify a method that mutates a locally allocated StringBuilder, StringJoiner, or list, that calls String.format or Character.toString (each of which returns a fresh string), or that contains a catch block. Three of RegexUtil's suppressions already explained this reasoning -- one comment reads "temp value used in pure method is equal up to equals but not up to ==" -- so "purity" joins the existing @SuppressWarnings lists rather than arriving as a separate annotation, and each key now carries its own justification. In framework-test, both errors reached the same helper, so annotating the chain (TestUtilities.optionMapToList, SimpleOptionMap.getOptionsAsList, and ImmutableTestConfiguration.getFlatOptions as @SideEffectFree) needs one suppression instead of two and gives those methods a correct specification. `typecheck-part1` now excludes `checkPurity` by naming the four subprojects that still fail -- annotation-file-utilities, checker, dataflow, and framework -- so that a subproject is covered as soon as it passes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
annotationName returns `elm.getQualifiedName().toString()`, a freshly built string, so it is not deterministic in the sense @deterministic requires. The annotated JDK records that rule in a @CFComment on Object.toString(): such a method "returns a string that differs according to ==, and @deterministic requires that the results of two calls of the method are ==." compareByName is weakened for the same reason, because it compares the results of two annotationName calls. Weakening it cascades no further: areSameByName(AnnotationMirror, AnnotationMirror) has no purity annotation, and compareAnnotationMirrors already needs its own suppression for the local TreeSet that it builds. This removes a suppression rather than adding one. The @pure annotation on annotationName required suppressing the determinism warning on its own body, which amounted to documenting that the annotation was not true; the accurate annotation needs no suppression at all. Verified that nothing regresses: `typecheck-part1` and `typecheck-part2` (all nine checkers) pass with no errors for every subproject that runs checkPurity, and `checkNullness` still passes for framework and checker, which is where the weaker specification could have cost type refinement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: typetools/checker-framework/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (26)
💤 Files with no reviewable changes (2)
Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review. 📝 WalkthroughWalkthroughThe Suggested reviewers: Priority: ⬇️ Low Change: Other Merge Risk: ⚪ Minimal · up to This change adds purity annotations and warning suppressions and enables purity checking in the build for most subprojects. Runtime behavior is unchanged, and no concrete defects were found. As stated in the description, the companion JDK annotation change should be merged first so that the updated test expectations match. 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 75.61% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 82 functions across 24 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
smillst
changed the title
-AcheckPurityAnnotationsComparator.thenComparing is @SideEffectFree in the annotated JDK, so the overriding method in Issue579 inherits that annotation and may only call side-effect-free constructors.
…y Checker Methods annotated @SideEffectsOnly are now checked, so their callees need purity annotations.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I've added annotations for
javacutil, and the smaller subprojects.Merge typetools/jdk#299 first.