Preserve comments and Groovy conversions when replacing wrapper constructors - #991
Draft
martinfrancois wants to merge 1 commit into
Draft
Conversation
…ped comments and missing Groovy cast preserveCommentsWithinNewClass pins that comments between 'new' and the argument list are dropped when the constructor is replaced with valueOf. groovyCompoundDoubleToFloat pins that a compound double argument in Groovy gets no (float) cast because its type is not resolved. Both are marked @ExpectedToFail; related: openrewrite#970.
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: 42 of 52 (Score: 2)
Review first: openrewrite/rewrite-migrate-java#1195
What's changed?
Adds 2 known-failing tests to
PrimitiveWrapperClassConstructorToValueOfTestthat reproduce two defects inPrimitiveWrapperClassConstructorToValueOf: the recipe drops comments written betweennewand the constructor arguments, and on Groovy it misses the(float)cast for a compounddoubleargument tonew Float(...). No recipe code changes. The tests are marked@ExpectedToFailso the suite stays green; removing the annotation shows the failures. The class had 7 tests on main, all passing, and has 9 with these additions.What's your motivation?
Recipe:
org.openrewrite.staticanalysis.PrimitiveWrapperClassConstructorToValueOf.Case 1: preserve Java comments
Before
Actual after the recipe
Using current main.
Expected after the recipe
Integer i = /* keep me */ Integer.valueOf(42);The
/* keep me */comment is gone. The recipe replaces the wholeJ.NewClasswith a template, and comments carried on the inner class identifier are discarded. This applies to every wrapper class the recipe handles, not justInteger. After the recipe runs, the code should keep the comment, for exampleInteger i = /* keep me */ Integer.valueOf(42);.Case 2: preserve a compound
doubleargumentBefore
Actual after the recipe
Using current main.
Expected after the recipe
Float sum = Float.valueOf((float) (d1 + d2))The Groovy parser attributes the type of
d1 + d2asjava.lang.Objectrather thandouble, so the recipe's argument type checks miss and it falls through to the plainFloat.valueOf(#{any(float)})template with no cast. There is noFloat.valueOf(double)overload; the equivalent Java code does not compile ("no suitable method found for valueOf(double)", verified on Java 25). Dynamic Groovy coerces at runtime, so this is not a runtime crash there, but the output breaks under@CompileStaticand is inconsistent with what the recipe produces for the same Java code. The expected output isFloat.valueOf((float) (d1 + d2)); I verified thatnew Float(d1 + d2)andFloat.valueOf((float) (d1 + d2))produce equal values.Found while preparing #970, which fixes a related
Float(double)cast defect in this recipe. Both defects are present on current main and stay present with the change from #970 applied. #970 keys its fix onJavaType.Primitive.Double, and the Groovy argument is attributed asObject, so #970 will not close the Groovy case.Anything in particular you'd like reviewers to focus on?
I think both are genuine bugs: defect 1 silently loses source content, and defect 2 produces code that fails under static compilation. The Groovy root cause looks framework level, in rewrite-groovy type attribution of compound expressions, rather than in this recipe. 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.
Related open PR of mine on the same recipe: #970 (it does not fix either defect here). openrewrite/rewrite#8170 documents a different Groovy type attribution gap and is adjacent context 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