org.openrewrite.java.cleanup.RenamePrivateFieldsToCamelCase does not recognize @Data lombok annotation

[blipper]

org.openrewrite.java.cleanup.RenamePrivateFieldsToCamelCase only checks if there is a private field. It does not recognize @DaTa or any other lombok annotation so renaming the field will introducing a breaking change. To preserve compatibility it should ignore private fields if the class is annotated with @Data/@Getter/@Setter

[timtebeek]

Hi @blipper ! While we do not yet support lombok, we can and indeed should at least not make any changes to private fields when lombok annotations are present. Thanks for the suggestion!

I think it should be fairly easy to detect such annotations to begin with; either on the field directly, or on the surrounding class. The first step is likely a unit test in RenamePrivateFieldsToCamelCaseTest.java that replicates the issue, before we add detection and skip logic in RenamePrivateFieldsToCamelCase.java.

[timtebeek]

@gwydionmv added a very similar change in

• #242 Fix remove unused private fields #244

That could be replicated here to make RenamePrivateFieldsToCamelCase skip classes or fields with Lombok annotations.

[timtebeek]

As part of that change we'd then likely want to do the same for ExplicitInitializationVisitor

rewrite-static-analysis/src/main/java/org/openrewrite/staticanalysis/ExplicitInitializationVisitor.java

Lines 59 to 62 in a5dc2f7

	Iterator<Cursor> clz = getCursor().getPathAsCursors(c -> c.getValue() instanceof J.ClassDeclaration);
	if (clz.hasNext() && service(AnnotationService.class).matches(clz.next(), LOMBOK_VALUE)) {
	return v;
	}

[gwydionmv]

@timtebeek after thinking about this, it's not exactly the same case, right? Lombok makes some "unsued" variables being used in the getters/setters, so removing them would be wrong. But if a recipe is applied and a variable "HasToChange" is changed to "hasToChange", it doesn't matter if it had lombok annotation or not, it was wrong 😅

[blipper]

Ultimately what think is needed for all of these annotation processors is a mechanism where rewrite has access to the paired byte code generated at compilation time. AST analysis will never be enough with annotation processors.

[blipper]

Or a at a minimum if you want to keep pure AST analysis you need to pair with the delomboked source

[timtebeek]

Thanks for helping think this one through! The reason I'd want to skip lombok annotated classes and fields is that this recipe also changes snake case to camel case, as seen in this test case

rewrite-static-analysis/src/test/java/org/openrewrite/staticanalysis/RenamePrivateFieldsToCamelCaseTest.java

Lines 45 to 50 in 541198c

	private String D_TYPE_CONNECT = "";
	}
	""",
	"""
	class Test {
	private String dTypeConnect = "";

I'm not a 100% sure what getters lombok would generate before and after the rename, but I think that'd be getD_TYPE_CONNECT(), so best skipped.

[gwydionmv]

yes, yes, you're right, that's what Lombok generates for that field, .getD_TYPE_CONNECT(). And that's what my point was, if that variable name is wrong, then it's ok that the recipe breaks the code, isn't it? It won't be as automatic as other recipes, but I don't find it a bad thing that after running the recipe the dev has to replace the old getter with getDTypeConnect()

[timtebeek]

I understand where you're coming from in that if you want to thoroughly clean up the code in a single repository, that it's then perhaps acceptable to have to put some effort as well. However, we strongly belief in keeping to our do no harm convention, where we only make a code change when we believe it safe to do so.

Consider for example that you're not looking at a single project but dozens or even hundreds; and then not running a single recipe, but also dozens or hundreds. In those cases you'd much rather have each of those recipes not make a change when there's potential to break code, as there would be a lot of code to check afterwards. Or consider the case where you run the recipe on a library, that may then subtly change the exposed getter, which you might only notice in consumers.

Given the above I think it's best to make all the code changes we know are safe, and back off from making code changes when there might be unhandled side effects such as in the case of Lombok annotated fields. That's also why we for instance don't run our "Convert @lombok.Value class to Record" recipe by default. As much as we think it might improve code, there's also potential for negative effects in downstream consumers of libraries, so we leave it up to the user to consider running that by itself as desired.

[blipper]

Working at Amazon and previously worked at Google which both operate at scale you need to minimize false positives so +1.

That being said with Lombok the private fields are similar to records is that they become a public API/source of truth so arguably they aren't private at all anymore.