`RenamePrivateFieldsToCamelCase` should not rename annotated fields, similar to `FinalizePrivateFields`

[timo-abele]

What version of OpenRewrite are you using?

I am using

• Maven/Gradle plugin v5.22.0
• rewrite-static-analysis_v1.3.0

How are you running OpenRewrite?

I am using the Maven plugin, and my project is a single module project.

            <plugin>
              <groupId>org.openrewrite.maven</groupId>
              <artifactId>rewrite-maven-plugin</artifactId>
              <version>5.22.0</version>
              <configuration>
                <activeRecipes>
                  <recipe>org.openrewrite.staticanalysis.CommonStaticAnalysis</recipe>
                </activeRecipes>
              </configuration>
              <dependencies>
                <dependency>
                  <groupId>org.openrewrite.recipe</groupId>
                  <artifactId>rewrite-static-analysis</artifactId>
                  <version>1.3.0</version>
                </dependency>
              </dependencies>
            </plugin>

What is the smallest, simplest way to reproduce the problem?

Haven't tested this, original has a slightly different name and longer class, but result should be the same.

class A {

    private static String USERNAME_TOO_LONG = "User name too long";

}

What did you expect to see?

class A {

    private static final String USERNAME_TOO_LONG = "User name too long";

}

What did you see instead?

class A {

    private static String usernameTooLong = "User name too long";

}

What is the full stack trace of any errors you encountered?

stacktrace output here

Are you interested in contributing a fix to OpenRewrite?

[timtebeek]

Hmm; I've tried to replicate this issue with a test, but so far I'm not seeing the same result. 🤔 This test passes.

package org.openrewrite.staticanalysis;

import org.junit.jupiter.api.Test;
import org.openrewrite.DocumentExample;
import org.openrewrite.Issue;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;

import static org.openrewrite.java.Assertions.java;

class CommonStaticAnalysisTest implements RewriteTest {
    @Override
    public void defaults(RecipeSpec spec) {
        spec.recipeFromResources("org.openrewrite.staticanalysis.CommonStaticAnalysis");
    }

    @Test
    @Issue("https://github.com/openrewrite/rewrite-static-analysis/issues/254")
    void finalizePrivateFieldBeforeRenamePrivateFieldsToCamelCase() {
        rewriteRun(
          //language=java
          java(
            """
              class A {
                  private static String USERNAME_TOO_LONG = "User name too long";
              }
              """,
            """
              class A {
                  private static final String USERNAME_TOO_LONG = "User name too long";
              }
              """
          )
        );
    }
}

Is there any way in which your situation might differ from the above test? How can you can tweak the test to make it match & fail?

[timo-abele]

I can already say that the issue is not as I suspected with the order in which the recipes are run but solely with FinalizePrivateFields, which skips the class. RenamePrivateFieldsToCamelCase on the other hand does not skip the class. This is the third instance of class skipping I've encountered, I hope I can manage to reproduce this one in a shareable form.

Update: @timtebeek Can you please add @Service (org.springframework.stereotype.Service) to class A? If I add A to my project as you listed it above, it is processed, but with @Service it is skipped.

[timo-abele]

I guess here is the cause: https://github.com/openrewrite/rewrite-static-analysis/blob/a049e1c75c5b3e976612a0448d0088039bfaf246/src/main/java/org/openrewrite/staticanalysis/FinalizePrivateFields.java#L56-L63 Any annotation on the class will cause it to be skipped. I think an explicit list would be better here, otherwise a subsequently run RenamePrivateFieldsToCamelCase will make things worse and rename constants that the user forgot to make final.

[timtebeek]

Any annotation on the class will cause it to be skipped. I think an explicit list would be better here, otherwise a subsequently run RenamePrivateFieldsToCamelCase will make things worse and rename constants that the user forgot to make final.

That helps narrow it down, thanks! It's hard to be exhaustive with listing annotations though, as there's a lot of libraries and frameworks out there that might work differently if a field is made final: Jackson, Lombok, Hibernate, Validation, MapStruct... we can't list them all without breaking some use cases, which is why we then err on the side of caution to do no harm.

But come to think of it; any such annotations likely also mean we should not rename the field either (similar to how we don't finalize the field), because those same annotations will also result in a different getter/table/json/... Could the fix here then be to take that skip that you've identified in FinalizePrivateFields and similarly apply that in RenamePrivateFieldsToCamelCase? Seems the best we can do to not make a change in both of those cases, instead of in only one of those cases.

[timo-abele]

I agree that RenamePrivateFieldsToCamelCase should have a similar protection against annotations.

How do you feel about a whitelist of annotations considered safe? Such a list would not need to be exhaustive but would allow for more migrations than an outright ban. I would for example consider allowing @Getter a calculated risk in the case of FinalizePrivateFields. If someday, some framework relies on lombok's @Getter and behaves differently depending on whether a field in a class annotated with @Getter is finalized, then the annotation can still be removed from the whitelist. But until that unlikely moment such fields would be covered.

In any case, such limitations should be documented in the description of the recipe.

[timtebeek]

Good that we agree on an approach to solve this immediate issue; would you want to help out with a PR to make it so? :)

As for a whitelist: while indeed it would be nice to do more conversions where we can, in practice I fear there's limited cases where we can safely do so. In this case for instance we can only safely finalize a field if there's only a @Getter annotation. Any use of @Setter, @Value, @*ArgsConstructor or @Data has the potential to break code, and that's just looking at Lombok annotations.

We really want to avoid breaking code, as that would mean we lose trust with our users, who might be running hundreds of recipes in a composite recipe, and not have any tolerance for small fixes to apply for each of those. Not making any change is then often the safest option.

I agree that we should also expand the documentation to list any restrictions on when thise recipe applies, as folks should not have to read that from the code when wondering why a particular case was not converted. Thanks for pointing that out!

[timo-abele]

I'm afraid I won't find the time to help out with PRs on this issue.

regarding whitelist: I still think of it as an appropriate solution for the limited but (IMO) frequent cases where there are only harmless annotations. Clearly harmless @Getter, @Service, @Autowired would go into the whitelist. Clearly problematic @Setter, @Value, @*ArgsConstructor and @Data would not. And potentially harmful annotations, say @nonnull at the maintainers discretion. As long as we check that class annotations ∪ field annotations are all contained in the whitelist, I see no risk.

regarding documentation expansion: I guess a quickfix would be a "limitations" block in the description string, but maybe that's worth to be a field of its own?