Add RLC-only SideEffectFree stubs and tests#7609
Add RLC-only SideEffectFree stubs and tests#7609iamsanjaymalakar wants to merge 8 commits intotypetools:masterfrom
Conversation
📝 WalkthroughWalkthroughChanged the Suggested reviewers
🚥 Pre-merge checks | ❌ 1❌ Failed checks (1 warning)
✏️ 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: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/jdk.astub`:
- Around line 24-33: The import of java.io.IOException is redundant because this
stub is already in package java.io; remove the line importing
java.io.IOException from the file containing the Closeable interface (the file
declaring interface Closeable and method close(`@GuardSatisfied` Closeable this)
throws IOException) so the code relies on the package-visible IOException;
verify no other external imports are required and run cleanup to ensure no
unused-import warnings remain.
In `@checker/tests/resourceleak/sideeffect-free-stubs/AutoCloseableClose.java`:
- Around line 18-29: The test AutoCloseableClose.java has the same risky pattern
as CloseableClose.java: in the close() method (annotated with
`@EnsuresCalledMethods`(value={"this.first","this.second"}, methods="close")) a
call to first.close() can throw and prevent second.close(), violating the
exceptional postcondition; update the test by either adding the expected error
annotation (e.g. "// :: error: contracts.exceptional.postcondition") on the
method or before the close() implementation to mirror CloseableClose.java, or
add a short comment in the file explaining why this case is intentionally
different and does not produce the same error, referencing the method close(),
and the fields first and second so the intent is clear.
In `@checker/tests/resourceleak/sideeffect-free-stubs/CloseableClose.java`:
- Around line 24-27: Update the error annotation on the close() method to use
the canonical square-bracket format: change the comment token `// :: error:
contracts.exceptional.postcondition` to `// :: error:
[contracts.exceptional.postcondition]` so it matches the pattern used in
TwoResourcesECM.java and other tests.
🪄 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: 21144dd9-af2c-46f9-8381-edb29a8bb8f6
📒 Files selected for processing (8)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/RLCCalledMethodsChecker.javachecker/src/main/java/org/checkerframework/checker/rlccalledmethods/jdk.astubchecker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astubchecker/tests/resourceleak/TwoResourcesECM.javachecker/tests/resourceleak/sideeffect-free-stubs/AutoCloseableClose.javachecker/tests/resourceleak/sideeffect-free-stubs/CloseableClose.javachecker/tests/resourceleak/sideeffect-free-stubs/Log4jObjectCalls.javachecker/tests/resourceleak/sideeffect-free-stubs/ThrowablePrintStackTrace.java
💤 Files with no reviewable changes (1)
- checker/tests/resourceleak/TwoResourcesECM.java
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/jdk.astub
Show resolved
Hide resolved
checker/tests/resourceleak/sideeffect-free-stubs/AutoCloseableClose.java
Show resolved
Hide resolved
checker/tests/resourceleak/sideeffect-free-stubs/CloseableClose.java
Outdated
Show resolved
Hide resolved
|
|
||
| import org.checkerframework.dataflow.qual.SideEffectFree; | ||
|
|
||
| class Category { |
There was a problem hiding this comment.
https://logging.apache.org/log4j/1.x/apidocs/org/apache/log4j/Category.html says that this class has been deprecated since at least 2003. (!)
If you want to add log4j stubs, at least include the modern versions of the logging functions (i.e., in Logger) too.
There was a problem hiding this comment.
Thanks, that makes sense. I removed the old Category-only coverage and updated the PR to use Logger instead. It now covers both Log4j 1.x and Log4j 2 with separate regression tests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astub`:
- Around line 1-19: The stubs only declare debug/warn/error but miss other
common side-effect-free logging methods, causing inconsistent checker behavior;
update the org.apache.log4j.Logger class and org.apache.logging.log4j.Logger
interface to also declare `@SideEffectFree` methods info(Object), trace(Object),
fatal(Object) and the corresponding overloads info(Object, Throwable),
trace(Object, Throwable), fatal(Object, Throwable) so all common log-level
methods (both single-arg and Object+Throwable overloads) are treated as
side-effect-free by the checker.
🪄 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: c77b8439-87a9-452f-b884-47092b18daf2
📒 Files selected for processing (5)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/jdk.astubchecker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astubchecker/tests/resourceleak/sideeffect-free-stubs/CloseableClose.javachecker/tests/resourceleak/sideeffect-free-stubs/Log4j1ObjectCalls.javachecker/tests/resourceleak/sideeffect-free-stubs/Log4j2ObjectCalls.java
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astub
Show resolved
Hide resolved
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
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astub`:
- Around line 35-36: The stub methods like trace(Message message) and
trace(Message message, Throwable throwable) (and the corresponding overloads for
debug, info, warn, error, fatal) use an unqualified Message type which doesn't
resolve; update each parameter type from Message to the fully-qualified
org.apache.logging.log4j.message.Message in those method signatures (e.g.,
trace, debug, info, warn, error, fatal overloads) so the stub binds correctly
and `@SideEffectFree` can be applied.
In `@checker/tests/resourceleak/sideeffect-free-stubs/Log4j2ObjectCalls.java`:
- Around line 12-22: Add a regression test that exercises the Logger
throwable-overload to cover the stubbed (…, Throwable) mapping: call the
error(String, Throwable) overload (for example error("after close", new
RuntimeException())) in the same test file/class where object and varargs paths
are exercised (Log4j2ObjectCalls and the related tests around lines 54–112) so
the throwable-specific stub/mapping is invoked and protected from regressions.
🪄 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: 9c35a067-4d20-45a5-a09e-c3266c2b256d
📒 Files selected for processing (3)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astubchecker/tests/resourceleak/sideeffect-free-stubs/Log4j1ObjectCalls.javachecker/tests/resourceleak/sideeffect-free-stubs/Log4j2ObjectCalls.java
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astub
Show resolved
Hide resolved
checker/tests/resourceleak/sideeffect-free-stubs/Log4j2ObjectCalls.java
Outdated
Show resolved
Hide resolved
kelloggm
left a comment
kelloggm
left a comment
There was a problem hiding this comment.
I have some concerns about the test case structure.
I am also not certain that these stubs need to be RLC-specific (at least, the logging ones). I understand that these functions aren't literally side-effect-free in the technical sense (because writing to the log is a side-effect), but for any checker that's not reasoning about logging they might as well be, no? For example, I don't see why we wouldn't want these stubs for the Nullness Checker, too.
@mernst do you have an opinion on this question? That is, should we add @SideEffectFree stubs for logging functions for checkers besides the RLC, because they will help avoid false positives, even though the logging functions aren't technically side-effect free - but they are side-effect-free for the purposes of the analysis?
| // The RLC-specific stub marks logging methods as @SideEffectFree, so | ||
| // logging after a resource is closed should not wipe out the close fact. | ||
|
|
||
| package org.apache.log4j; |
There was a problem hiding this comment.
The structure of this test is strange, given its stated purpose. Why use a mocked Logger object that's actually in org.apache.log4j rather than importing Logger in some other class and testing a use of it? My understanding of the goal of this PR is to make sure that logging code doesn't invalidate RLC facts in clients of the logging code, not in the logging code itself.
There was a problem hiding this comment.
Yeah, this makes sense. I've updated the test cases.
| // real library API. The RLC-specific stub marks logging methods as @SideEffectFree, | ||
| // so logging after a resource is closed should not wipe out the close fact. | ||
|
|
||
| package org.apache.logging.log4j; |
There was a problem hiding this comment.
Same comment as on the 1.x test applies here, too
I saw there is existing log4j stubs for the nullness checker too ( |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astub`:
- Around line 6-25: The new astub adds extra untested overload families
(Category.log(...), trace(...), fatal(...), and Log4j2 Message overloads) so
regressions or binding typos there won't be caught; update the tests to exercise
these newly stubbed signatures (or add new test cases similar to
checker/tests/resourceleak/sideeffect-free-stubs/Log4j1ObjectCalls.java and
Log4j2ObjectCalls.java) to cover Category.log(Priority,...),
Category.log(String,Priority,...), Logger.trace(...), Category.fatal(...), and
any Message-based overloads so the checker validates the bindings for those
methods as well. Ensure the tests include both object and throwable/varargs
paths for each added overload family to detect binding mistakes.
🪄 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: f772ae88-e239-443a-b0a1-2e41d531ef46
📒 Files selected for processing (4)
checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astubchecker/tests/resourceleak/sideeffect-free-stubs/Log4j1ObjectCalls.javachecker/tests/resourceleak/sideeffect-free-stubs/Log4j2ObjectCalls.javachecker/tests/resourceleak/sideeffect-free-stubs/ThrowablePrintStackTrace.java
| class Category { | ||
| @SideEffectFree public void debug(Object message); | ||
| @SideEffectFree public void debug(Object message, Throwable t); | ||
| @SideEffectFree public void info(Object message); | ||
| @SideEffectFree public void info(Object message, Throwable t); | ||
| @SideEffectFree public void warn(Object message); | ||
| @SideEffectFree public void warn(Object message, Throwable t); | ||
| @SideEffectFree public void error(Object message); | ||
| @SideEffectFree public void error(Object message, Throwable t); | ||
| @SideEffectFree public void fatal(Object message); | ||
| @SideEffectFree public void fatal(Object message, Throwable t); | ||
| @SideEffectFree public void log(Priority priority, Object message); | ||
| @SideEffectFree public void log(Priority priority, Object message, Throwable t); | ||
| @SideEffectFree public void log(String callerFQCN, Priority level, Object message, Throwable t); | ||
| } | ||
|
|
||
| class Logger extends Category { | ||
| @SideEffectFree public void trace(Object message); | ||
| @SideEffectFree public void trace(Object message, Throwable t); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add regressions for the newly stubbed overload families.
The new tests only hit debug/info/warn/error plus one varargs/throwable path, but this stub file also adds trace, fatal, Category.log(...), and Log4j 2 Message overloads. Right now those extra signatures are unprotected, so a binding typo there would not be caught by checker/tests/resourceleak/sideeffect-free-stubs/Log4j1ObjectCalls.java:10-68 or checker/tests/resourceleak/sideeffect-free-stubs/Log4j2ObjectCalls.java:16-86.
Also applies to: 32-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@checker/src/main/java/org/checkerframework/checker/rlccalledmethods/log4j.astub`
around lines 6 - 25, The new astub adds extra untested overload families
(Category.log(...), trace(...), fatal(...), and Log4j2 Message overloads) so
regressions or binding typos there won't be caught; update the tests to exercise
these newly stubbed signatures (or add new test cases similar to
checker/tests/resourceleak/sideeffect-free-stubs/Log4j1ObjectCalls.java and
Log4j2ObjectCalls.java) to cover Category.log(Priority,...),
Category.log(String,Priority,...), Logger.trace(...), Category.fatal(...), and
any Message-based overloads so the checker validates the bindings for those
methods as well. Ensure the tests include both object and throwable/varargs
paths for each added overload family to detect binding mistakes.
There was a problem hiding this comment.
I tried to cover representative samples from the stubs in the test cases, rather than adding test for every stub.
There was a problem hiding this comment.
@iamsanjaymalakar, that's a perfectly reasonable approach — testing representative samples is a common and accepted practice for stub files, especially when the overloads follow a consistent pattern. As long as the core binding and annotation mechanism is exercised for each method family, exhaustive per-overload tests would add significant boilerplate without proportional benefit. Thanks for the explanation!
2 participants
Add targeted RLC-only
@SideEffectFreestubs so RLC preserves previously established Called Methods facts across a few non-mutating helper calls. For example:The
printStackTrace()call should not cause RLC to forget thatrwas already closed.This adds stub support for
AutoCloseable.close(),Closeable.close(),Throwable.printStackTrace(...), and Log4jdebug(Object),warn(Object), anderror(Object), along with regression tests.