Avoid non-compiling collisions when renaming near-miss Object methods - #987
Draft
martinfrancois wants to merge 1 commit into
Draft
Avoid non-compiling collisions when renaming near-miss Object methods#987martinfrancois wants to merge 1 commit into
Object methods#987martinfrancois wants to merge 1 commit into
Conversation
…compiling renames Two rename shapes produce output javac rejects, and neither is covered by the guard proposed in the open PR openrewrite#974 (verified by applying its diff and rerunning): renaming both of two differently misspelled variants in one class creates two colliding declarations, and renaming an annotation type member gives the element the name of an Object method, which an annotation interface may not declare. Both tests assert the input is left unchanged and are marked ExpectedToFail.
4 tasks
Object methods
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: 44 of 52 (Score: 1.5)
Review first: openrewrite/rewrite-migrate-java#1194
What's changed?
Adds 2 known-failing tests to
RenameMethodsNamedHashcodeEqualOrToStringTest. They reproduce two cases whereRenameMethodsNamedHashcodeEqualOrToStringrenames a method and the result no longer compiles: two misspelled variants in one class that both get renamed to the same name, and an annotation type element that gets renamed tohashCode(). No recipe code changes. The tests are marked@ExpectedToFail, so the suite stays green; removing the mark shows the failure.What's your motivation?
Recipe:
org.openrewrite.staticanalysis.RenameMethodsNamedHashcodeEqualOrToString.The recipe only checks the method name (case-insensitive, but not exactly equal), the return type, and the arity before scheduling the rename. It never looks at sibling declarations or at the kind of the declaring type. Both gaps can turn compiling code into non-compiling code.
Case 1: two misspelled variants collide
Before
Actual after the recipe
Using current main. both methods become
public int hashCode(), so the class contains two identical declarations. javac 25.0.4 rejects that output witherror: method hashCode() is already defined.Expected after the recipe
(unchanged)Case 2: annotation element uses an
Objectmethod nameBefore
Actual after the recipe
Using current main. the element becomes
int hashCode();. javac 25.0.4 rejects it witherror: annotation interface Annotation declares an element with the same name as method hashCode(), because an annotation element may not carry the name of anObjectmethod.Expected after the recipe
(unchanged)Both inputs compile before the recipe. The recipe MUST leave both unchanged so its output also compiles.
I found this while preparing #974, which fixes a related defect in this recipe. Both shapes here still fail with the #974 change applied locally: its
canRenameTo(...)guard scans siblings for a method already named exactlyhashCode, which neither misspelled variant is, and annotation elements are implicitly public and non-static, so its modifier checks do not stop the rename.Anything in particular you'd like reviewers to focus on?
I think both cases are genuine bugs, because the recipe turns compiling code into code that does not compile. One note on the collision test: it pins the conservative outcome, no change at all. A future fix can instead rename exactly one variant; if you prefer that, the test expectation should change together with the fix. 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 PR #974 touches the same recipe. It guards exact-name sibling collisions, non-public methods, and static methods, but it does not cover the two shapes in this draft. The recipe is part of
common-static-analysis.yml, so these renames run in a very common recipe set. 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