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 778ed30c5..f27968dae 100644 --- a/src/main/java/org/openrewrite/java/migrate/lombok/LombokUtils.java +++ b/src/main/java/org/openrewrite/java/migrate/lombok/LombokUtils.java @@ -24,8 +24,10 @@ import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JavaType; +import org.openrewrite.java.tree.Statement; import java.util.List; +import java.util.stream.Collectors; import static lombok.AccessLevel.*; import static org.openrewrite.java.tree.J.Modifier.Type.*; @@ -156,6 +158,42 @@ private static boolean hasMatchingSetterMethodName(J.MethodDeclaration method, S return method.getSimpleName().equals("set" + StringUtils.capitalize(simpleName)); } + public static boolean isEffectivelySetter(J.MethodDeclaration method) { + boolean isVoid = "void".equals(method.getType().toString()); + List actualParameters = method.getParameters().stream() + .filter(s -> !(s instanceof J.Empty)) + .collect(Collectors.toList()); + boolean oneParam = actualParameters.size() == 1; + if (!isVoid || !oneParam) + return false; + + J.VariableDeclarations variableDeclarations = (J.VariableDeclarations) actualParameters.get(0); + J.VariableDeclarations.NamedVariable param = variableDeclarations.getVariables().get(0); + String paramName = param.getName().toString(); + + boolean singularStatement = method.getBody() != null //abstract methods can be null + && method.getBody().getStatements().size() == 1 && + method.getBody().getStatements().get(0) instanceof J.Assignment; + + if (!singularStatement) { + return false; + } + J.Assignment assignment = (J.Assignment) method.getBody().getStatements().get(0); + + if (assignment.getVariable() instanceof J.FieldAccess || assignment.getVariable() instanceof J.Identifier) { + JavaType fieldType = assignment.getVariable().getType(); + // assigned value is exactly the parameter + return assignment.getAssignment().toString().equals(paramName) && + param.getType() != null && + param.getType().equals(fieldType); // type of parameter and field have to match + } + return false; + } + + public static String deriveSetterMethodName(JavaType.Variable fieldType) { + return "set" + StringUtils.capitalize(fieldType.getName()); + } + static AccessLevel getAccessLevel(J.MethodDeclaration methodDeclaration) { if (methodDeclaration.hasModifier(Public)) { return PUBLIC; diff --git a/src/main/java/org/openrewrite/java/migrate/lombok/NormalizeSetter.java b/src/main/java/org/openrewrite/java/migrate/lombok/NormalizeSetter.java new file mode 100644 index 000000000..b92157b4f --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/lombok/NormalizeSetter.java @@ -0,0 +1,181 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * 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.Value; +import org.jspecify.annotations.Nullable; +import org.openrewrite.ExecutionContext; +import org.openrewrite.ScanningRecipe; +import org.openrewrite.Tree; +import org.openrewrite.TreeVisitor; +import org.openrewrite.java.ChangeMethodName; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaType; + +import java.util.ArrayList; +import java.util.List; +import java.util.stream.Collectors; + +@Value +@EqualsAndHashCode(callSuper = false) +public class NormalizeSetter extends ScanningRecipe { + + private final static String DO_NOT_RENAME = "DO_NOT_RENAME"; + + @Override + public String getDisplayName() { + return "Rename setter methods to fit lombok"; + } + + @Override + public String getDescription() { + return "Rename methods that are effectively setter to the name lombok would give them.\n" + + "Limitations:\n" + + " - If two methods in a class are effectively the same setter then one's name will be corrected and the others name will be left as it is.\n" + + " - If the correct name for a method is already taken by another method then the name will not be corrected.\n" + + " - Method name swaps or circular renaming within a class cannot be performed because the names block each other. " + + "E.g. `int getFoo() { return ba; } int getBa() { return foo; }` stays as it is."; + } + + public static class MethodAcc { + List renameRecords = new ArrayList<>(); + } + + @Value + private static class RenameRecord { + String pathToClass_; + String methodName_; + String parameterType_; + String newMethodName_; + } + + @Override + public MethodAcc getInitialValue(ExecutionContext ctx) { + return new MethodAcc(); + } + + @Override + public TreeVisitor getScanner(MethodAcc acc) { + return new JavaIsoVisitor() { + + @Override + public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ctx) { + + List doNotRename = classDecl.getBody().getStatements().stream() + .filter(s -> s instanceof J.MethodDeclaration) + .map(s -> (J.MethodDeclaration) s) + .map(J.MethodDeclaration::getSimpleName) + .collect(Collectors.toList()); + + getCursor().putMessage(DO_NOT_RENAME, doNotRename); + + super.visitClassDeclaration(classDecl, ctx); + + return classDecl; + } + + @Override + public J.MethodDeclaration visitMethodDeclaration(J.MethodDeclaration method, ExecutionContext ctx) { + assert method.getMethodType() != null; + + if (!LombokUtils.isEffectivelySetter(method)) { + return method; + } + + // return early if the method overrides another + // if the project defined both the original and the overridden method, + // then the renaming of the "original" in the base class will cover the override + if (method.getLeadingAnnotations().stream().anyMatch(a -> "Override".equals(a.getSimpleName()))) { + return method; + } + + JavaType.Variable fieldType = extractVariable(method); + + String expectedMethodName = LombokUtils.deriveSetterMethodName(fieldType); + String parameterType = fieldType.getType().toString(); + String actualMethodName = method.getSimpleName(); + + // If method already has the name it should have, then nothing to be done + if (expectedMethodName.equals(actualMethodName)) { + return method; + } + + // If the desired method name is already taken by an existing method, the current method cannot be renamed + List doNotRename = getCursor().getNearestMessage(DO_NOT_RENAME); + assert doNotRename != null; + if (doNotRename.contains(expectedMethodName)) { + return method; + } + //WON'T DO: there is a rare edge case, that is not addressed yet. + // If `getFoo()` returns `ba` and `getBa()` returns `foo` then neither will be renamed. + // This could be fixed by compiling a list of planned changes and doing a soundness check (and not renaming sequentially, or rather introducing temporary method names) + // At this point I don't think it's worth the effort. + + + String pathToClass = method.getMethodType().getDeclaringType().getFullyQualifiedName().replace('$', '.'); + //todo write separate recipe for merging effective setters + acc.renameRecords.add( + new RenameRecord( + pathToClass, + actualMethodName, + parameterType, + expectedMethodName + ) + ); + doNotRename.remove(actualMethodName);//actual method name becomes available again + doNotRename.add(expectedMethodName);//expected method name now blocked + return method; + } + + private JavaType.@Nullable Variable extractVariable(J.MethodDeclaration method) { + J.Assignment assignment_ = (J.Assignment) method.getBody().getStatements().get(0); + + JavaType.Variable fieldType; + if (assignment_.getVariable() instanceof J.FieldAccess) { + J.FieldAccess fieldAccess = (J.FieldAccess) assignment_.getVariable(); + fieldType = fieldAccess.getName().getFieldType(); + } else if (assignment_.getVariable() instanceof J.Identifier) { + J.Identifier fieldAccess = (J.Identifier) assignment_.getVariable(); + fieldType = fieldAccess.getFieldType(); + } else { + //only those types above are possible, see LombokUtils::isEffectivelySetter + throw new IllegalStateException("Unexpected type for returned variable"); + } + return fieldType; + } + }; + } + + @Override + public TreeVisitor getVisitor(MethodAcc acc) { + + return new TreeVisitor() { + + @Override + public @Nullable Tree visit(@Nullable Tree tree, ExecutionContext ctx) { + + for (RenameRecord rr : acc.renameRecords) { + String methodPattern = String.format("%s %s(%s)", rr.pathToClass_, rr.methodName_, rr.parameterType_); + tree = new ChangeMethodName(methodPattern, rr.newMethodName_, true, null) + .getVisitor().visit(tree, ctx); + } + return tree; + } + }; + } +} diff --git a/src/test/java/org/openrewrite/java/migrate/lombok/NormalizeSetterTest.java b/src/test/java/org/openrewrite/java/migrate/lombok/NormalizeSetterTest.java new file mode 100644 index 000000000..8f55ca25d --- /dev/null +++ b/src/test/java/org/openrewrite/java/migrate/lombok/NormalizeSetterTest.java @@ -0,0 +1,604 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://www.apache.org/licenses/LICENSE-2.0 + *

+ * 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.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; + +class NormalizeSetterTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new NormalizeSetter()); + } + + @DocumentExample + @Test + void renameInSingleClass() { + rewriteRun(// language=java + java( + """ + class A { + int foo = 9; + public void storeFoo(int foo) { + this.foo = foo; + } + } + """, + """ + class A { + int foo = 9; + public void setFoo(int foo) { + this.foo = foo; + } + } + """ + ) + ); + } + + @Test + void renameWithoutFieldAccess() { + rewriteRun(// language=java + java( + """ + class A { + int foo = 9; + public void storeFoo(int newfoo) { + foo = newfoo; + } + } + """, + """ + class A { + int foo = 9; + public void setFoo(int newfoo) { + foo = newfoo; + } + } + """ + ) + ); + } + + @Test + void renameInSingleClassWhitespace() { + rewriteRun(// language=java + java( + """ + class A { + int foo = 9; + public void storeFoo( int foo ) { + this .foo = foo; + } + } + """, + """ + class A { + int foo = 9; + public void setFoo( int foo ) { + this .foo = foo; + } + } + """ + ) + ); + } + + @Test + void renamePrimitiveBooleanInSingleClass() { + rewriteRun(// language=java + java( + """ + class A { + boolean foo; + void storeFoo(boolean foo) { this.foo = foo; } + } + """, + """ + class A { + boolean foo; + void setFoo(boolean foo) { this.foo = foo; } + } + """ + ) + ); + } + + @Test + void renameClassBooleanInSingleClass() { + rewriteRun(// language=java + java( + """ + class A { + Boolean foo; + void storeFoo(Boolean foo) { this.foo = foo; } + } + """, + """ + class A { + Boolean foo; + void setFoo(Boolean foo) { this.foo = foo; } + } + """ + ) + ); + } + + @Test + void noBoxing1() { + rewriteRun(// language=java + java( + """ + class A { + Boolean Foo; + void storeFoo(boolean foo) { this.foo = foo; } + } + """ + ) + ); + } + + @Test + void noBoxing2() { + rewriteRun(// language=java + java( + """ + class A { + boolean Foo; + void storeFoo(Boolean foo) { this.foo = foo; } + } + """ + ) + ); + } + + @Test + void renameAcrossClasses() { + rewriteRun(// language=java + java( + """ + class A { + int foo = 9; + void storeFoo(int foo) { this.foo = foo; } + } + """, + """ + class A { + int foo = 9; + void setFoo(int foo) { this.foo = foo; } + } + """ + ),// language=java + java( + """ + class B { + void useIt() { + var a = new A(); + a.storeFoo(4); + } + } + """, + """ + class B { + void useIt() { + var a = new A(); + a.setFoo(4); + } + } + """ + ) + ); + } + + @Test + void shouldNotChangeOverridesOfExternalMethods() { + rewriteRun(// language=java + java( + """ + + import java.util.Date; + + class A extends Date { + + private long foo; + + @Override + public long setTime(long time) { + this.foo = time; + } + } + """ + ) + ); + } + + @Test + void withoutPackage() { + rewriteRun(// language=java + java( + """ + class A { + + private long foo; + + public void setTime(long foo) { + this.foo = foo; + } + } + """, + """ + class A { + + private long foo; + + public void setFoo(long foo) { + this.foo = foo; + } + } + """ + ) + ); + } + + @Test + void shouldChangeOverridesOfInternalMethods() { + rewriteRun(// language=java + java( + """ + class A { + + private long foo; + + public void setTime(long foo) { + this.foo = foo; + } + } + """, + """ + class A { + + private long foo; + + public void setFoo(long foo) { + this.foo = foo; + } + } + """ + ),// language=java + java( + """ + class B extends A { + + @Override + public void setTime(long foo) { + } + } + """, + """ + class B extends A { + + @Override + public void setFoo(long foo) { + } + } + """ + ) + ); + } + + @Test + void shouldNotRenameToExistingMethods() { + rewriteRun(// language=java + java( + """ + + class A { + + private long foo; + + public void setTime(long foo) { + this.foo = foo; + } + + public void setFoo(long foo) { + } + } + """ + ) + ); + } + + /** + * If two methods are effectively the same setter then only one can be renamed. + * Renaming both would result in a duplicate method definition, so we cannot do this. + * Ideally the other effective setter would have their usages renamed but be themselves deleted... + * TODO: create a second cleanup recipe that identifies redundant Setters (isEffectiveSetter + field already has the setter annotation) + * and redirects their usage (ChangeMethodName with both flags true) and then deletes them. + */ + @Test + void shouldNotRenameTwoToTheSame() { + rewriteRun(// language=java + java( + """ + + class A { + + private long foo; + + public void firstToBeRenamed(long foo) { + this.foo = foo; + } + + public void secondToBeRenamed(long foo) { + this.foo = foo; + } + } + """, + """ + + class A { + + private long foo; + + public void setFoo(long foo) { + this.foo = foo; + } + + public void secondToBeRenamed(long foo) { + this.foo = foo; + } + } + """ + ) + ); + } + + /** + * Methods in inner classes should be renamed as well. + */ + @Test + void shouldWorkOnInnerClasses() { + rewriteRun(// language=java + java( + """ + + class A { + + class B { + + private long foo; + + public void storeFoo(long foo) { + this.foo = foo; + } + } + } + """, + """ + + class A { + + class B { + + private long foo; + + public void setFoo(long foo) { + this.foo = foo; + } + } + } + """ + ) + ); + } + + @Test + void shouldWorkOnInnerClasses2() { + rewriteRun(// language=java + java( + """ + + class A { + + class B { + + class C { + + private long foo; + + public void giveFoo(long foo) { + this.foo = foo; + } + }} + } + """, + """ + + class A { + + class B { + + class C { + + private long foo; + + public void setFoo(long foo) { + this.foo = foo; + } + }} + } + """ + ) + ); + } + + /** + * Methods on top level should be renamed just as well when there is an inner class. + */ + @Test + void shouldWorkDespiteInnerClassesSameNameMethods() { + rewriteRun(// language=java + java( + """ + + class A { + + private long foo; + + public void storeFoo(long foo) { + this.foo = foo; + } + + class B { + + private long foo; + + public void storeFoo(long foo) { + this.foo = foo; + } + } + } + """, + """ + + class A { + + private long foo; + + public void setFoo(long foo) { + this.foo = foo; + } + + class B { + + private long foo; + + public void setFoo(long foo) { + this.foo = foo; + } + } + } + """ + ) + ); + } + + /** + * Methods on top level should be renamed just as well when there is an inner class. + */ + @Test + void shouldWorkDespiteInnerClassesDifferentNameMethods() { + rewriteRun(// language=java + java( + """ + + class A { + + private long foo; + + public void storeFoo(long foo) { + this.foo = foo; + } + + class B { + + private long ba; + + public void storeBa(long ba) { + this.ba = ba; + } + } + } + """, + """ + + class A { + + private long foo; + + public void setFoo(long foo) { + this.foo = foo; + } + + class B { + + private long ba; + + public void setBa(long ba) { + this.ba = ba; + } + } + } + """ + ) + ); + } + + /** + * If existing method names need to be rotated in a loop the recipe should still work. + * For now this is not planned. + */ + @Test + void shouldWorkOnCircleCasesButDoesntYet() { + rewriteRun(// language=java + java( + """ + + class A { + + int foo; + + int bar; + + public void setBar(int bar) { + this.foo = bar; + } + + public void setFoo(int foo) { + this.bar = foo; + } + + } + """ +// , +// """ +// +// class A { +// +// int foo; +// +// int bar; +// +// public void setFoo(int foo) { +// this.foo = foo; +// } +// +// public void setBar(int bar) { +// this.bar = bar; +// } +// +// } +// """ + ) + ); + } + +}