Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling of interfaces in LombokValueToRecord #452

Merged
merged 6 commits into from
Apr 24, 2024
Merged

Improve handling of interfaces in LombokValueToRecord #452

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c5e8a58
fix: exclude only conflicting interfaces
holgpar Apr 3, 2024
dcb81af
Merge branch 'main' into dontConvertClassesThatImplementInterfaces
timtebeek Apr 9, 2024
78ee454
Fix indentation of text blocks in tests
timtebeek Apr 24, 2024
d2cf6af
Apply formatter to LombokValueToRecord.java
timtebeek Apr 24, 2024
cdfc04a
Slight polish
timtebeek Apr 24, 2024
bee3830
Merge branch 'main' into dontConvertClassesThatImplementInterfaces
timtebeek Apr 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 47 additions & 16 deletions src/main/java/org/openrewrite/java/migrate/lombok/LombokValueToRecord.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,36 +99,67 @@ public J.ClassDeclaration visitClassDeclaration(J.ClassDeclaration classDecl, Ex
return cd;
}

assert cd.getType() != null : "Class type must not be null";
assert cd.getType() != null : "Class type must not be null"; // Checked in isRelevantClass
Set<String> memberVariableNames = getMemberVariableNames(memberVariables);
if (implementsConflictingInterfaces(cd, memberVariableNames)) {
return cd;
}

acc.putIfAbsent(
cd.getType().getFullyQualifiedName(),
getMemberVariableNames(memberVariables));
memberVariableNames);

return cd;
}

private boolean isRelevantClass(J.ClassDeclaration classDeclaration) {
List<J.Annotation> allAnnotations = classDeclaration.getAllAnnotations();
return classDeclaration.getType() != null
&& !J.ClassDeclaration.Kind.Type.Record.equals(classDeclaration.getKind())
&& classDeclaration.getAllAnnotations().stream()
.allMatch(ann -> LOMBOK_VALUE_MATCHER.matches(ann) && (ann.getArguments() == null || ann.getArguments().isEmpty()))
&& !hasGenericTypeParameter(classDeclaration)
&& classDeclaration.getBody().getStatements().stream().allMatch(this::isRecordCompatibleField)
&& !hasIncompatibleModifier(classDeclaration)
&& !implementsInterfaces(classDeclaration);
&& !J.ClassDeclaration.Kind.Type.Record.equals(classDeclaration.getKind())
&& !allAnnotations.isEmpty()
&& allAnnotations.stream().allMatch(ann -> LOMBOK_VALUE_MATCHER.matches(ann) && (ann.getArguments() == null || ann.getArguments().isEmpty()))
&& !hasGenericTypeParameter(classDeclaration)
&& classDeclaration.getBody().getStatements().stream().allMatch(this::isRecordCompatibleField)
&& !hasIncompatibleModifier(classDeclaration);
}

