Skip to content

Add PMD rule for inline closeable resources#1617

Open
GHX5T-SOL wants to merge 1 commit into
yegor256:masterfrom
GHX5T-SOL:1592
Open

Add PMD rule for inline closeable resources#1617
GHX5T-SOL wants to merge 1 commit into
yegor256:masterfrom
GHX5T-SOL:1592

Conversation

@GHX5T-SOL

Copy link
Copy Markdown
Contributor

Closes #1592.

Changes:

  • Added CloseInlineResourceRule to flag inline AutoCloseable constructor calls and resource-propagating method-call chains that are consumed outside local/resource boundaries.
  • Registered the rule in the Qulice PMD ruleset.
  • Added fixtures for the leaking FileReader/parser method chain and an allowed try-with-resources case.

Validation:

  • mise x java@temurin-21 maven@3.9 -- mvn -q -Dtest=CloseInlineResourceRuleTest test
  • mise x java@temurin-21 maven@3.9 -- mvn -q -DskipITs test
  • mise x java@temurin-21 maven@3.9 -- mvn -B --no-transfer-progress verify -Pqulice -DskipTests -DskipITs
  • git diff --cached --check
  • git diff --cached --no-ext-diff | gitleaks stdin --no-banner --redact --exit-code 1

@edmoffo edmoffo left a comment

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.

Verdict: REQUEST_CHANGES. Three inline comments below. The strongest reason is a false-positive on constructor-time field assignment of a closeable to this.x — the most common Yegor-style ownership pattern in the projects this ruleset is shipped to, which the rule unconditionally flags. CI is green at ee03c28.

private static boolean inline(final ASTExpression expr) {
return expr.ancestors(ASTResource.class).isEmpty()
&& expr.ancestors(ASTVariableDeclarator.class).isEmpty();
}

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.

inline() treats only ASTResource and ASTVariableDeclarator ancestors as managed sinks, but constructor-time field assignment is neither. A canonical Yegor-style closeable owner — public Foo(final Path p) { this.reader = new FileReader(p.toFile()); } — produces an ASTAssignmentExpression > ASTConstructorCall shape with no ASTResource and no ASTVariableDeclarator above it, so leaksInline returns true and the rule fires on every such constructor. The enclosing class is the actual closeable owner via its own close(), which the rule has no way to see. Extend inline() to treat ASTAssignmentExpression whose left-hand side is an ASTFieldAccess (or any ASTVariableInitializer of a field) as a managed sink, otherwise the rule rejects the project's own composition convention.

return found;
}

private static boolean returnedDirectly(final ASTExpression expr) {

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.

closedDirectly only accepts a literal .close() invoked on the expression itself. Two equally legitimate close paths produce false positives: a closer helper that takes the closeable as an argument — IOUtils.closeQuietly(new FileReader(file)), Closeables.close(new X(), true) — and a deferred close inside a finally block. Either widen the check to accept any method named close* and any closeable passed as an argument to a method whose declaring type is a known closer helper, or document the gap and ship suppressions for the affected fixtures in the rest of the ruleset.

return data;
}

private static boolean leaksInline(final ASTExpression expr) {

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 ASTMethodCall visit is gated by hasCloseableArgument, which rejects any method call that returns a closeable but takes no closeable argument. Files.newInputStream(path).readAllBytes(), Runtime.getRuntime().exec(cmd).getInputStream().read(), and Channels.newChannel(socket).read(buf) all leak a closeable but pass no closeable into the method, so the rule stays silent — even though they match the issue's expression-tree contract exactly. Either drop the gate, or state in the rule description that producer-style closeable factories are intentionally out of scope so adopters do not expect coverage.

@yegor256

Copy link
Copy Markdown
Owner

@GHX5T-SOL pay attention to reviewers comments please

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

PMD.CloseResource does not flag Closeable constructed inline inside method-call chains

3 participants