Add super.finalize() through two-level finalizer hierarchies - #993
Draft
martinfrancois wants to merge 1 commit into
Draft
Add super.finalize() through two-level finalizer hierarchies#993martinfrancois wants to merge 1 commit into
super.finalize() through two-level finalizer hierarchies#993martinfrancois wants to merge 1 commit into
Conversation
…rarchy Pins the correct behavior for A and B extends A where both override finalize() without calling super: one run should add both calls and widen both throws clauses so the hierarchy compiles. On main both calls are added but neither clause is widened, and B gets a duplicate call in cycle two. Marked @ExpectedToFail pending a ScanningRecipe.
4 tasks
super.finalize() through two-level finalizer hierarchies
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: 45 of 52 (Score: 1.5)
Review first: #987
What's changed?
Adds 1 known-failing test to
ObjectFinalizeCallsSuperTest, namedaddsSuperFinalizeAndWidensThrowsAcrossClassHierarchy. It reproduces two defects that show up whenObjectFinalizeCallsSuperruns over a two-levelfinalize()hierarchy. No recipe code changes. The test is marked@ExpectedToFailso the suite stays green; removing the mark shows the failure. The class goes from 2 tests to 3.What's your motivation?
Recipe:
org.openrewrite.staticanalysis.ObjectFinalizeCallsSuper.When a class and its subclass both override
finalize()in the same run, the recipe produces output that does not compile, and it does not converge.Before
Actual after the recipe
Using current main after the first cycle.
Two things are wrong here.
First, neither throws clause is widened.
java.lang.Object#finalize()declaresthrows Throwable, so I compiled this output with javac and it fails at eachsuper.finalize()call witherror: unreported exception Throwable; must be caught or declared to be thrown.Second, the run does not converge. The second cycle inserts a duplicate
super.finalize();intoBonly, so its body ends up with two consecutivesuper.finalize();calls and the test harness reportsExpected recipe to complete in 1 cycle, but took at least one more cycle. The reason is thatBresolves its inserted call through the parse-time type attribution ofA#finalize(), which does not carry the edit made toAin the same run, so the recipe does not recognize its own first insertion.Expected after the recipe
Both overrides MUST call
super.finalize()exactly once and MUST declarethrows Throwable. I verified with javac that this expected result compiles.Found while preparing #969, which fixes the throws widening for a single class and discloses this limitation in its "Two limits" section without fixing it. A full fix needs hierarchy-aware handling beyond #969, for example a
ScanningRecipethat collects the finalize hierarchy before editing, because the subclass reads the parse-time attribution ofA#finalize()and does not see edits made toAin the same run.Affected code in real projects
NationalSecurityAgency/ghidraGhidraRandomAccessFile.java: warns about files left unclosed in afinalize()override that declares nothrowsclause and never callssuper.finalize(). The recipe from main appendssuper.finalize();without widening thethrowsclause, so the file stops compiling withunreported exception Throwable; must be caught or declared to be thrown.Tencent/libpagPAGSurface.java: releases native resources in afinalize()override with nothrowsclause. The recipe from main insertssuper.finalize();into it, producing the same unreportedThrowablecompile error.google/guavaGcFinalizationTest.java: several anonymousObjectsubclasses overridefinalize()without athrowsclause to count down latches. The recipe from main addssuper.finalize();to each of them, and none of the resulting overrides compile.Anything in particular you'd like reviewers to focus on?
I think this is a genuine bug: the recipe turns compiling code into non-compiling code, and it duplicates a statement on the next cycle. If you agree this should change, I would gladly prepare the fix. If this behavior is intended, feel free to close this and I know it is settled.
Any additional context
Pre-existing tests changed: None.
My open #969 touches the same recipe but does not fix this; it widens the throws clause for a single class only.
This reproduction was prepared with AI assistance (Claude Code). I reviewed the tests and this description.
The added reproduction tests and the existing suite together cover changed and unchanged behavior. The known-failing tests remain disabled until implementation. The formatter run was calibrated per file; untouched lines were not reformatted.
Checklist
./gradlew buildlocally, and committed any resulting changes torecipes.csv