Skip to content

Commit 7ebbf72

Browse files
timtebeekclaude
andauthored
Fix Lombok recipes incorrectly replacing getters/setters accessing other objects' fields (#873)
* Fix UseLombokGetter incorrectly replacing getters that access fields of other objects Fixes #872 The UseLombokGetter recipe was incorrectly removing getter methods when the method body referenced a field of another object (e.g., `return sample.number;` where `sample` is a field of the current class). The issue was in LombokUtils.isGetter() when handling J.FieldAccess expressions. Previously, it checked if the target object was a field of the current class, then incorrectly assumed the accessed field should be annotated with @Getter. The fix ensures that only `this.field` patterns are handled for J.FieldAccess expressions, preventing the recipe from replacing getters that access fields of other objects. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> * Apply same fix to UseLombokSetter for consistency The setter logic had the same issue as the getter - it was incorrectly replacing setter methods when they set fields of other objects (e.g., `sample.number = value;`). Applied the same fix: only handle `this.field` patterns for J.FieldAccess expressions in the isSetter() method. Added test case to verify the fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]> --------- Co-authored-by: Claude <[email protected]>
1 parent e83ed0d commit 7ebbf72

File tree

3 files changed

+68
-13
lines changed

3 files changed

+68
-13
lines changed

src/main/java/org/openrewrite/java/migrate/lombok/LombokUtils.java

Lines changed: 18 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -73,15 +73,18 @@ static boolean isGetter(Cursor cursor, AnnotationService service) {
7373
} else if (returnExpression instanceof J.FieldAccess) {
7474
J.FieldAccess fieldAccess = (J.FieldAccess) returnExpression;
7575
Expression target = fieldAccess.getTarget();
76-
if (target instanceof J.Identifier && ((J.Identifier) target).getFieldType() != null &&
77-
declaringType == ((J.Identifier) target).getFieldType().getOwner()) {
78-
// Don't replace instance methods accessing static fields
76+
// Only handle "this.field" pattern, not "someObject.field"
77+
if (target instanceof J.Identifier && "this".equals(((J.Identifier) target).getSimpleName())) {
78+
// Check field is declared on method type
7979
if (fieldAccess.getName().getFieldType() != null &&
80-
fieldAccess.getName().getFieldType().hasFlags(Flag.Static)) {
81-
return false;
80+
declaringType == fieldAccess.getName().getFieldType().getOwner()) {
81+
// Don't replace instance methods accessing static fields
82+
if (fieldAccess.getName().getFieldType().hasFlags(Flag.Static)) {
83+
return false;
84+
}
85+
// Check return: type and matching field name
86+
return hasMatchingTypeAndGetterName(method, fieldAccess.getType(), fieldAccess.getSimpleName());
8287
}
83-
// Check return: type and matching field name
84-
return hasMatchingTypeAndGetterName(method, fieldAccess.getType(), fieldAccess.getSimpleName());
8588
}
8689
}
8790
return false;
@@ -172,12 +175,14 @@ static boolean isSetter(Cursor cursor, AnnotationService service) {
172175
J.FieldAccess assignedField = (J.FieldAccess) variable;
173176
if (hasMatchingSetterMethodName(method, assignedField.getSimpleName())) {
174177
Expression target = assignedField.getTarget();
175-
// Check field is declared on method type
176-
if (target instanceof J.Identifier && ((J.Identifier) target).getFieldType() != null &&
177-
declaringType == ((J.Identifier) target).getFieldType().getOwner()) {
178-
// Don't replace instance methods accessing static fields
179-
return assignedField.getName().getFieldType() != null &&
180-
!assignedField.getName().getFieldType().hasFlags(Flag.Static);
178+
// Only handle "this.field" pattern, not "someObject.field"
179+
if (target instanceof J.Identifier && "this".equals(((J.Identifier) target).getSimpleName())) {
180+
// Check field is declared on method type
181+
if (assignedField.getName().getFieldType() != null &&
182+
declaringType == assignedField.getName().getFieldType().getOwner()) {
183+
// Don't replace instance methods accessing static fields
184+
return !assignedField.getName().getFieldType().hasFlags(Flag.Static);
185+
}
181186
}
182187
}
183188
}

src/test/java/org/openrewrite/java/migrate/lombok/UseLombokGetterTest.java

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -613,4 +613,29 @@ int getField() {
613613
)
614614
);
615615
}
616+
617+
@Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/872")
618+
@Test
619+
void noChangeWhenMethodReturnsFieldOfAnotherObject() {
620+
rewriteRun(// language=java
621+
java(
622+
"""
623+
public class A {
624+
625+
private class Sample {
626+
public Long number;
627+
}
628+
629+
private class Inner {
630+
static Sample sample = null;
631+
632+
private Long getNumber() {
633+
return sample.number;
634+
}
635+
}
636+
}
637+
"""
638+
)
639+
);
640+
}
616641
}

src/test/java/org/openrewrite/java/migrate/lombok/UseLombokSetterTest.java

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -622,4 +622,29 @@ void setField(int field) {
622622
)
623623
);
624624
}
625+
626+
@Issue("https://github.com/openrewrite/rewrite-migrate-java/issues/872")
627+
@Test
628+
void noChangeWhenMethodSetsFieldOfAnotherObject() {
629+
rewriteRun(// language=java
630+
java(
631+
"""
632+
public class A {
633+
634+
private class Sample {
635+
public Long number;
636+
}
637+
638+
private class Inner {
639+
static Sample sample = null;
640+
641+
private void setNumber(Long value) {
642+
sample.number = value;
643+
}
644+
}
645+
}
646+
"""
647+
)
648+
);
649+
}
625650
}

0 commit comments

Comments
 (0)