@SideEffectsOnly annotation - #7818
Conversation
Co-Authored-By: James Yoo <jyoo980@gmail.com> Co-Authored-By: Rashmi Mudduluru <rashmi4@cs.washington.edu>
|
@CodeRabbit review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughThis PR adds Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java (1)
1320-1330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead placeholder code left in method.
The comment explicitly says this is "just a placeholder";
body/bodyAssignedare (re)computed here but never used afterward, since the method returns immediately after. This is unreachable-in-effect scaffolding that should be removed before merge.- // There will be code here that *may* use `body` (and may set `body` before using it). - // The below is just a placeholder so `bodyAssigned` is not a dead variable. - // ... - if (!bodyAssigned) { - body = atypeFactory.getPath(tree.getBody()); - bodyAssigned = true; - } - // ... }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java` around lines 1320 - 1330, Remove the placeholder fallback block guarded by !bodyAssigned in the surrounding method, including the assignments to body and bodyAssigned and its explanatory comments. Preserve the method’s existing control flow and immediate return without adding replacement logic.framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java (1)
329-337: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve selectively retained method-call facts.
updateMethodCallValuesretains facts unaffected by@SideEffectsOnly, but Lines 331-337 immediately evict every modifiableMethodCallanyway. This makesmethodA(optContainer, strs)invalidateoptContainer.getOpt()despite its contract listing only#2, so theOptionalSideEffectsPrecondition.test1flow refinement is lost.Only run the broad eviction when there is no
@SideEffectsOnlycontract; includedoNotUnrefinein the selective path as well.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java` around lines 329 - 337, Update the method-call fact eviction around updateMethodCallValues so the broad removal of modifiable MethodCall entries runs only when no `@SideEffectsOnly` contract applies. When a selective contract is present, preserve the facts retained by updateMethodCallValues while also applying doNotUnrefine to avoid removing protected calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@checker-qual/src/main/java/org/checkerframework/dataflow/qual/SideEffectsOnly.java`:
- Around line 10-16: Update the Javadoc example for the SideEffectsOnly
annotation to use valid array-value syntax with braces around the multiple
expressions, while preserving the documented A and B side-effect values.
In `@docs/manual/advanced-features.tex`:
- Around line 1121-1129: Correct the `@SideEffectsOnly` example in the
computeValue documentation so its two arguments are distinct placeholder
expressions, replacing the duplicated someOtherVariable1 entry while preserving
the multi-argument syntax.
In `@docs/manual/nullness-checker.tex`:
- Around line 242-243: Clarify the documentation near the
SideEffectFree/SideEffectsOnly reference that SideEffectsOnly preserves nullness
refinement only for fields not included in its side-effect expressions; listed
fields may lose the refinement because the method can set them to null. Keep the
existing exception wording and add only this parenthetical qualification.
In `@docs/manual/purity-checker.tex`:
- Around line 48-52: Reword the trusted-annotations sentence around the
SideEffectFree and SideEffectsOnly references so `@SideEffectsOnly` is described
as valid only for methods whose side effects stay within the specified
expressions, rather than as side-effect-free. Preserve the existing warning
about incorrectly using SideEffectFree and `@Deterministic`.
In
`@framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java`:
- Around line 1154-1164: Remove the redundant checkPurityAnnotations condition
at the later validation point in checkPurityAnnotations(MethodTree tree), since
the method already returns when the field is false. Remove the temporary
`@SuppressWarnings`("AlreadyChecked") annotation and preserve the remaining
purity-validation logic unchanged.
- Around line 1298-1319: Remove the three commented-out System.out.printf debug
statements near the DisallowedSideEffects result handling in BaseTypeVisitor,
including the "Error 2%n" statement, while leaving the surrounding side-effect
validation and error reporting unchanged.
- Around line 1273-1296: Update the empty sideEffectsOnlyExpressionStrings
branch in the surrounding annotation-processing method to return immediately
after reporting purity.empty.sideeffectsonly, preventing the later
purity.more.sideeffectfree warning for the same empty annotation. Preserve the
existing parse-error returns and non-empty expression handling.
In
`@framework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.java`:
- Around line 243-273: Update visitUnary and visitCompoundAssignment to record
the affected expression in extraSideEffects using the same filtering and
expression-conversion pattern as visitAssignment, while preserving traversal
through super. Ensure increment/decrement and compound assignments are reported
when their target is not in sideEffectsOnlyExpressionsFromAnnotation; leave
visitNewClass unchanged unless required by the existing side-effect model.
- Around line 159-174: Update the purity.incorrect.sideeffectsonly reporting in
visitMethodInvocation to pass the enclosing annotated method’s name and the
specific offending expression, rather than invokedElem.getSimpleName() and the
full sideEffectsOnlyExpressionsFromAnnotation list. Ensure all calls classified
as potentially modifying anything use this correctly scoped diagnostic.
- Around line 258-263: Update DisallowedSideEffects.visitVariable to check
node.getInitializer() before calling JavaExpression.fromTree; when the
initializer is null, return without adding an alias, otherwise preserve the
existing aliasing and superclass traversal behavior.
In `@framework/tests/purity-suggestions/PuritySuggestionsClass.java`:
- Around line 180-182: Update the test for foo() to explicitly expect both
diagnostics emitted by the empty `@SideEffectsOnly` contract: add a
purity.empty.sideeffectsonly expectation alongside the existing
purity.more.sideeffectfree directive, preserving the adjacent test-directive
format.
---
Outside diff comments:
In
`@framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java`:
- Around line 1320-1330: Remove the placeholder fallback block guarded by
!bodyAssigned in the surrounding method, including the assignments to body and
bodyAssigned and its explanatory comments. Preserve the method’s existing
control flow and immediate return without adding replacement logic.
In
`@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java`:
- Around line 329-337: Update the method-call fact eviction around
updateMethodCallValues so the broad removal of modifiable MethodCall entries
runs only when no `@SideEffectsOnly` contract applies. When a selective contract
is present, preserve the facts retained by updateMethodCallValues while also
applying doNotUnrefine to avoid removing protected calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 29472d01-63bd-49ba-8f4d-4ba839a7ee30
📒 Files selected for processing (29)
annotation-file-utilities/src/test/resources/annotations/tests/classfile/all-annotations.jaifchecker-qual/src/main/java/org/checkerframework/dataflow/qual/SideEffectsOnly.javachecker/src/test/java/org/checkerframework/checker/test/junit/SideEffectsOnlyTest.javachecker/src/test/java/org/checkerframework/checker/test/junit/ainferrunners/OptionalSideEffectsTest.javachecker/tests/optional-side-effects/OptionalSideEffectsLambda.javachecker/tests/optional-side-effects/OptionalSideEffectsPrecondition.javachecker/tests/sideeffectsonly/CheckSideEffectsOnly.javachecker/tests/sideeffectsonly/EmptySideEffectsOnly.javachecker/tests/sideeffectsonly/IncorrectSideEffectsOnly.javachecker/tests/sideeffectsonly/NestedSideEffectsNoAliasing.javachecker/tests/sideeffectsonly/NestedSideEffectsWithAliasing.javachecker/tests/sideeffectsonly/SideEffectsMultiple.javachecker/tests/sideeffectsonly/SideEffectsOnlyConflictingAnnotations.javachecker/tests/sideeffectsonly/SideEffectsOnlyField.javachecker/tests/sideeffectsonly/SideEffectsOnlyTest1.javachecker/tests/sideeffectsonly/SideEffectsTest1.javachecker/tests/sideeffectsonly/TestMethodInvocation.javadocs/manual/advanced-features.texdocs/manual/called-methods-checker.texdocs/manual/introduction.texdocs/manual/nullness-checker.texdocs/manual/purity-checker.texdocs/manual/troubleshooting.texframework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.javaframework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.javaframework/src/main/java/org/checkerframework/common/basetype/messages.propertiesframework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.javaframework/src/main/java/org/checkerframework/framework/type/GenericAnnotatedTypeFactory.javaframework/tests/purity-suggestions/PuritySuggestionsClass.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java (3)
467-488: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftUse alias-aware matching for side-effected fields.
sideEffectsOnlyExpressions.contains(fieldAccess)only matches exactJavaExpressionequality. A contract forobj.fcan affect the same field accessed through an alias such asalias.f; preserving that entry can leave stale flow facts. Reuse the store’s conservative alias analysis when deciding whether a field is covered by the contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java` around lines 467 - 488, Update updateFieldValuesForMethodCall to use the store’s existing conservative alias-analysis helper when determining whether each fieldAccess is covered by sideEffectsOnlyExpressions, instead of relying on exact List.contains equality. Preserve previousValue only when the field is not alias-matched by any side-effect expression, so accesses such as obj.f and alias.f are handled consistently.
311-324: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftHonor
SideEffectsOnlywhen alias unrefinement is enabled.When
sideEffectsUnrefineAliasesis true, the earlier branch removes all modifiable local/field facts, whilesideEffectsOnlyExpressionsis only used in theelsebranch. This causes@SideEffectsOnlyto invalidate facts outside its declared expressions whenever that option is enabled. Apply the contract consistently in both paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java` around lines 311 - 324, Update the sideEffectsUnrefineAliases branch in the field-value update flow to honor sideEffectsOnlyExpressions, restricting removal to facts whose FieldAccess expressions are declared side-effect targets while still removing eligible modifiable aliases. Preserve the existing doNotUnrefine predicate and the non-alias branch through updateFieldValuesForMethodCall.
329-337: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not undo the selective method-call filtering.
updateMethodCallValues(sideEffectsOnlyExpressions)selectively removes affected method-call facts, but the followingremoveIfimmediately removes every modifiable method call. Consequently,@SideEffectsOnly(...)never preserves method-call refinements.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java` around lines 329 - 337, Update the method-call cleanup following updateMethodCallValues so it does not unconditionally remove all modifiable calls; preserve entries retained by the selective side-effects filtering, including `@SideEffectsOnly` refinements. Adjust the removeIf predicate or cleanup flow around methodCallExpressions to remove only facts not already handled by updateMethodCallValues, while retaining the existing doNotUnrefine behavior.
♻️ Duplicate comments (1)
framework/tests/purity-suggestions/PuritySuggestionsClass.java (1)
181-181: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the diagnostic-key typo.
The test expects
purity.emptysideffectsonly, but the registered key ispurity.empty.sideeffectsonly. The current expectation will not match the emitted diagnostic.Proposed fix
- // :: error: (purity.emptysideffectsonly) :: warning: (purity.more.sideeffectfree) + // :: error: (purity.empty.sideeffectsonly) :: warning: (purity.more.sideeffectfree)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/tests/purity-suggestions/PuritySuggestionsClass.java` at line 181, Update the diagnostic expectation comment in PuritySuggestionsClass to use the registered key purity.empty.sideeffectsonly instead of the misspelled purity.emptysideffectsonly, while preserving the existing purity.more.sideeffectfree warning expectation.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In
`@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java`:
- Around line 467-488: Update updateFieldValuesForMethodCall to use the store’s
existing conservative alias-analysis helper when determining whether each
fieldAccess is covered by sideEffectsOnlyExpressions, instead of relying on
exact List.contains equality. Preserve previousValue only when the field is not
alias-matched by any side-effect expression, so accesses such as obj.f and
alias.f are handled consistently.
- Around line 311-324: Update the sideEffectsUnrefineAliases branch in the
field-value update flow to honor sideEffectsOnlyExpressions, restricting removal
to facts whose FieldAccess expressions are declared side-effect targets while
still removing eligible modifiable aliases. Preserve the existing doNotUnrefine
predicate and the non-alias branch through updateFieldValuesForMethodCall.
- Around line 329-337: Update the method-call cleanup following
updateMethodCallValues so it does not unconditionally remove all modifiable
calls; preserve entries retained by the selective side-effects filtering,
including `@SideEffectsOnly` refinements. Adjust the removeIf predicate or cleanup
flow around methodCallExpressions to remove only facts not already handled by
updateMethodCallValues, while retaining the existing doNotUnrefine behavior.
---
Duplicate comments:
In `@framework/tests/purity-suggestions/PuritySuggestionsClass.java`:
- Line 181: Update the diagnostic expectation comment in PuritySuggestionsClass
to use the registered key purity.empty.sideeffectsonly instead of the misspelled
purity.emptysideffectsonly, while preserving the existing
purity.more.sideeffectfree warning expectation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14b00220-59c9-4390-bf93-1cc5f615d6a3
📒 Files selected for processing (4)
docs/manual/advanced-features.texframework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.javaframework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.javaframework/tests/purity-suggestions/PuritySuggestionsClass.java
💤 Files with no reviewable changes (1)
- framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
docs/manual/advanced-features.tex (1)
1121-1129: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd
@SideEffectsOnlyto the Java-expression annotation index.The new annotation accepts Java-expression strings, but the list at lines 1292-1316 omits it.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/manual/advanced-features.tex` around lines 1121 - 1129, Add the SideEffectsOnly annotation to the Java-expression annotation index alongside the existing expression annotations, ensuring its Java-expression string parameters are documented there. Keep the existing advanced-features example and annotation behavior unchanged.framework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.java (1)
134-167: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMisleading diagnostic args for unannotated calls that "may modify anything" — still unresolved.
When an un-annotated, argument/receiver-less call is made inside a
@SideEffectsOnlymethod, this reportspurity.incorrect.sideeffectsonlywithinvokedElem.getSimpleName()(the callee's name) andsideEffectsOnlyExpressionsFromAnnotation(the entire allow-list) as arguments. Per the message format (the method %s may side-effect %s), this produces a confusing message naming the wrong method and printing the whole allow-list instead of the specific offending expression, unlike the analogous report a few lines below (line ~1305 inBaseTypeVisitor) which correctly uses the annotated method's name and the specific offending expression.🐛 Suggested fix direction
if (actualSideEffectedExprs.isEmpty()) { // If the args are empty, it might be modifying anything. checker.reportError( node, "purity.incorrect.sideeffectsonly", - invokedElem.getSimpleName(), - sideEffectsOnlyExpressionsFromAnnotation); + enclosingMethodName, + invokedElem.getSimpleName() + "()"); }(Requires threading the enclosing annotated method's name into
DisallowedSideEffectsHelper, e.g. viacheckSideEffectsOnly(...)'s parameters.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.java` around lines 134 - 167, Update DisallowedSideEffectsHelper’s side-effects-only checking flow, including checkSideEffectsOnly and visitMethodInvocation, to receive the enclosing annotated method’s name and use it in purity.incorrect.sideeffectsonly diagnostics. For unannotated calls with no argument or receiver expressions, report the specific offending expression rather than the entire sideEffectsOnlyExpressionsFromAnnotation allow-list, matching the analogous BaseTypeVisitor diagnostic.framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java (1)
1154-1157: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
@SideEffectsOnlyshould not depend on purity-mode flagscheckPurityAnnotations()returns before the new@SideEffectsOnlyblock unless-AcheckPurityAnnotations(or-AsuggestPureMethods/-Ainfer) is enabled, so standalone@SideEffectsOnlymethods get no check.SideEffectsOnlyTest.javaalready passes-AcheckPurityAnnotations, but if this contract is meant to be enforced on its own, move that block out of the early return.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java` around lines 1154 - 1157, Update checkPurityAnnotations in BaseTypeVisitor so `@SideEffectsOnly` validation executes independently of the checkPurityAnnotations flag and related purity-mode options. Move the `@SideEffectsOnly` handling before the early return, while leaving the existing purity checks gated by that flag.framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java (2)
89-91: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache
SideEffectsOnly.valueoutside the store constructor.upperBound()creates a fresh store at each merge/widen point, soTreeUtils.getMethod(SideEffectsOnly.class, "value", 0, analysis.env)is resolved repeatedly on a hot path. Hoist that lookup toCFAbstractAnalysisor the type factory and pass it in; the field is only assigned in constructors, so it can also beprotected finalinstead ofpublic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java` around lines 89 - 91, The SideEffectsOnly.value method lookup is repeated for every fresh CFAbstractStore created by upperBound(). Cache the resolved ExecutableElement in CFAbstractAnalysis or the type factory, pass it into CFAbstractStore constructors, and change sideEffectsOnlyValueElement to a protected final field while preserving all existing uses.
287-309: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor
@SideEffectsOnlyfor field invalidation in the alias-unrefining branch. The field path still drops every modifiable field except@DoesNotUnrefineReceiver, so@SideEffectsOnlyis ignored there even though arrays and method-call expressions already respect it. Reuse the same side-effect filter for fields to avoid unnecessary loss of precision.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java` around lines 287 - 309, Update the alias-unrefining field removal in CFAbstractStore so it uses the existing isSideEffected filter, preserving doNotUnrefine handling while honoring sideEffectsOnlyExpressions like arrayValues and methodCallExpressions. Keep the non-alias-unrefining updateFieldValuesForMethodCall path unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dataflow/src/main/java/org/checkerframework/dataflow/util/PurityUtils.java`:
- Around line 37-45: Update both overloads of hasPurityAnnotation and the
underlying getPurityKinds logic to recognize `@SideEffectsOnly` as a purity
annotation. Remove the obsolete TODO and revise the method Javadoc if its
terminology or behavior needs clarification, ensuring PurityChecker no longer
treats `@SideEffectsOnly` methods as unannotated.
In
`@framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java`:
- Around line 1310-1319: Remove the placeholder block guarded by !bodyAssigned
in the surrounding method, including the reassignment of body and bodyAssigned;
it is dead code immediately before the method returns. Leave the surrounding
logic and comments intact unless they only describe this removed TODO block.
- Around line 1278-1291: In the JavaExpressionParseException handling loop for
seOnlyExpressionStrings, update the checker.report call to use the enclosing
methodTree as the diagnostic source instead of the raw String variable st.
Preserve the existing special handling for "flowexpr.parse.error" and the normal
DiagMessage reporting path.
In
`@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java`:
- Around line 365-368: Annotate the nullable parameters in isSideEffected—
specifically notSideEffectedExpression and sideEffectsOnlyExpressions—with
`@Nullable`, and likewise annotate receiverJe and sideEffectsOnlyExpressions in
updateFieldValuesForMethodCall. Preserve the existing null-checking behavior and
use the project’s established nullable annotation import.
- Around line 355-380: Update the sideEffectsOnlyExpressions branch in
isSideEffected so a matching expression is treated as side-effected by returning
the anyMatch result directly, while preserving the existing behavior for
non-modifiable expressions and notSideEffectedExpression.
---
Outside diff comments:
In `@docs/manual/advanced-features.tex`:
- Around line 1121-1129: Add the SideEffectsOnly annotation to the
Java-expression annotation index alongside the existing expression annotations,
ensuring its Java-expression string parameters are documented there. Keep the
existing advanced-features example and annotation behavior unchanged.
In
`@framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java`:
- Around line 1154-1157: Update checkPurityAnnotations in BaseTypeVisitor so
`@SideEffectsOnly` validation executes independently of the checkPurityAnnotations
flag and related purity-mode options. Move the `@SideEffectsOnly` handling before
the early return, while leaving the existing purity checks gated by that flag.
In
`@framework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.java`:
- Around line 134-167: Update DisallowedSideEffectsHelper’s side-effects-only
checking flow, including checkSideEffectsOnly and visitMethodInvocation, to
receive the enclosing annotated method’s name and use it in
purity.incorrect.sideeffectsonly diagnostics. For unannotated calls with no
argument or receiver expressions, report the specific offending expression
rather than the entire sideEffectsOnlyExpressionsFromAnnotation allow-list,
matching the analogous BaseTypeVisitor diagnostic.
In
`@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java`:
- Around line 89-91: The SideEffectsOnly.value method lookup is repeated for
every fresh CFAbstractStore created by upperBound(). Cache the resolved
ExecutableElement in CFAbstractAnalysis or the type factory, pass it into
CFAbstractStore constructors, and change sideEffectsOnlyValueElement to a
protected final field while preserving all existing uses.
- Around line 287-309: Update the alias-unrefining field removal in
CFAbstractStore so it uses the existing isSideEffected filter, preserving
doNotUnrefine handling while honoring sideEffectsOnlyExpressions like
arrayValues and methodCallExpressions. Keep the non-alias-unrefining
updateFieldValuesForMethodCall path unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0dfcb23f-611a-46af-9587-0dc72c18b032
📒 Files selected for processing (14)
checker-qual/src/main/java/org/checkerframework/dataflow/qual/SideEffectsOnly.javachecker/src/main/java/org/checkerframework/checker/initialization/InitializationStore.javachecker/src/main/java/org/checkerframework/checker/lock/LockStore.javadataflow/src/main/java/org/checkerframework/dataflow/expression/JavaExpression.javadataflow/src/main/java/org/checkerframework/dataflow/util/PurityUtils.javadocs/CHANGELOG.mddocs/manual/advanced-features.texdocs/manual/nullness-checker.texdocs/manual/purity-checker.texframework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.javaframework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.javaframework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.javaframework/src/main/java/org/checkerframework/framework/flow/CFAbstractTransfer.javaframework/tests/purity-suggestions/PuritySuggestionsClass.java
…no-factory into side-effects-only-2-pre
…effects-only-2-pre
… into side-effects-only-2
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
framework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.java (2)
145-162: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMisleading diagnostic arguments — still unfixed.
purity.incorrect.sideeffectsonlyexpects(enclosing method name, offending expression), but this call passesinvokedElem.getSimpleName()(the callee, not the annotated method under check) andsideEffectsOnlyExpressionsFromAnnotation(the entire allow-list, not the specific violation). Previously flagged, unresolved.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.java` around lines 145 - 162, The diagnostic call in the `DisallowedSideEffects` checking flow passes the wrong arguments. Update the `purity.incorrect.sideeffectsonly` report to use the enclosing annotated method’s name and the specific offending expression, rather than `invokedElem.getSimpleName()` and the full `sideEffectsOnlyExpressionsFromAnnotation` allow-list.
134-146: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNo null-check on
invokedElembefore dereferencing.
TreeUtils.elementFromUse(node)can returnnull(e.g., for enum super-constructor calls or unresolved symbols under error recovery).annoProvider.getDeclAnnotation(invokedElem, Pure.class)will NPE in that case.BaseTypeVisitor.visitMethodInvocationalready guards this exact scenario (TreeUtils.elementFromUse(tree) == null || TreeUtils.isEnumSuperCall(tree)); this scanner should do the same before proceeding.🐛 Suggested fix
public Void visitMethodInvocation(MethodInvocationTree node, Void aVoid) { Element invokedElem = TreeUtils.elementFromUse(node); + if (invokedElem == null || TreeUtils.isEnumSuperCall(node)) { + return super.visitMethodInvocation(node, aVoid); + } boolean isMarkedPure = annoProvider.getDeclAnnotation(invokedElem, Pure.class) != null;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.java` around lines 134 - 146, Guard the start of visitMethodInvocation before annotation lookups: return via the existing safe visitor path when TreeUtils.elementFromUse(node) is null or TreeUtils.isEnumSuperCall(node) is true. Only dereference invokedElem and evaluate Pure, SideEffectFree, or SideEffectsOnly annotations after this guard, matching BaseTypeVisitor’s handling.framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java (2)
461-464: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
updateFieldValuesForMethodCallparams still not@Nullabledespite nullable call-site arguments.Previously flagged for both
isSideEffected(now fixed) and this method (still open).receiverJeandsideEffectsOnlyExpressionsare passed possibly-nullvalues from the caller (line 287:receiverJe/seOnlyExpressionscan both benull), but the parameters here aren't annotated@Nullable.🛡️ Proposed fix
private void updateFieldValuesForMethodCall( GenericAnnotatedTypeFactory<V, S, ?, ?> atypeFactory, - JavaExpression receiverJe, - List<JavaExpression> sideEffectsOnlyExpressions) { + `@Nullable` JavaExpression receiverJe, + `@Nullable` List<JavaExpression> sideEffectsOnlyExpressions) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java` around lines 461 - 464, Annotate the receiverJe and sideEffectsOnlyExpressions parameters of updateFieldValuesForMethodCall with `@Nullable`, matching the nullable values passed by its caller. Preserve the method’s existing behavior and handling of null arguments.
273-278: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
thisValueis unrefined without consulting@SideEffectsOnly.Unlike locals/fields/arrays/method-calls (all routed through
isSideEffected(expr, receiverJe, seOnlyExpressions)),thisValueis nulled purely based onsideEffectsUnrefineAliasesand the@DoesNotUnrefineReceiverreceiver exemption — it never checksseOnlyExpressions. When a called method declares a narrow@SideEffectsOnlycontract that doesn't mention anything aboutthis, information aboutthisis still discarded unnecessarily, undercutting the precision this feature is meant to provide.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java` around lines 273 - 278, The thisValue update in CFAbstractStore must consult the `@SideEffectsOnly` expressions before unrefining. Route the receiver through the existing isSideEffected(expr, receiverJe, seOnlyExpressions) logic, while preserving the sideEffectsUnrefineAliases guard and ThisReference/SuperReference exemptions, so thisValue is nulled only when the contract indicates the receiver is side-effected.
♻️ Duplicate comments (1)
framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java (1)
1278-1287: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winStill passes a
Stringas the diagnosticsource.This was flagged previously and remains unfixed:
checker.report(st, new DiagMessage(ex));passesst(aString) assource.SourceChecker.reportonly acceptsElement/Tree, and throwsBugInCFotherwise. UsemethodTree(as the siblingcheckContractsAtMethodDeclarationdoes at line 1501).🐛 Proposed fix
} else { - checker.report(st, new DiagMessage(ex)); + checker.report(methodTree, new DiagMessage(ex)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java` around lines 1278 - 1287, Update the fallback reporting branch in the JavaExpressionParseException catch block to pass methodTree instead of st as the diagnostic source, matching the valid source usage in checkContractsAtMethodDeclaration and preserving the existing diagnostic message handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java`:
- Around line 1154-1160: Update the early-return guard in checkPurityAnnotations
so it also continues when the method has a `@SideEffectsOnly` annotation, using
the existing purity/annotation utility for detection. Preserve the current
suggestPureMethods and `@Pure-related` behavior while ensuring
SideEffectsOnly-only methods reach their validation block.
---
Outside diff comments:
In
`@framework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.java`:
- Around line 145-162: The diagnostic call in the `DisallowedSideEffects`
checking flow passes the wrong arguments. Update the
`purity.incorrect.sideeffectsonly` report to use the enclosing annotated
method’s name and the specific offending expression, rather than
`invokedElem.getSimpleName()` and the full
`sideEffectsOnlyExpressionsFromAnnotation` allow-list.
- Around line 134-146: Guard the start of visitMethodInvocation before
annotation lookups: return via the existing safe visitor path when
TreeUtils.elementFromUse(node) is null or TreeUtils.isEnumSuperCall(node) is
true. Only dereference invokedElem and evaluate Pure, SideEffectFree, or
SideEffectsOnly annotations after this guard, matching BaseTypeVisitor’s
handling.
In
`@framework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java`:
- Around line 461-464: Annotate the receiverJe and sideEffectsOnlyExpressions
parameters of updateFieldValuesForMethodCall with `@Nullable`, matching the
nullable values passed by its caller. Preserve the method’s existing behavior
and handling of null arguments.
- Around line 273-278: The thisValue update in CFAbstractStore must consult the
`@SideEffectsOnly` expressions before unrefining. Route the receiver through the
existing isSideEffected(expr, receiverJe, seOnlyExpressions) logic, while
preserving the sideEffectsUnrefineAliases guard and ThisReference/SuperReference
exemptions, so thisValue is nulled only when the contract indicates the receiver
is side-effected.
---
Duplicate comments:
In
`@framework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.java`:
- Around line 1278-1287: Update the fallback reporting branch in the
JavaExpressionParseException catch block to pass methodTree instead of st as the
diagnostic source, matching the valid source usage in
checkContractsAtMethodDeclaration and preserving the existing diagnostic message
handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 579b3c95-0f4b-4ab6-a0ed-fb775cacb4fd
📒 Files selected for processing (7)
checker/src/main/java/org/checkerframework/checker/lock/LockAnnotatedTypeFactory.javadataflow/src/main/java/org/checkerframework/dataflow/expression/JavaExpression.javadataflow/src/main/java/org/checkerframework/dataflow/util/PurityUtils.javadocs/manual/advanced-features.texframework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.javaframework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.javaframework/src/main/java/org/checkerframework/framework/flow/CFAbstractStore.java
💤 Files with no reviewable changes (1)
- dataflow/src/main/java/org/checkerframework/dataflow/util/PurityUtils.java
…no-factory into side-effects-only-2-pre
There was a problem hiding this comment.
♻️ Duplicate comments (1)
framework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.java (1)
1047-1052: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHonor the type factory's purity overrides in
isStable, asMethodCall.containsAsReceiverdoes.
isStableconsults onlyPurityUtils. It ignores the overrides thatAnnotatedTypeFactory.isDeterministicandAnnotatedTypeFactory.isSideEffectFreeprovide:-AassumeDeterministic,-AassumeSideEffectFree,-AassumePure,-AassumePureGetters, and auto-generated record accessors.Two consequences follow. First,
@SideEffectsOnly("this.component()")on a record component accessor is rejected withpurity.unstable.sideeffectsonly, even though every evaluation of that call yields the same value. The same happens for any getter under-AassumePureGetters.BaseTypeVisitor.checkPurityAnnotationsreturns immediately after that error, so the method body is never checked.Second, this file now disagrees with
MethodCall.containsAsReceiveratdataflow/src/main/java/org/checkerframework/dataflow/expression/MethodCall.javalines 170-172, which was changed in this PR to check both sources:!PurityUtils.isSideEffectFree(provider, method) && !provider.isSideEffectFree(method).isStableandcontainsAsReceivertherefore classify the same call differently.🐛 Proposed fix
} else if (expression instanceof MethodCall methodCall) { ExecutableElement method = methodCall.getElement(); - if (!PurityUtils.isDeterministic(provider, method) - || !PurityUtils.isSideEffectFree(provider, method)) { + if ((!PurityUtils.isDeterministic(provider, method) && !provider.isDeterministic(method)) + || (!PurityUtils.isSideEffectFree(provider, method) + && !provider.isSideEffectFree(method))) { return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@framework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.java` around lines 1047 - 1052, Update isStable’s MethodCall branch to honor AnnotatedTypeFactory overrides alongside PurityUtils for both determinism and side-effect freedom. Treat each property as satisfied when either its PurityUtils result or the corresponding provider result is true, preserving stability for assumed-pure getters and generated record accessors. Align the resulting classification with MethodCall.containsAsReceiver.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In
`@framework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.java`:
- Around line 1047-1052: Update isStable’s MethodCall branch to honor
AnnotatedTypeFactory overrides alongside PurityUtils for both determinism and
side-effect freedom. Treat each property as satisfied when either its
PurityUtils result or the corresponding provider result is true, preserving
stability for assumed-pure getters and generated record accessors. Align the
resulting classification with MethodCall.containsAsReceiver.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0a169fe7-70ea-4ff8-a584-1278b8823a1a
📒 Files selected for processing (34)
checker/bin-devel/Dockerfile-READMEchecker/bin-devel/Dockerfile-contents-ubuntu-base.m4checker/bin-devel/Dockerfile-ubuntu-jdk17checker/bin-devel/Dockerfile-ubuntu-jdk17-pluschecker/bin-devel/Dockerfile-ubuntu-jdk21checker/bin-devel/Dockerfile-ubuntu-jdk21-pluschecker/bin-devel/Dockerfile-ubuntu-jdk25checker/bin-devel/Dockerfile-ubuntu-jdk25-pluschecker/bin-devel/Dockerfile-ubuntu-jdk26checker/bin-devel/Dockerfile-ubuntu-jdk26-pluschecker/bin-devel/Dockerfile-ubuntu-jdkbasechecker/bin-devel/Dockerfile-ubuntu-jdkpluschecker/bin-devel/Dockerfile-ubunturolling-jdkbasechecker/bin-devel/Dockerfile-ubunturolling-jdkpluschecker/tests/sideeffectsonly-stubfile/Library.javachecker/tests/sideeffectsonly-stubfile/UseSiteParseError.javachecker/tests/sideeffectsonly-stubfile/seonly.astubchecker/tests/sideeffectsonly/AnnotationInBody.javachecker/tests/sideeffectsonly/DesugaredCalls.javachecker/tests/sideeffectsonly/FreshValueAliasing.javachecker/tests/sideeffectsonly/ImplicitConstructorCode.javachecker/tests/sideeffectsonly/LambdaUnstableSideEffectsOnly.javachecker/tests/sideeffectsonly/NestedCodeSeonly.javachecker/tests/sideeffectsonly/ThisSubexpressionInAnnotation.javachecker/tests/sideeffectsonly/UnrepresentableArgument.javadataflow/src/main/java/org/checkerframework/dataflow/cfg/visualize/CFGVisualizeLauncher.javadataflow/src/main/java/org/checkerframework/dataflow/expression/ArrayAccess.javadataflow/src/main/java/org/checkerframework/dataflow/expression/FieldAccess.javadataflow/src/main/java/org/checkerframework/dataflow/expression/JavaExpression.javadataflow/src/main/java/org/checkerframework/dataflow/expression/MethodCall.javadocs/manual/advanced-features.texframework/src/main/java/org/checkerframework/common/basetype/BaseTypeVisitor.javaframework/src/main/java/org/checkerframework/common/basetype/DisallowedSideEffects.javaframework/src/main/java/org/checkerframework/framework/flow/CFAbstractAnalysis.java
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/manual/called-methods-checker.tex (1)
222-234: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDocument
@SideEffectsOnlyas a conditional refinement guarantee.
@SideEffectsOnlybounds effects but does not preserve every flow fact. The listed expressions must not affect the expression whose refinement the caller needs to preserve.
docs/manual/called-methods-checker.tex#L222-L234: state that the effect list must not invalidate thesock.close()fact.docs/manual/troubleshooting.tex#L350-L350: state that the effect list must not affectcurrentOutgoing.This follows the
@SideEffectsOnlycontract documented indocs/manual/advanced-features.tex.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/manual/called-methods-checker.tex` around lines 222 - 234, Update the Called Methods Checker guidance in docs/manual/called-methods-checker.tex lines 222-234 to state that the `@SideEffectsOnly` effect list must not invalidate the refinement fact that sock.close() was called; update docs/manual/troubleshooting.tex line 350 to state that its effect list must not affect currentOutgoing, consistent with the `@SideEffectsOnly` contract in advanced-features.tex.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/manual/called-methods-checker.tex`:
- Around line 222-234: Update the Called Methods Checker guidance in
docs/manual/called-methods-checker.tex lines 222-234 to state that the
`@SideEffectsOnly` effect list must not invalidate the refinement fact that
sock.close() was called; update docs/manual/troubleshooting.tex line 350 to
state that its effect list must not affect currentOutgoing, consistent with the
`@SideEffectsOnly` contract in advanced-features.tex.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b12ac3b7-d170-49a3-b86f-7a7694738b4a
📒 Files selected for processing (7)
docs/CHANGELOG.mddocs/manual/advanced-features.texdocs/manual/called-methods-checker.texdocs/manual/introduction.texdocs/manual/nullness-checker.texdocs/manual/purity-checker.texdocs/manual/troubleshooting.tex
…work into side-effects-only-2
1 participant
Merge with typetools/jdk#282.