ObjectFinalizeCallsSuper: declare Throwable when adding super.finalize() - #969
Open
martinfrancois wants to merge 4 commits into
Open
Conversation
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.
4 tasks
Open
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Suggested review order: 5 of 52 (Score: 8.5)
Review first: #992
What's changed?
ObjectFinalizeCallsSupernow declaresthrows Throwableon the override when the inheritedfinalize()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 declaresThrowable, the recipe addsthrows Throwableto the override, withJavaTemplateatmd.getCoordinates().replaceThrows(), then adds the call; an override that already declaresThrowablekeeps 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
Actual after the recipe
Expected after the recipe
Object#finalize()is declaredprotected 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 matchessuper.finalize()syntactically.The second is a crash. An abstract redeclaration,
protected abstract void finalize();, has no body, somd.getBody().getCoordinates()threw aNullPointerExceptionon 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.JavaTemplatere-attributes the whole method declaration, so afterwards theJavaType.Methodheld 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 setwithMethodType(...)andwithName(md.getName().withType(...))to one and the same instance.UnnecessaryThrowsin 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
NullPointerExceptionabove, has no test.Two limits:
finalize()declares an exception, it acts only on an override that already declaresThrowable, or on one with no throws clause of its own while the inherited method declaresThrowable. So forclass Parent { protected void finalize() throws IOException {} }and a subclass whose override also declaresthrows IOExceptionand has nosuper.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. TheallMatchcheck below would win that case back.AandB extends Awhere both overridefinalize()and both need the call, a single run adds the call to both but widens onlyA's throws clause. When the recipe visitsB, the type information it reads forA'sfinalize()is still the attribution produced when the sources were parsed, which does not carry the clause the same run has just added, soBgets 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
throwsclause by hand fromJContainer,JRightPaddedand a newJ.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
throwsclause on the override that covers every exception the inheritedfinalize()declares, not only one that listsThrowable. That is anallMatchcheck: 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,addsThrowsThrowableToEmptyBodyandaddsThrowsThrowableRetainingCommentsassert the addedthrows Throwableon a directObjectsubclass.addsThrowsThrowableWhenSuperclassDeclaresThrowableandaddsSuperFinalizeWithoutThrowsWhenSuperclassDeclaresNoThrowsuse a superclass declared in the same file, the shape in which the generated call is not type attributed.doNotChangeWhenSuperclassNarrowsThrowsToAnotherExceptionanddoNotChangeWhenThrowsClauseDoesNotCoverThrowableassert no change at all, and current main rewrites both.No existing test changed: the two that were already there,
addsSuperFinalizeInvocation(the@DocumentExample) andhasSuperFinalizeInvocation, 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
libyal/libpffate9b304c79a8dcdff7b11529022b2dfd8bf64a20f(363 stars on 2026-08-16).org.openrewrite.staticanalysis.ObjectFinalizeCallsSuperfrom the latest release tagv2.41.0, published asorg.openrewrite.recipe:rewrite-static-analysis:2.41.0.org.openrewrite.maven:rewrite-maven-plugin:6.46.1:runNoFork, with the checkout'sjpffdirectory configured as the external source directory and the recipe artifact supplied throughrewrite.recipeArtifactCoordinates.jpff/File.java.The recipe changes the repository's existing override as follows:
protected void finalize() { synchronized( this ) { internal_free(); } + super.finalize(); }Object.finalize()declaresthrows Throwable. The existing override declares nothrowsclause, so the inserted call is an unhandled checked exception and the generated Java source fails compilation. The recipe MUST either widen the override'sthrowsclause when legal or leave this method unchanged.Apache Cassandra reproduction
Real-world input
Actual after the recipe:
Expected after the recipe:
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
./gradlew buildlocally, and committed any resulting changes torecipes.csv