/**
* If the class target class implements an interface, transforming it to a record will not work in general,
* because the record access methods do not have the "get" prefix.
*
* @param classDeclaration
* @return
* @return true if the class implements an interface with a getter method based on a member variable
*/
private boolean implementsInterfaces(J.ClassDeclaration classDeclaration) {
private boolean implementsConflictingInterfaces(J.ClassDeclaration classDeclaration, Set<String> memberVariableNames) {
List<TypeTree> classDeclarationImplements = classDeclaration.getImplements();
return !(classDeclarationImplements == null || classDeclarationImplements.isEmpty());
if (classDeclarationImplements == null) {
return false;
}
return classDeclarationImplements.stream().anyMatch(implemented -> {
JavaType type = implemented.getType();
if (type instanceof JavaType.FullyQualified) {
return isConflictingInterface((JavaType.FullyQualified) type, memberVariableNames);
} else {
return false;
}
});
}

private static boolean isConflictingInterface(JavaType.FullyQualified implemented, Set<String> memberVariableNames) {
boolean hasConflictingMethod = implemented.getMethods().stream()
.map(JavaType.Method::getName)
.map(LombokValueToRecordVisitor::getterMethodNameToFluentMethodName)
.anyMatch(memberVariableNames::contains);
if (hasConflictingMethod) {
return true;
}
List<JavaType.FullyQualified> superInterfaces = implemented.getInterfaces();
if (superInterfaces != null) {
return superInterfaces.stream().anyMatch(i -> isConflictingInterface(i, memberVariableNames));
}
return false;
}

private boolean hasGenericTypeParameter(J.ClassDeclaration classDeclaration) {
List<J.TypeParameter> typeParameters = classDeclaration.getTypeParameters();
return typeParameters != null && !typeParameters.isEmpty();
Expand Down Expand Up @@ -182,10 +213,10 @@ private static class LombokValueToRecordVisitor extends JavaIsoVisitor<Execution
private static final String TO_STRING_MEMBER_DELIMITER = "\", \" +\n";
private static final String STANDARD_GETTER_PREFIX = "get";

private final Boolean useExactToString;
private final @Nullable Boolean useExactToString;
private final Map<String, Set<String>> recordTypeToMembers;

public LombokValueToRecordVisitor(Boolean useExactToString, Map<String, Set<String>> recordTypeToMembers) {
public LombokValueToRecordVisitor(@Nullable Boolean useExactToString, Map<String, Set<String>> recordTypeToMembers) {
this.useExactToString = useExactToString;
this.recordTypeToMembers = recordTypeToMembers;
}
Expand Down Expand Up @@ -220,8 +251,8 @@ private boolean isMethodInvocationOnRecordTypeClassMember(J.MethodInvocation met
String classFqn = classType.getFullyQualifiedName();

return recordTypeToMembers.containsKey(classFqn)
&& methodName.startsWith(STANDARD_GETTER_PREFIX)
&& recordTypeToMembers.get(classFqn).contains(getterMethodNameToFluentMethodName(methodName));
&& methodName.startsWith(STANDARD_GETTER_PREFIX)
&& recordTypeToMembers.get(classFqn).contains(getterMethodNameToFluentMethodName(methodName));
}

private static boolean isClassExpression(@Nullable Expression expression) {
Expand Down
142 changes: 101 additions & 41 deletions src/test/java/org/openrewrite/java/migrate/lombok/LombokValueToRecordTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,18 +48,18 @@ void valueAnnotatedClassWithUseExactOptionKeepsLombokToString() {
java(
"""
import lombok.Value;

@Value
public class Test {
String field1;

String field2;
}
""",
"""
public record Test(
String field1,

String field2) {
@Override
public String toString() {
Expand All @@ -84,17 +84,17 @@ void convertOnlyValueAnnotatedClassWithoutDefaultValuesToRecord() {
java(
"""
package example;

import lombok.Value;

@Value
public class A {
String test;
}
""",
"""
package example;

public record A(
String test) {
}
Expand All @@ -103,31 +103,31 @@ public record A(
java(
"""
package example;

public class UserOfA {

private final A record;

public UserOfA() {
this.record = new A("some value");
}

public String getRecordValue() {
return record.getTest();
}
}
""",
"""
package example;

public class UserOfA {

private final A record;

public UserOfA() {
this.record = new A("some value");
}

public String getRecordValue() {
return record.test();
}
Expand All @@ -148,7 +148,7 @@ void onlyRemoveAnnotationFromRecords() {

import lombok.ToString;
import lombok.Value;

@Value
public class A {
String test;
Expand All @@ -162,7 +162,7 @@ public class B {
""",
"""
package example;

import lombok.ToString;
import lombok.Value;

Expand Down Expand Up @@ -209,6 +209,38 @@ record B(
"""
)
);


}

@Test
void interfaceIsImplementedThatDoesNotDefineFieldGetter() {
//language=java
rewriteRun(
s -> s.typeValidationOptions(TypeValidation.none()),
java(
"""
package example;

import lombok.Value;
import java.io.Serializable;

@Value
public class A implements Serializable {
String test;
}
""",
"""
package example;

import java.io.Serializable;

public record A(
String test) implements Serializable {
}
"""
)
);
}

@Nested
Expand All @@ -220,11 +252,11 @@ void classWithExplicitConstructor() {
java(
"""
import lombok.Value;

@Value
public class A {
String test;

public A() {
this.test = "test";
}
Expand All @@ -243,10 +275,10 @@ void classWithFieldAnnotations() {
"""
import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.Value;

@Value
public class A {

@JsonProperty
String test;
}
Expand All @@ -262,11 +294,11 @@ void classWithExplicitMethods() {
java(
"""
import lombok.Value;

@Value
public class A {
String test;

public String getTest() {
return test;
}
Expand All @@ -283,7 +315,7 @@ void genericClass() {
java(
"""
import lombok.Value;

@Value
public class A<T extends Object> {
T test;
Expand All @@ -301,7 +333,7 @@ void nonJava17Class() {
java(
"""
import lombok.Value;

@Value
public class A {
String test;
Expand All @@ -321,7 +353,7 @@ void classWithMultipleLombokAnnotations() {
"""
import lombok.Value;
import lombok.experimental.Accessors;

@Value
@Accessors(fluent = true)
public class A {
Expand Down Expand Up @@ -352,7 +384,7 @@ void classWithStaticField() {
java(
"""
import lombok.Value;

@Value
public class A {
static String disqualifyingField;
Expand Down Expand Up @@ -406,24 +438,52 @@ public class A {
}

@Test
void classImplementingInterfaces() {
void classImplementingConflictingInterface() {
//language=java
rewriteRun(
java(
"""
package example;

import lombok.Value;

interface I {
String getTest();
}

@Value
public class A implements I {
String test;
}
""")
"""
package example;

import lombok.Value;

interface I {
String getTest();
}

@Value
public class A implements I {
String test;
}
"""
)
);

}

@Test
void classImplementingConflictingInterfaceWithInheritance() {
//language=java
rewriteRun(
java(
"""
package example;

import lombok.Value;

interface I {
String getTest();
}

interface J extends I {
}

@Value
public class A implements J {
String test;
}
"""
)
);

}
Expand Down