Skip to content

ObjectFinalizeCallsSuper: declare Throwable when adding super.finalize() - #969

Open
martinfrancois wants to merge 4 commits into
openrewrite:mainfrom
martinfrancois:fix/object-finalize-calls-super-uncaught-throwable
Open

ObjectFinalizeCallsSuper: declare Throwable when adding super.finalize()#969
martinfrancois wants to merge 4 commits into
openrewrite:mainfrom
martinfrancois:fix/object-finalize-calls-super-uncaught-throwable

Conversation

@martinfrancois

@martinfrancois martinfrancois commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Suggested review order: 5 of 52 (Score: 8.5)
Review first: #992

What's changed?

ObjectFinalizeCallsSuper now declares throws Throwable on the override when the inherited finalize() declares it and the override declares no exception of its own, so the output compiles.

The recipe looks up the nearest finalize() in the supertype chain. If that inherited method declares Throwable, the recipe adds throws Throwable to the override, with JavaTemplate at md.getCoordinates().replaceThrows(), then adds the call; an override that already declares Throwable keeps its clause and only gets the call, and one that declares a different exception is left alone. If the inherited method declares no exception, it adds only the call and leaves the throws clause alone, which is what current main does in every case. Anything else, including missing type information, is left unchanged.

Groovy and Kotlin are not affected: neither has checked exceptions, and the code that adds the clause only runs when the enclosing file is a Java compilation unit.

What's your motivation?

Recipe: org.openrewrite.staticanalysis.ObjectFinalizeCallsSuper.

Before

protected void finalize() { }

Actual after the recipe

protected void finalize() { super.finalize(); }

Expected after the recipe

protected void finalize() throws Throwable { super.finalize(); }

Object#finalize() is declared protected void finalize() throws Throwable. Most overrides declare no checked exception, and current main adds the call without touching the throws clause, so the output does not compile.

Two further problems live in the same method, and this branch fixes both.

The first is a duplicated call. The generated super.finalize() is not type attributed when the superclass is declared in the same compilation unit, before and after this change. Current main's search for an already present call matched on type information alone, so in those files it missed the call an earlier cycle had added, a later cycle added a second one, and the parent finalizer ran twice. The search now also matches super.finalize() syntactically.

The second is a crash. An abstract redeclaration, protected abstract void finalize();, has no body, so md.getBody().getCoordinates() threw a NullPointerException on current main. Null checks on the method type and body now return the method unchanged instead.

All three problems reproduce on current main (5785534) and v2.40.0.

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

Please look at the three lines directly after the replaceThrows() call. JavaTemplate re-attributes the whole method declaration, so afterwards the JavaType.Method held by the declaration and the one held by its name are different instances, and the tests fail with "LST contains missing or invalid type information". Those lines set withMethodType(...) and withName(md.getName().withType(...)) to one and the same instance. UnnecessaryThrows in this repository fixes the same problem the same way, which is where I took the pattern from.

One weak spot: the null check on the method body, which prevents the NullPointerException above, has no test.

Two limits:

  • This branch is more conservative than current main in one shape. Where the inherited finalize() declares an exception, it acts only on an override that already declares Throwable, or on one with no throws clause of its own while the inherited method declares Throwable. So for class Parent { protected void finalize() throws IOException {} } and a subclass whose override also declares throws IOException and has no super.finalize() call, adding the call is safe and current main does it correctly, while this branch adds nothing. That is a deliberate narrowing rather than a misfire, but it does give up a rewrite current main performs correctly. The allMatch check below would win that case back.
  • Given A and B extends A where both override finalize() and both need the call, a single run adds the call to both but widens only A's throws clause. When the recipe visits B, the type information it reads for A's finalize() is still the attribution produced when the sources were parsed, which does not carry the clause the same run has just added, so B gets no clause and does not compile. Current main also produces non-compiling output for this pair: it adds both calls and widens neither clause. A full fix needs a scanning recipe that collects the hierarchy before editing.

Have you considered any alternatives or workarounds?

An earlier version built the throws clause by hand from JContainer, JRightPadded and a new J.Identifier. The output was the same, but the recipe conventions say not to build LST elements by hand, so I dropped it. That version also hid the type problem above, because it left the existing method type in place instead of re-attributing the declaration.

One alternative is to accept any throws clause on the override that covers every exception the inherited finalize() declares, not only one that lists Throwable. That is an allMatch check: about five lines plus one test.

