Make close() be side-effect-free#267
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 53 minutes and 45 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/java.base/share/classes/java/io/Closeable.java`:
- Line 30: Remove the incorrect purity annotation: delete the import of
org.checkerframework.dataflow.qual.SideEffectFree and remove the `@SideEffectFree`
annotation from the Closeable.close() method declaration (referencing the
Closeable interface and its close() method) so that resource-release behavior is
not misrepresented to the Checker Framework; ensure no other references to that
import remain in the file and leave MustCallChecker handling of close semantics
unchanged.
In `@src/java.base/share/classes/java/lang/AutoCloseable.java`:
- Line 29: Remove the `@SideEffectFree` annotation and its import from the
AutoCloseable interface: delete the import statement
org.checkerframework.dataflow.qual.SideEffectFree and remove the `@SideEffectFree`
annotation applied to the AutoCloseable.close() declaration in the AutoCloseable
interface so the method signature and Javadoc remain but no purity annotation is
present (referenced symbols: AutoCloseable, AutoCloseable.close(), import
org.checkerframework.dataflow.qual.SideEffectFree).
🪄 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: 89ff1f46-abbd-4573-81ff-aa173f468356
📒 Files selected for processing (2)
src/java.base/share/classes/java/io/Closeable.javasrc/java.base/share/classes/java/lang/AutoCloseable.java
|
|
||
| import org.checkerframework.checker.lock.qual.GuardSatisfied; | ||
| import org.checkerframework.checker.mustcall.qual.InheritableMustCall; | ||
| import org.checkerframework.dataflow.qual.SideEffectFree; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Checker Framework SideEffectFree annotation side effects resource close method
💡 Result:
The @SideEffectFree annotation in the Checker Framework indicates that a method has no visible side effects, meaning it does not modify fields of pre-existing objects. This is crucial for flow-sensitive type refinement, as calling a @SideEffectFree method preserves facts known about objects before the call. Regarding resource close methods (e.g., Closeable.close or AutoCloseable.close), these typically have side effects—they release system resources, which may involve closing file handles, network connections, or other OS resources. Such actions modify the object's internal state or external resources, violating the "no visible side effects" rule. Therefore, close methods are not annotated @SideEffectFree and cannot be without misrepresenting their behavior. The Framework's conservative purity analysis (enabled via -AcheckPurityAnnotations) flags violations like assignments to fields/arrays or calls to non-@SideEffectFree methods. Close methods would fail this check due to their side effects. Resource management is handled separately by the Resource Leak Checker (MustCallChecker), not purity annotations. Close methods fulfill "must-call" obligations but are incompatible with @SideEffectFree. No official sources annotate standard close methods as @SideEffectFree; doing so would be incorrect.
Citations:
- 1: https://checkerframework.org/api/org/checkerframework/dataflow/qual/SideEffectFree.html
- 2: https://checkerframework.org/releases/3.6.1/api/org/checkerframework/dataflow/qual/SideEffectFree.html
- 3: https://checkerframework.org/releases/3.27.0/api/org/checkerframework/dataflow/qual/SideEffectFree.html
- 4: https://checkerframework.org/manual
- 5: https://checkerframework.org/api/org/checkerframework/dataflow/util/PurityChecker.html
- 6: https://checkerframework.org/api/org/checkerframework/dataflow/util/PurityUtils.html
- 7: Resource leak checker does not seem to honor try-with-resources in some cases checker-framework#6037
- 8: https://checkerframework.org/releases/3.47.0/api/org/checkerframework/checker/resourceleak/ResourceLeakChecker.html
@SideEffectFree annotation is incorrect for resource-release methods.
The @SideEffectFree annotation indicates a method has no visible side effects—it does not modify the state of pre-existing objects. However, Closeable.close() is documented to release system resources (lines 48–50), which is a visible side effect. The annotation misrepresents the method's behavior to the Checker Framework's purity analysis. Resource management for close() methods is handled separately via the MustCallChecker (which tracks must-call obligations), not purity annotations. Standard close methods should not be annotated @SideEffectFree.
Remove the @SideEffectFree import and annotation from Closeable.close().
Proposed fix
import org.checkerframework.checker.lock.qual.GuardSatisfied;
import org.checkerframework.checker.mustcall.qual.InheritableMustCall;
-import org.checkerframework.dataflow.qual.SideEffectFree;
import org.checkerframework.framework.qual.AnnotatedFor;
import org.checkerframework.framework.qual.CFComment;
@@
`@CFComment`({"lock: Note that the `@GuardSatisfied` is for locks that are external to the implementation of close(). The close() method itself could release locks if it wanted to."})
- `@SideEffectFree`
public void close(`@GuardSatisfied` Closeable this) throws IOException;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| import org.checkerframework.dataflow.qual.SideEffectFree; | |
| import org.checkerframework.checker.lock.qual.GuardSatisfied; | |
| import org.checkerframework.checker.mustcall.qual.InheritableMustCall; | |
| import org.checkerframework.framework.qual.AnnotatedFor; | |
| import org.checkerframework.framework.qual.CFComment; |
| import org.checkerframework.dataflow.qual.SideEffectFree; | |
| `@CFComment`({"lock: Note that the `@GuardSatisfied` is for locks that are external to the implementation of close(). The close() method itself could release locks if it wanted to."}) | |
| public void close(`@GuardSatisfied` Closeable this) throws IOException; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/java.base/share/classes/java/io/Closeable.java` at line 30, Remove the
incorrect purity annotation: delete the import of
org.checkerframework.dataflow.qual.SideEffectFree and remove the `@SideEffectFree`
annotation from the Closeable.close() method declaration (referencing the
Closeable interface and its close() method) so that resource-release behavior is
not misrepresented to the Checker Framework; ensure no other references to that
import remain in the file and leave MustCallChecker handling of close semantics
unchanged.
| package java.lang; | ||
|
|
||
| import org.checkerframework.checker.lock.qual.GuardSatisfied; | ||
| import org.checkerframework.dataflow.qual.SideEffectFree; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Checker Framework org.checkerframework.dataflow.qual.SideEffectFree semantics method visible side effects receiver mutation
💡 Result:
The @SideEffectFree annotation in org.checkerframework.dataflow.qual indicates that a method or constructor has no visible side effects. Visible side effects are changes to objects that existed before the method call, such as setting fields on the receiver object ('this') or other pre-existing objects. Key semantics: - No field assignments to pre-existing objects (e.g., this.f = ... or obj.f = ... where obj existed before the call). - Allowed: assignments to local variables, method parameters, or newly-created objects. - Array stores (e.g., a[i] = x) are forbidden as they modify pre-existing arrays. - Method calls and constructor invocations must also be @SideEffectFree. - Receiver mutation: Mutating 'this' (non-local, non-parameter) is a visible side effect, so forbidden. The receiver ('this') existed before the call. Purpose: Enables flow-sensitive type refinement. After a @SideEffectFree call, type qualifiers on pre-existing objects remain valid, even for non-final fields. Non-@SideEffectFree calls may invalidate qualifiers on any accessible object. Verification: Conservative static analysis (enable with -AcheckPurityAnnotations). Warns on forbidden constructs but may false-positive (e.g., safe caching rejected). From official API docs (latest version 3.53.1 as of results). PurityChecker verifies @SideEffectFree, @Deterministic, @Pure usage in dataflow.
Citations:
- 1: https://checkerframework.org/api/org/checkerframework/dataflow/qual/SideEffectFree.html
- 2: https://checkerframework.org/releases/3.27.0/api/org/checkerframework/dataflow/qual/SideEffectFree.html
- 3: https://checkerframework.org/releases/2.4.0/api/org/checkerframework/dataflow/qual/SideEffectFree.html
- 4: https://checkerframework.org/api/org/checkerframework/dataflow/qual/package-summary.html
- 5: https://checkerframework.org/releases/3.27.0/api/org/checkerframework/dataflow/util/PurityChecker.html
- 6: https://checkerframework.org/api/org/checkerframework/dataflow/util/PurityChecker.html
🏁 Script executed:
cat -n src/java.base/share/classes/java/lang/AutoCloseable.java | head -120Repository: typetools/jdk
Length of output: 6230
Remove @SideEffectFree annotation from AutoCloseable.close() and its import.
The @SideEffectFree annotation directly contradicts the method's documented contract. Lines 96–101 explicitly state that close() is not required to be idempotent and may have visible side effects when called multiple times. Additionally, implementers are advised (lines 74–79) to internally mark the resource as closed, which requires receiver mutation—a forbidden operation under @SideEffectFree. Annotating this root interface method misleads downstream Checker Framework analysis into trusting side-effectful implementations as purity-safe.
Proposed fix
-import org.checkerframework.dataflow.qual.SideEffectFree;
import org.checkerframework.framework.qual.AnnotatedFor;
import org.checkerframework.framework.qual.CFComment;- `@SideEffectFree`
void close(`@GuardSatisfied` AutoCloseable this) throws Exception;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/java.base/share/classes/java/lang/AutoCloseable.java` at line 29, Remove
the `@SideEffectFree` annotation and its import from the AutoCloseable interface:
delete the import statement org.checkerframework.dataflow.qual.SideEffectFree
and remove the `@SideEffectFree` annotation applied to the AutoCloseable.close()
declaration in the AutoCloseable interface so the method signature and Javadoc
remain but no purity annotation is present (referenced symbols: AutoCloseable,
AutoCloseable.close(), import
org.checkerframework.dataflow.qual.SideEffectFree).
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)
src/java.base/share/classes/java/lang/Throwable.java (1)
668-772:⚠️ Potential issue | 🟡 MinorOmit
@SideEffectFreefromprintStackTracemethods—they perform observable I/O side effects.The
@SideEffectFreeannotation asserts that a method has no visible side effects on pre-existing objects. All four annotated methods here violate that contract:
printStackTrace()andprintStackTrace(PrintStream)write toSystem.error a caller-supplied stream, mutating its internal buffers and external state.printStackTrace(PrintWriter)similarly mutates the supplied stream.- The private
printStackTrace(PrintStreamOrWriter)helper acquires locks on pre-existing objects, creating observable state changes.Per Checker Framework documentation, I/O operations like
printlnand stream writes are not suitable for@SideEffectFreeannotation. The recommended practice is to omit the annotation, allowing the framework to conservatively treat these methods as having side effects. This ensures proper flow-sensitive type refinement for other code paths.Consider removing these annotations and, if needed, using checker options like
@SuppressWarningsor-AassumeSideEffectFreeat call sites that conflict with purity requirements.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/java.base/share/classes/java/lang/Throwable.java` around lines 668 - 772, Remove the incorrect `@SideEffectFree` annotation from the four stack-trace printing methods so they are not marked pure: public void printStackTrace(), public void printStackTrace(PrintStream), private void printStackTrace(PrintStreamOrWriter), and public void printStackTrace(PrintWriter); simply delete the `@SideEffectFree` annotation instances on these method declarations so the methods (which perform I/O and locking) are no longer claimed to be side-effect-free.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/java.base/share/classes/java/lang/Throwable.java`:
- Around line 668-772: Remove the incorrect `@SideEffectFree` annotation from the
four stack-trace printing methods so they are not marked pure: public void
printStackTrace(), public void printStackTrace(PrintStream), private void
printStackTrace(PrintStreamOrWriter), and public void
printStackTrace(PrintWriter); simply delete the `@SideEffectFree` annotation
instances on these method declarations so the methods (which perform I/O and
locking) are no longer claimed to be side-effect-free.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1109d580-a14d-4d81-91ea-9ee95bfb26e4
📒 Files selected for processing (1)
src/java.base/share/classes/java/lang/Throwable.java
1 participant
Merge with typetools/checker-framework#7655