UnnecessaryCloseInTryWithResources: only remove a trailing close of a Closeable

[martinfrancois]

Suggested review order: 14 of 52 (Score: 7)
Review first: openrewrite/rewrite#8445

What's changed?

UnnecessaryCloseInTryWithResources no longer deletes a close() call that a later statement in the same try body can observe. It now removes a call only when all four of these hold.

1. The call is the last statement of the try body, or every statement after it is itself a close()
   call this same rule removes, so a run of repeated trailing close() calls is removed as a whole.
2. The receiver is the resource declared last. A try-with-resources statement closes its resources
   in the reverse of the order in which they are declared (JLS 14.20), so that one is closed first, at the position the explicit call already occupies. Removing the call on any earlier declared resource would move its close to a later point and change the order in which resources are closed. doNotRemoveCloseOfResourceThatIsNotClosedFirst covers this, and its "closed first" names the same resource as "declared last" here.
3. The receiver matches that resource under SemanticallyEqual.areEqual, rather than by simple name,
   which is how the code on main compares them.
4. The method is java.io.Closeable.close(), or an override of it, whose javadoc states "If the
   stream is already closed then invoking this method has no effect" (Closeable#close()). The MethodMatcher signature is narrowed from java.lang.AutoCloseable close() to java.io.Closeable close() for the reason given below.

Two comment situations also stop the removal, so a comment attached to a removable close() call, or to the end of the try body, is not deleted with the call: a close() call carrying a comment of its own, on the lines directly above it, is left in place; and if a comment sits between the last statement of the try body and that body's closing brace, the try statement is left untouched in full.

The recipe's description field is rewritten to state this behaviour, and the recipe's row in src/main/resources/META-INF/rewrite/recipes.csv carries the same new text.

Before

try (Resource resource = new Resource()) {
    resource.close();
    if (!resource.closed) {
        throw new AssertionError();
    }
}

Actual after the recipe

Using the recipe on current main.

try (Resource resource = new Resource()) {
    if (!resource.closed) {
        throw new AssertionError();
    }
}

Expected after the recipe

(unchanged)

The recipe leaves the input above exactly as written.

What's your motivation?

Recipe: org.openrewrite.staticanalysis.UnnecessaryCloseInTryWithResources.

Both the input and the output produced by main compile, so the problem shows only at run time: the input runs without error, the output throws AssertionError, because resource.closed is read while the resource is still open. On main the recipe deletes a close() call on any declared resource, at any position in the try body, and puts nothing in its place, so every statement that used to run after the explicit call now runs while the resource is still open.

Two further symptoms. For a resource whose close() is not idempotent, for example one that counts its own calls, the deletion makes close() run once where the source asked for twice: the contract of AutoCloseable#close(), the signature main's MethodMatcher is built with, does not require idempotence, which is why this branch narrows it. A comment written above the deleted call is deleted with it as well.

Separately, the recipe on main throws a NullPointerException on valid Java 9 code that uses a field as a resource, such as try (this.writer) { writer.close(); }. A field access is neither a variable declaration nor a plain identifier, so the visitor stores null as that resource's name and then calls equals on it. This branch reads only the last declared resource and returns the try statement unchanged when that resource is neither of those two forms.

Reproduced on v2.39.0, v2.40.0 and current main. The recipe is listed in common-static-analysis.yml, so this bug reaches everyone who runs that composite recipe.

Confirmed real-world execution

• Project: Apache Druid (14,042 stars on 2026-08-16).
• Location: ColumnSerializerUtils.java at 5d84de19.
• Recipe release: org.openrewrite.recipe:rewrite-static-analysis:2.41.0.

Druid closes its writer and smoosher before loading the file produced by them. The released recipe deletes both calls, so the load runs before the metadata file exists or reads stale metadata.

Anything in particular you'd like reviewers to focus on?

Two points:

• The new description. The old text was inaccurate in three ways: it named AutoCloseable, where the contract the recipe depends on is the narrower java.io.Closeable; it inferred redundancy from "try-with-resources closes each declared resource" alone, which claims it for a call on any declared resource, not only on the one declared last; and it never mentioned the last-statement condition.
• The visitor walks backwards from the last statement and removes the whole run of consecutive redundant close() calls in a single visit of the try statement, rather than one call per recipe cycle, which is what main does too. removeRepeatedTrailingClose covers a try body that ends in two consecutive scanner.close() calls, under the default RecipeSpec, which allows exactly one changing cycle, so it would fail if a second cycle were needed.

One limitation remains. A type can implement java.io.Closeable and still break the contract that interface documents, for example by counting every call to close() or by throwing on the second one. An explicit close() on such a type is still removed, exactly as on main.

Have you considered any alternatives or workarounds?

One alternative is to keep the matcher signature java.lang.AutoCloseable close(), which keeps the recipe active for custom resource types that implement AutoCloseable without implementing Closeable. Some such types define an idempotent close(), but the contract does not require idempotency, so the recipe would still change how often close() runs for the remaining types. Reverting means editing one string literal, the signature passed to the MethodMatcher constructor, and deleting doNotRemoveCloseOfResourceThatIsNotCloseable, the test that pins the narrower signature down. Nothing else here depends on the choice.

Any additional context

This change adds 7 tests to UnnecessaryCloseInTryWithResourcesTest. Without the code change in this pull request, these 5 tests fail:

• doNotRemoveCloseWhenResourceStateIsObservedAfterwards
• doNotRemoveCloseFollowedByOtherStatements
• doNotRemoveCloseOfResourceThatIsNotClosedFirst
• doNotRemoveCloseOfResourceThatIsNotCloseable
• doNotRemoveCloseWithComment

These 2 tests pass either way and cover behavior this change preserves:

• removeRepeatedTrailingClose, described above
• removeCloseOfReferencedResource, which shows that the try (scanner) form still has its trailing close() removed

No existing test expectation changed: the test file has 196 added lines and no deleted lines, so the three tests already in it, including the @DocumentExample, are untouched.

This change was prepared with AI assistance (Claude Code). I reviewed the code, the tests and this description.

Checklist

• I've added unit tests to cover both positive and negative cases
• I've read and applied the recipe conventions and best practices
• I've run ./gradlew build locally, and committed any resulting changes to recipes.csv
• I've formatted the lines I changed, without reformatting code I didn't touch