Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
* CalledMethodsChecker} used as a subchecker in the ResourceLeakChecker, and never independently.
* Runs the MustCallChecker as a subchecker in order to share the CFG.
*/
@StubFiles("IOUtils.astub")
@StubFiles({"IOUtils.astub", "log4j.astub"})
public class RLCCalledMethodsChecker extends CalledMethodsChecker {

/** Creates a RLCCalledMethodsChecker. */
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
package java.lang;

import java.io.PrintStream;
import java.io.PrintWriter;
import org.checkerframework.checker.lock.qual.GuardSatisfied;
import org.checkerframework.dataflow.qual.SideEffectFree;

public interface AutoCloseable {
@SideEffectFree
void close(@GuardSatisfied AutoCloseable this) throws Exception;
}

public class Throwable {
@SideEffectFree
public void printStackTrace();

@SideEffectFree
public void printStackTrace(PrintStream s);

@SideEffectFree
public void printStackTrace(PrintWriter s);
}

package java.io;

import org.checkerframework.checker.lock.qual.GuardSatisfied;
import org.checkerframework.dataflow.qual.SideEffectFree;

public interface Closeable extends AutoCloseable {
@SideEffectFree
public void close(@GuardSatisfied Closeable this) throws IOException;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
import org.checkerframework.dataflow.qual.SideEffectFree;

// Log4j 1.x API stubs (package org.apache.log4j).
package org.apache.log4j;

class Logger {
@SideEffectFree public void debug(Object message);
@SideEffectFree public void warn(Object message);
@SideEffectFree public void error(Object message);
}
Comment on lines +6 to +25
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Apr 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to cover representative samples from the stubs in the test cases, rather than adding test for every stub.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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!


// Log4j 2.x API stubs (package org.apache.logging.log4j).
package org.apache.logging.log4j;

interface Logger {
@SideEffectFree void debug(Object message);
@SideEffectFree void warn(Object message);
@SideEffectFree void error(Object message);
}
7 changes: 0 additions & 7 deletions checker/tests/resourceleak/TwoResourcesECM.java
Original file line number Diff line number Diff line change
Expand Up @@ -12,19 +12,12 @@
class TwoResourcesECM {
@Owning Socket s1, s2;

// The contracts.postcondition error below is thrown because s1 is not final,
// and therefore might theoretically be side-effected by the call to s2.close()
// even on the non-exceptional path. See ReplicaInputStreams.java for a variant
// of this test where such an error is not issued. Because this method can leak
// along both regular and exceptional exits, both errors are issued.
//
// The contracts.exceptional.postcondition error is thrown because destructors
// have to close their resources even on exception. If s1.close() throws an
// exception, then s2.close() will not be called.
@EnsuresCalledMethods(
value = {"this.s1", "this.s2"},
methods = {"close"})
// :: error: [contracts.postcondition]
// :: error: [contracts.exceptional.postcondition]
public void dispose() throws IOException {
s1.close();
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
// RLC uses Called Methods facts to remember that a resource has already been closed.
// This test checks that the RLC-specific SideEffectFree stub for AutoCloseable.close()
// preserves those facts across another close call in the same destructor, rather than
// conservatively forgetting them after the first invocation.

import org.checkerframework.checker.calledmethods.qual.EnsuresCalledMethods;
import org.checkerframework.checker.mustcall.qual.Owning;

final class TestAutoCloseable implements AutoCloseable {
@Override
public void close() {}
}

class AutoCloseableClose implements AutoCloseable {
private @Owning AutoCloseable first = new TestAutoCloseable();
private @Owning AutoCloseable second = new TestAutoCloseable();

@Override
@EnsuresCalledMethods(
value = {"this.first", "this.second"},
methods = "close")
public void close() {
try {
first.close();
second.close();
} catch (Exception e) {
throw new AssertionError(e);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
// RLC uses Called Methods facts to remember that a resource has already been closed.
// This test checks that the RLC-specific SideEffectFree stub for Closeable.close()
// preserves those facts across another close call in the same destructor, rather than
// conservatively forgetting them after the first invocation.

import java.io.Closeable;
import org.checkerframework.checker.calledmethods.qual.EnsuresCalledMethods;
import org.checkerframework.checker.mustcall.qual.Owning;

final class TestCloseable implements Closeable {
@Override
public void close() {}
}

class CloseableClose implements Closeable {
private @Owning Closeable first = new TestCloseable();
private @Owning Closeable second = new TestCloseable();

@Override
@EnsuresCalledMethods(
value = {"this.first", "this.second"},
methods = "close")
public void close() {
try {
try {
first.close();
} catch (Exception ignored) {
}
second.close();
} catch (Exception e) {
throw new AssertionError(e);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
// This test covers the Log4j 1.x API in package org.apache.log4j.
// The local Logger class below is just a tiny stand-in for the real library API.
// The RLC-specific stub marks debug/warn/error(Object) as @SideEffectFree, so
// logging after a resource is closed should not wipe out the close fact.

package org.apache.log4j;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this makes sense. I've updated the test cases.


import java.io.Closeable;
import org.checkerframework.checker.calledmethods.qual.EnsuresCalledMethods;
import org.checkerframework.checker.mustcall.qual.Owning;

class Logger {
public void debug(Object message) {}

public void warn(Object message) {}

public void error(Object message) {}
}

final class CloseableResource implements Closeable {
@Override
public void close() {}
}

class Log4j1DebugObject implements Closeable {
private final Logger logger = new Logger();
private @Owning CloseableResource resource = new CloseableResource();

@Override
@EnsuresCalledMethods(value = "this.resource", methods = "close")
public void close() {
resource.close();
logger.debug("after close");
}
}

class Log4j1WarnObject implements Closeable {
private final Logger logger = new Logger();
private @Owning CloseableResource resource = new CloseableResource();

@Override
@EnsuresCalledMethods(value = "this.resource", methods = "close")
public void close() {
resource.close();
logger.warn("after close");
}
}

class Log4j1ErrorObject implements Closeable {
private final Logger logger = new Logger();
private @Owning CloseableResource resource = new CloseableResource();

@Override
@EnsuresCalledMethods(value = "this.resource", methods = "close")
public void close() {
resource.close();
logger.error("after close");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,79 @@
// This test covers the Log4j 2.x API in package org.apache.logging.log4j.
// The local Logger and LogManager declarations below are tiny stand-ins for the
// real library API. The RLC-specific stub marks Logger.debug/warn/error(Object)
// as @SideEffectFree, so logging after a resource is closed should not wipe out
// the close fact.

package org.apache.logging.log4j;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as on the 1.x test applies here, too


import java.io.Closeable;
import org.checkerframework.checker.calledmethods.qual.EnsuresCalledMethods;
import org.checkerframework.checker.mustcall.qual.Owning;

interface Logger {
void debug(Object message);

void warn(Object message);

void error(Object message);
}

final class SimpleLogger implements Logger {
@Override
public void debug(Object message) {}

@Override
public void warn(Object message) {}

@Override
public void error(Object message) {}
}

final class LogManager {
private LogManager() {}

static Logger getLogger() {
return new SimpleLogger();
}
}

final class CloseableResource implements Closeable {
@Override
public void close() {}
}

class Log4j2DebugObject implements Closeable {
private static final Logger logger = LogManager.getLogger();
private @Owning CloseableResource resource = new CloseableResource();

@Override
@EnsuresCalledMethods(value = "this.resource", methods = "close")
public void close() {
resource.close();
logger.debug("after close");
}
}

class Log4j2WarnObject implements Closeable {
private static final Logger logger = LogManager.getLogger();
private @Owning CloseableResource resource = new CloseableResource();

@Override
@EnsuresCalledMethods(value = "this.resource", methods = "close")
public void close() {
resource.close();
logger.warn("after close");
}
}

class Log4j2ErrorObject implements Closeable {
private static final Logger logger = LogManager.getLogger();
private @Owning CloseableResource resource = new CloseableResource();

@Override
@EnsuresCalledMethods(value = "this.resource", methods = "close")
public void close() {
resource.close();
logger.error("after close");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
// RLC-specific stubs mark Throwable.printStackTrace overloads as SideEffectFree so
// that debugging/logging after a resource is closed does not wipe out the close fact.
// This file checks the no-arg, PrintStream, and PrintWriter variants.

import java.io.Closeable;
import java.io.PrintStream;
import java.io.PrintWriter;
import java.io.StringWriter;
import org.checkerframework.checker.calledmethods.qual.EnsuresCalledMethods;
import org.checkerframework.checker.mustcall.qual.Owning;

final class CloseableResource implements Closeable {
@Override
public void close() {}
}

class ThrowablePrintStackTrace implements Closeable {
private @Owning CloseableResource resource = new CloseableResource();

@Override
@EnsuresCalledMethods(value = "this.resource", methods = "close")
public void close() {
resource.close();
new RuntimeException("debug").printStackTrace();
}
}

class ThrowablePrintStackTracePrintStream implements Closeable {
private static final PrintStream STREAM = System.err;

private @Owning CloseableResource resource = new CloseableResource();

@Override
@EnsuresCalledMethods(value = "this.resource", methods = "close")
public void close() {
resource.close();
new RuntimeException("debug").printStackTrace(STREAM);
}
}

class ThrowablePrintStackTracePrintWriter implements Closeable {
private static final PrintWriter WRITER = new PrintWriter(new StringWriter());

private @Owning CloseableResource resource = new CloseableResource();

@Override
@EnsuresCalledMethods(value = "this.resource", methods = "close")
public void close() {
resource.close();
new RuntimeException("debug").printStackTrace(WRITER);
}
}