Any additional context

Pre-existing tests changed: None.

This change adds 7 tests to ObjectFinalizeCallsSuperTest, and without the code change in this pull request all 7 fail. addsThrowsThrowableWhenOverrideDeclaresNoThrows, addsThrowsThrowableToEmptyBody and addsThrowsThrowableRetainingComments assert the added throws Throwable on a direct Object subclass. addsThrowsThrowableWhenSuperclassDeclaresThrowable and addsSuperFinalizeWithoutThrowsWhenSuperclassDeclaresNoThrows use a superclass declared in the same file, the shape in which the generated call is not type attributed. doNotChangeWhenSuperclassNarrowsThrowsToAnotherException and doNotChangeWhenThrowsClauseDoesNotCoverThrowable assert no change at all, and current main rewrites both.

No existing test changed: the two that were already there, addsSuperFinalizeInvocation (the @DocumentExample) and hasSuperFinalizeInvocation, are untouched and are not in that failing list. The class holds 9 tests.

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

Real-world reproduction

  • Repository: libyal/libpff at e9b304c79a8dcdff7b11529022b2dfd8bf64a20f (363 stars on 2026-08-16).
  • Recipe: org.openrewrite.staticanalysis.ObjectFinalizeCallsSuper from the latest release tag v2.41.0, published as org.openrewrite.recipe:rewrite-static-analysis:2.41.0.
  • Execution: Maven plugin org.openrewrite.maven:rewrite-maven-plugin:6.46.1:runNoFork, with the checkout's jpff directory configured as the external source directory and the recipe artifact supplied through rewrite.recipeArtifactCoordinates.
  • Affected file: jpff/File.java.
    The recipe changes the repository's existing override as follows:
 protected void finalize()
 {
     synchronized(
      this )
     {
         internal_free();
     }
+    super.finalize();
 }

Object.finalize() declares throws Throwable. The existing override declares no throws clause, so the inserted call is an unhandled checked exception and the generated Java source fails compilation. The recipe MUST either widen the override's throws clause when legal or leave this method unchanged.

Apache Cassandra reproduction

  • Repository: apache/cassandra at 6cf449342daf45b7388a6c105f627b3859a63b46 (10,071 stars on 2026-08-16).
  • Recipe: org.openrewrite.staticanalysis.ObjectFinalizeCallsSuper from the latest release tag v2.41.0, published as org.openrewrite.recipe:rewrite-static-analysis:2.41.0.
  • Execution: Maven plugin org.openrewrite.maven:rewrite-maven-plugin:6.46.1:runNoFork, with the checkout source supplied directly to the recipe.
  • Affected file: src/java/org/apache/cassandra/utils/binlog/BinLog.java.

Real-world input

@Override
protected void finalize() {
    stop();
}

Actual after the recipe:

@Override
protected void finalize() {
    stop();
    super.finalize();
}

Expected after the recipe:

@Override
protected void finalize() throws Throwable {
    stop();
    super.finalize();
}

The inserted call invokes Object.finalize(), which declares throws Throwable. The existing override declares no throws clause, so the generated Java source fails compilation. This is the highest-star exact real-world reproduction found; the libpff reproduction below independently demonstrates the same defect.

Checklist

The recipe appended `super.finalize()` to any declaration matching
`java.lang.Object finalize()` without looking at the throws clause.
`Object.finalize()` declares `Throwable`, and an override that does not
call the superclass may legally omit it, so the added call left those
sources with an unreported checked exception and they no longer
compiled.

Resolve the nearest supertype declaration of `finalize()` and branch on
what it throws. Nothing declared means the added call throws nothing
either, so it is appended as before. `Throwable`, which includes
reaching `java.lang.Object`, means add `throws Throwable` to the
override first, but only when the override declares no clause of its
own. Every other case, including a supertype that narrowed the clause
to another checked exception and missing type attribution, has no
legal way to add the call without inventing exception handling, so
those methods are left unchanged. The throws check applies only inside
a Java compilation unit, since `Throwable` is unchecked in the other
JVM languages this recipe runs on.

The recipe therefore declines a few shapes it used to rewrite, all of
which it could only have made uncompilable. The search for an existing
`super.finalize()` now matches syntactically as well as by type,
because a call the recipe itself generates is not always attributed
when the superclass is in the same compilation unit, and a later cycle
appended it a second time.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In Progress

Development

Successfully merging this pull request may close these issues.

2 participants