From bb00d0a55c6fc426b15fb737c326bbdda0590eff Mon Sep 17 00:00:00 2001 From: timo-a <1398557+timo-a@users.noreply.github.com> Date: Sun, 15 Dec 2024 19:32:05 +0100 Subject: [PATCH] Recipe that converts explicit setters to the lombok annotation (#625) * feat: add recipe that converts explicit getters to the lombok annotation * chore: IntelliJ auto-formatter * add licence header Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * roll back nullable annotation * Light polish * Rename and add Lombok tag * Also handle field access * Push down method and variable name matching into utils * Demonstrate failing case of nested inner class getter * fix: year in licence header had copy-pasted from the example recipe * migrate existing recipe as-is * deactivate getter test for development * Rename and add Lombok tag * fix year in licence header * chore: IntelliJ auto-formatter * apply best practices * light polish * copy from: Also handle field access * minor changes * Minimize changes with `main` branch ahead of rebase to avoid conflicts * Resolve compilation issues * Ensure there is no change for a nested Setter * Extract a reusable FieldAnnotator class * Adopt now shared `FieldAnnotator` for setters as well * Convert most of the checks as used for UseLombokGetter * Add one more style we ought to cover * Add remaining checks to make all tests pass * Move down variable and method closer to usage * Inline variables used only once --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Tim te Beek Co-authored-by: Tim te Beek --- .../java/migrate/lombok/FieldAnnotator.java | 54 ++ .../java/migrate/lombok/LombokUtils.java | 63 +- .../java/migrate/lombok/UseLombokGetter.java | 34 +- .../java/migrate/lombok/UseLombokSetter.java | 77 +++ .../migrate/lombok/UseLombokSetterTest.java | 540 ++++++++++++++++++ 5 files changed, 729 insertions(+), 39 deletions(-) create mode 100644 src/main/java/org/openrewrite/java/migrate/lombok/FieldAnnotator.java create mode 100644 src/main/java/org/openrewrite/java/migrate/lombok/UseLombokSetter.java create mode 100644 src/test/java/org/openrewrite/java/migrate/lombok/UseLombokSetterTest.java diff --git a/src/main/java/org/openrewrite/java/migrate/lombok/FieldAnnotator.java b/src/main/java/org/openrewrite/java/migrate/lombok/FieldAnnotator.java new file mode 100644 index 000000000..2a42531b8 --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/lombok/FieldAnnotator.java @@ -0,0 +1,54 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.lombok; + +import lombok.AccessLevel; +import lombok.EqualsAndHashCode; +import lombok.Value; +import org.openrewrite.ExecutionContext; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.JavaParser; +import org.openrewrite.java.JavaTemplate; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaType; + +import static java.util.Comparator.comparing; +import static lombok.AccessLevel.PUBLIC; + +@Value +@EqualsAndHashCode(callSuper = false) +class FieldAnnotator extends JavaIsoVisitor { + + Class annotation; + JavaType field; + AccessLevel accessLevel; + + @Override + public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { + for (J.VariableDeclarations.NamedVariable variable : multiVariable.getVariables()) { + if (variable.getName().getFieldType() == field) { + maybeAddImport(annotation.getName()); + maybeAddImport("lombok.AccessLevel"); + String suffix = accessLevel == PUBLIC ? "" : String.format("(AccessLevel.%s)", accessLevel.name()); + return JavaTemplate.builder("@" + annotation.getSimpleName() + suffix) + .imports(annotation.getName(), "lombok.AccessLevel") + .javaParser(JavaParser.fromJavaVersion().classpath("lombok")) + .build().apply(getCursor(), multiVariable.getCoordinates().addAnnotation(comparing(J.Annotation::getSimpleName))); + } + } + return multiVariable; + } +} diff --git a/src/main/java/org/openrewrite/java/migrate/lombok/LombokUtils.java b/src/main/java/org/openrewrite/java/migrate/lombok/LombokUtils.java index 629303be9..a397bf904 100644 --- a/src/main/java/org/openrewrite/java/migrate/lombok/LombokUtils.java +++ b/src/main/java/org/openrewrite/java/migrate/lombok/LombokUtils.java @@ -48,7 +48,7 @@ static boolean isGetter(J.MethodDeclaration method) { J.Identifier identifier = (J.Identifier) returnExpression; if (identifier.getFieldType() != null && declaringType == identifier.getFieldType().getOwner()) { // Check return: type and matching field name - return hasMatchingTypeAndName(method, identifier.getType(), identifier.getSimpleName()); + return hasMatchingTypeAndGetterName(method, identifier.getType(), identifier.getSimpleName()); } } else if (returnExpression instanceof J.FieldAccess) { J.FieldAccess fieldAccess = (J.FieldAccess) returnExpression; @@ -56,13 +56,13 @@ static boolean isGetter(J.MethodDeclaration method) { if (target instanceof J.Identifier && ((J.Identifier) target).getFieldType() != null && declaringType == ((J.Identifier) target).getFieldType().getOwner()) { // Check return: type and matching field name - return hasMatchingTypeAndName(method, fieldAccess.getType(), fieldAccess.getSimpleName()); + return hasMatchingTypeAndGetterName(method, fieldAccess.getType(), fieldAccess.getSimpleName()); } } return false; } - private static boolean hasMatchingTypeAndName(J.MethodDeclaration method, @Nullable JavaType type, String simpleName) { + private static boolean hasMatchingTypeAndGetterName(J.MethodDeclaration method, @Nullable JavaType type, String simpleName) { if (method.getType() == type) { String deriveGetterMethodName = deriveGetterMethodName(type, simpleName); return method.getSimpleName().equals(deriveGetterMethodName); @@ -83,15 +83,62 @@ private static String deriveGetterMethodName(@Nullable JavaType type, String fie return "get" + StringUtils.capitalize(fieldName); } - static AccessLevel getAccessLevel(J.MethodDeclaration modifiers) { - if (modifiers.hasModifier(Public)) { + static boolean isSetter(J.MethodDeclaration method) { + // Check return type: void + if (method.getType() != JavaType.Primitive.Void) { + return false; + } + // Check signature: single parameter + if (method.getParameters().size() != 1 || method.getParameters().get(0) instanceof J.Empty) { + return false; + } + // Check body: just an assignment + if (method.getBody() == null || //abstract methods can be null + method.getBody().getStatements().size() != 1 || + !(method.getBody().getStatements().get(0) instanceof J.Assignment)) { + return false; + } + + // Check there's no up/down cast between parameter and field + J.VariableDeclarations.NamedVariable param = ((J.VariableDeclarations) method.getParameters().get(0)).getVariables().get(0); + Expression variable = ((J.Assignment) method.getBody().getStatements().get(0)).getVariable(); + if (param.getType() != variable.getType()) { + return false; + } + + // Method name has to match + JavaType.FullyQualified declaringType = method.getMethodType().getDeclaringType(); + if (variable instanceof J.Identifier) { + J.Identifier assignedVar = (J.Identifier) variable; + if (hasMatchingSetterMethodName(method, assignedVar.getSimpleName())) { + // Check field is declared on method type + return assignedVar.getFieldType() != null && declaringType == assignedVar.getFieldType().getOwner(); + } + } else if (variable instanceof J.FieldAccess) { + J.FieldAccess assignedField = (J.FieldAccess) variable; + if (hasMatchingSetterMethodName(method, assignedField.getSimpleName())) { + Expression target = assignedField.getTarget(); + // Check field is declared on method type + return target instanceof J.Identifier && ((J.Identifier) target).getFieldType() != null && + declaringType == ((J.Identifier) target).getFieldType().getOwner(); + } + } + + return false; + } + + private static boolean hasMatchingSetterMethodName(J.MethodDeclaration method, String simpleName) { + return method.getSimpleName().equals("set" + StringUtils.capitalize(simpleName)); + } + + static AccessLevel getAccessLevel(J.MethodDeclaration methodDeclaration) { + if (methodDeclaration.hasModifier(Public)) { return PUBLIC; - } else if (modifiers.hasModifier(Protected)) { + } else if (methodDeclaration.hasModifier(Protected)) { return PROTECTED; - } else if (modifiers.hasModifier(Private)) { + } else if (methodDeclaration.hasModifier(Private)) { return PRIVATE; } return PACKAGE; } - } diff --git a/src/main/java/org/openrewrite/java/migrate/lombok/UseLombokGetter.java b/src/main/java/org/openrewrite/java/migrate/lombok/UseLombokGetter.java index e7c99bbdf..b3584d79a 100644 --- a/src/main/java/org/openrewrite/java/migrate/lombok/UseLombokGetter.java +++ b/src/main/java/org/openrewrite/java/migrate/lombok/UseLombokGetter.java @@ -15,26 +15,21 @@ */ package org.openrewrite.java.migrate.lombok; -import lombok.AccessLevel; import lombok.EqualsAndHashCode; +import lombok.Getter; import lombok.Value; import org.jspecify.annotations.Nullable; import org.openrewrite.ExecutionContext; import org.openrewrite.Recipe; import org.openrewrite.TreeVisitor; import org.openrewrite.java.JavaIsoVisitor; -import org.openrewrite.java.JavaParser; -import org.openrewrite.java.JavaTemplate; import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; -import org.openrewrite.java.tree.JavaType; import java.util.Collections; -import java.util.List; import java.util.Set; import static java.util.Comparator.comparing; -import static lombok.AccessLevel.PUBLIC; @Value @EqualsAndHashCode(callSuper = false) @@ -66,12 +61,14 @@ public TreeVisitor getVisitor() { if (returnExpression instanceof J.Identifier && ((J.Identifier) returnExpression).getFieldType() != null) { doAfterVisit(new FieldAnnotator( + Getter.class, ((J.Identifier) returnExpression).getFieldType(), LombokUtils.getAccessLevel(method))); return null; } else if (returnExpression instanceof J.FieldAccess && ((J.FieldAccess) returnExpression).getName().getFieldType() != null) { doAfterVisit(new FieldAnnotator( + Getter.class, ((J.FieldAccess) returnExpression).getName().getFieldType(), LombokUtils.getAccessLevel(method))); return null; @@ -81,29 +78,4 @@ public TreeVisitor getVisitor() { } }; } - - - @Value - @EqualsAndHashCode(callSuper = false) - static class FieldAnnotator extends JavaIsoVisitor { - - JavaType field; - AccessLevel accessLevel; - - @Override - public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { - for (J.VariableDeclarations.NamedVariable variable : multiVariable.getVariables()) { - if (variable.getName().getFieldType() == field) { - maybeAddImport("lombok.Getter"); - maybeAddImport("lombok.AccessLevel"); - String suffix = accessLevel == PUBLIC ? "" : String.format("(AccessLevel.%s)", accessLevel.name()); - return JavaTemplate.builder("@Getter" + suffix) - .imports("lombok.Getter", "lombok.AccessLevel") - .javaParser(JavaParser.fromJavaVersion().classpath("lombok")) - .build().apply(getCursor(), multiVariable.getCoordinates().addAnnotation(comparing(J.Annotation::getSimpleName))); - } - } - return multiVariable; - } - } } diff --git a/src/main/java/org/openrewrite/java/migrate/lombok/UseLombokSetter.java b/src/main/java/org/openrewrite/java/migrate/lombok/UseLombokSetter.java new file mode 100644 index 000000000..9bf659606 --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/lombok/UseLombokSetter.java @@ -0,0 +1,77 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.lombok; + +import lombok.EqualsAndHashCode; +import lombok.Setter; +import lombok.Value; +import org.jspecify.annotations.Nullable; +import org.openrewrite.ExecutionContext; +import org.openrewrite.Recipe; +import org.openrewrite.TreeVisitor; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.tree.Expression; +import org.openrewrite.java.tree.J; + +import java.util.Collections; +import java.util.Set; + +@Value +@EqualsAndHashCode(callSuper = false) +public class UseLombokSetter extends Recipe { + + @Override + public String getDisplayName() { + return "Convert setter methods to annotations"; + } + + @Override + public String getDescription() { + return "Convert trivial setter methods to `@Setter` annotations on their respective fields."; + } + + @Override + public Set getTags() { + return Collections.singleton("lombok"); + } + + @Override + public TreeVisitor getVisitor() { + return new JavaIsoVisitor() { + @Override + public J.@Nullable MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { + if (LombokUtils.isSetter(method)) { + Expression assignmentVariable = ((J.Assignment) method.getBody().getStatements().get(0)).getVariable(); + if (assignmentVariable instanceof J.FieldAccess && + ((J.FieldAccess) assignmentVariable).getName().getFieldType() != null) { + doAfterVisit(new FieldAnnotator(Setter.class, + ((J.FieldAccess) assignmentVariable).getName().getFieldType(), + LombokUtils.getAccessLevel(method))); + return null; //delete + + } else if (assignmentVariable instanceof J.Identifier && + ((J.Identifier) assignmentVariable).getFieldType() != null) { + doAfterVisit(new FieldAnnotator(Setter.class, + ((J.Identifier) assignmentVariable).getFieldType(), + LombokUtils.getAccessLevel(method))); + return null; //delete + } + } + return method; + } + }; + } +} diff --git a/src/test/java/org/openrewrite/java/migrate/lombok/UseLombokSetterTest.java b/src/test/java/org/openrewrite/java/migrate/lombok/UseLombokSetterTest.java new file mode 100644 index 000000000..3b0bb02c4 --- /dev/null +++ b/src/test/java/org/openrewrite/java/migrate/lombok/UseLombokSetterTest.java @@ -0,0 +1,540 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.lombok; + +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.java.JavaParser; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; + +// This is a test for the ConvertToNoArgsConstructor recipe, as an example of how to write a test for an imperative recipe. +class UseLombokSetterTest implements RewriteTest { + + // Note, you can define defaults for the RecipeSpec and these defaults will be used for all tests. + // In this case, the recipe and the parser are common. See below, on how the defaults can be overridden + // per test. + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new UseLombokSetter()) + .parser(JavaParser.fromJavaVersion() + .logCompilationWarningsAndErrors(true)); + } + + int foo; + + @DocumentExample + @Test + void replaceSetter() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + + public void setFoo(int foo) { + this.foo = foo; + } + } + """, + """ + import lombok.Setter; + + class A { + + @Setter + int foo = 9; + } + """ + ) + ); + } + + @Test + void replaceSetterWhenArgNameWithUnderscore() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + + public void setFoo(int foo_) { + this.foo = foo_; + } + } + """, + """ + import lombok.Setter; + + class A { + + @Setter + int foo = 9; + } + """ + ) + ); + } + + @Test + void replaceSetterWhenArgNameWithUnderscoreAndUnqualifiedFieldAccess() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + + public void setFoo(int foo_) { + foo = foo_; + } + } + """, + """ + import lombok.Setter; + + class A { + + @Setter + int foo = 9; + } + """ + ) + ); + } + + @Test + void tolerantToNonstandardParameterNames() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + + public void setFoo(int fub) { + this.foo = fub; + } + } + """, + """ + import lombok.Setter; + + class A { + + @Setter + int foo = 9; + } + """ + ) + ); + } + + @Test + void replacePackageSetter() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + + void setFoo(int foo) { + this.foo = foo; + } + } + """, + """ + import lombok.AccessLevel; + import lombok.Setter; + + class A { + + @Setter(AccessLevel.PACKAGE) + int foo = 9; + } + """ + ) + ); + } + + @Test + void replaceProtectedSetter() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + + protected void setFoo(int foo) { + this.foo = foo; + } + } + """, + """ + import lombok.AccessLevel; + import lombok.Setter; + + class A { + + @Setter(AccessLevel.PROTECTED) + int foo = 9; + } + """ + ) + ); + } + + @Test + void replacePrivateSetter() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + + private void setFoo(int foo) { + this.foo = foo; + } + } + """, + """ + import lombok.AccessLevel; + import lombok.Setter; + + class A { + + @Setter(AccessLevel.PRIVATE) + int foo = 9; + } + """ + ) + ); + } + + @Test + void replaceJustTheMatchingSetter() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + + int ba; + + public A() { + ba = 1; + } + + public void setFoo(int foo) { + this.foo = foo; + } + + public void setMoo(int foo) {//method name wrong + this.foo = foo; + } + } + """, + """ + import lombok.Setter; + + class A { + + @Setter + int foo = 9; + + int ba; + + public A() { + ba = 1; + } + + public void setMoo(int foo) {//method name wrong + this.foo = foo; + } + } + """ + ) + ); + } + + @Test + void noChangeWhenMethodNameDoesntMatch() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + + public A() { + } + + public void setfoo(int foo) {//method name wrong + this.foo = foo; + } + } + """ + ) + ); + } + + @Test + void noChangeWhenParameterTypeDoesntMatch() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + + public A() { + } + + public void setFoo(long foo) {//parameter type wrong + this.foo = (int) foo; + } + } + """ + ) + ); + } + + @Test + void noChangeWhenFieldIsNotAssigned() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + int ba = 10; + + public A() { + } + + public void setFoo(int foo) { + int foo = foo; + } + } + """ + ) + ); + } + + @Test + void noChangeWhenDifferentFieldIsAssigned() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + int ba = 10; + + public A() { + } + + public void setFoo(int foo) { + this.ba = foo; //assigns wrong variable + } + } + """ + ) + ); + } + + @Test + void noChangeWhenSideEffects1() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + int ba = 10; + + public A() { + } + + public void setFoo(int foo) { + foo++;//does extra stuff + this.foo = foo; + } + } + """ + ) + ); + } + + @Test + void replacePrimitiveBoolean() { + rewriteRun(// language=java + java( + """ + class A { + + boolean foo = true; + + public void setFoo(boolean foo) { + this.foo = foo; + } + } + """, + """ + import lombok.Setter; + + class A { + + @Setter + boolean foo = true; + } + """ + ) + ); + } + + @Test + void replaceBoolean() { + rewriteRun(// language=java + java( + """ + class A { + + Boolean foo = true; + + public void setFoo(Boolean foo) { + this.foo = foo; + } + } + """, + """ + import lombok.Setter; + + class A { + + @Setter + Boolean foo = true; + } + """ + ) + ); + } + + @Test + void annotateOnlyFields() { + rewriteRun(// language=java + java( + """ + class A { + + int foo = 9; + + public void setFoo(int foo) { + this.foo = foo; + } + + public void unrelated1(int foo) { + } + public void unrelated2() { + int foo = 10; + } + } + """, + """ + import lombok.Setter; + + class A { + + @Setter + int foo = 9; + + public void unrelated1(int foo) { + } + public void unrelated2() { + int foo = 10; + } + } + """ + ) + ); + } + + @Test + void annotateOnlyFields2() { + rewriteRun(// language=java + java( + """ + class A { + + public void setFoo(int foo) { + this.foo = foo; + } + + public void unrelated1(int foo) { + } + public void unrelated2() { + int foo = 10; + } + + int foo = 9; + } + """, + """ + import lombok.Setter; + + class A { + + public void unrelated1(int foo) { + } + public void unrelated2() { + int foo = 10; + } + + @Setter + int foo = 9; + } + """ + ) + ); + } + + @Test + void noChangeNestedClassSetter() { + rewriteRun(// language=java + java( + """ + class Outer { + int foo = 9; + + class Inner { + public void setFoo(int foo) { + Outer.this.foo = foo; + } + } + } + """ + ) + ); + } +}