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

fix ReplaceConstantWithAnotherConstant not work when contains import conflict #5225

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+205 −13
Open

fix ReplaceConstantWithAnotherConstant not work when contains import conflict #5225

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
153 changes: 153 additions & 0 deletions ...-java-test/src/test/java/org/openrewrite/java/ReplaceConstantWithAnotherConstantTest.java
View file
Original file line number Diff line number Diff line change
@@ -275,4 +275,157 @@ void a() {
)
);
}

@Test
@Issue("https://github.com/openrewrite/rewrite/issues/5224")
void shouldFullyQualifyWhenNewTypeIsAmbiguous() {
rewriteRun(
spec -> spec.recipe(new ReplaceConstantWithAnotherConstant("foo1.Bar.QUX1", "foo2.Bar.QUX1")),
java(
"""
package foo1;

public class Bar {
public static final String QUX1 = "QUX1_FROM_FOO1";
public static final String QUX2 = "QUX1_FROM_FOO2";
}
"""
),
java(
"""
package foo2;

public class Bar {
public static final String QUX1 = "QUX1_FROM_FOO1";
}
"""
),
java(
"""
import foo1.Bar;

class Test {
void a() {
System.out.println(Bar.QUX1);
System.out.println(Bar.QUX2);
}
}
""",
"""
import foo1.Bar;

class Test {
void a() {
System.out.println(foo2.Bar.QUX1);
System.out.println(Bar.QUX2);
}
}
"""
)
);
}

@Test
@Issue("https://github.com/openrewrite/rewrite/issues/5224")
void shouldFullyQualifyWhenNewTypeIsAmbiguous2() {
rewriteRun(
spec -> spec.recipe(new ReplaceConstantWithAnotherConstant("foo1.Bar.QUX1", "foo3.Bar.QUX2")),
java(
"""
package foo1;

public class Bar {
public static final String QUX1 = "QUX_FROM_FOO1";
}
"""
),
java(
"""
package foo2;

public class Bar {
public static final String QUX2 = "QUX_FROM_FOO2";
}
"""
),
java(
"""
package foo3;

public class Bar {
public static final String QUX2 = "QUX_FROM_FOO3";
}
"""
),
java(
"""
import static foo1.Bar.QUX1;
import static foo2.Bar.QUX2;

class Test {
void a() {
System.out.println(QUX1);
System.out.println(QUX2);
}
}
""",
"""
import static foo2.Bar.QUX2;

class Test {
void a() {
System.out.println(foo3.Bar.QUX2);
System.out.println(QUX2);
}
}
"""
)
);
}

@Test
@Issue("https://github.com/openrewrite/rewrite/issues/5224")
void shouldFullyQualifyWhenNewTypeIsAmbiguous3() {
rewriteRun(
spec -> spec.recipe(new ReplaceConstantWithAnotherConstant("foo1.Bar.QUX1", "foo2.Bar.QUX1")),
java(
"""
package foo1;

public class Bar {
public static final String QUX1 = "QUX_FROM_FOO1";
}
"""
),
java(
"""
package foo2;

public class Bar {
public static final String QUX1 = "QUX_FROM_FOO2";
}
"""
),
java(
"""
import static foo1.Bar.QUX1;

class Test {
void a() {
System.out.println(QUX1);
}
}
""",
"""
import static foo2.Bar.QUX1;

class Test {
void a() {
System.out.println(QUX1);
}
}
"""
)
);
}
}
65 changes: 52 additions & 13 deletions rewrite-java/src/main/java/org/openrewrite/java/ReplaceConstantWithAnotherConstant.java
View file
Original file line number Diff line number Diff line change
@@ -20,10 +20,7 @@
import org.jspecify.annotations.Nullable;
import org.openrewrite.*;
import org.openrewrite.java.search.UsesType;
import org.openrewrite.java.tree.Expression;
import org.openrewrite.java.tree.J;
import org.openrewrite.java.tree.JavaType;
import org.openrewrite.java.tree.TypeUtils;
import org.openrewrite.java.tree.*;

@Value
@EqualsAndHashCode(callSuper = false)
@@ -57,21 +54,26 @@ private static class ReplaceConstantWithAnotherConstantVisitor extends JavaVisit

private final String existingOwningType;
private final String constantName;
private final JavaType.FullyQualified existingOwningTypeFqn;
private final JavaType.FullyQualified newOwningType;
private final JavaType.FullyQualified newTargetType;
private final String newConstantName;

public ReplaceConstantWithAnotherConstantVisitor(String existingFullyQualifiedConstantName, String fullyQualifiedConstantName) {
this.existingOwningType = existingFullyQualifiedConstantName.substring(0, existingFullyQualifiedConstantName.lastIndexOf('.'));
this.constantName = existingFullyQualifiedConstantName.substring(existingFullyQualifiedConstantName.lastIndexOf('.') + 1);
this.existingOwningTypeFqn = JavaType.ShallowClass.build(existingOwningType);
this.newOwningType = JavaType.ShallowClass.build(fullyQualifiedConstantName.substring(0, fullyQualifiedConstantName.lastIndexOf('.')));
this.newTargetType = JavaType.ShallowClass.build(fullyQualifiedConstantName);
this.newConstantName = fullyQualifiedConstantName.substring(fullyQualifiedConstantName.lastIndexOf('.') + 1);
}

@Override
public J visitFieldAccess(J.FieldAccess fieldAccess, ExecutionContext ctx) {
JavaType.Variable fieldType = fieldAccess.getName().getFieldType();
if (isConstant(fieldType)) {
return replaceFieldAccess(fieldAccess, fieldType);
JavaSourceFile sf = getCursor().firstEnclosing(JavaSourceFile.class);
return replaceFieldAccess(fieldAccess, fieldType, hasNoConflictingImport(sf));
}
return super.visitFieldAccess(fieldAccess, ctx);
}
@@ -80,36 +82,43 @@ public J visitFieldAccess(J.FieldAccess fieldAccess, ExecutionContext ctx) {
public J visitIdentifier(J.Identifier ident, ExecutionContext ctx) {
JavaType.Variable fieldType = ident.getFieldType();
if (isConstant(fieldType)) {
return replaceFieldAccess(ident, fieldType);
JavaSourceFile sf = getCursor().firstEnclosing(JavaSourceFile.class);
return replaceFieldAccess(ident, fieldType, hasNoConflictingImport(sf));
}
return super.visitIdentifier(ident, ctx);
}

private J replaceFieldAccess(Expression expression, JavaType.Variable fieldType) {
private J replaceFieldAccess(Expression expression, JavaType.Variable fieldType, boolean hasNoConflictingImport) {
JavaType owner = fieldType.getOwner();
while (owner instanceof JavaType.FullyQualified) {
maybeRemoveImport(((JavaType.FullyQualified) owner).getFullyQualifiedName());
owner = ((JavaType.FullyQualified) owner).getOwningClass();
}

if (expression instanceof J.Identifier) {
maybeAddImport(newOwningType.getFullyQualifiedName(), newConstantName, false);
String realName = hasNoConflictingImport ? newConstantName : newTargetType.getFullyQualifiedName();
if (hasNoConflictingImport) {
maybeAddImport(newOwningType.getFullyQualifiedName(), newConstantName, false);
}
J.Identifier identifier = (J.Identifier) expression;
return identifier
.withSimpleName(newConstantName)
.withFieldType(fieldType.withOwner(newOwningType).withName(newConstantName));
.withSimpleName(realName)
.withFieldType(fieldType.withOwner(newOwningType).withName(realName));
} else if (expression instanceof J.FieldAccess) {
maybeAddImport(newOwningType.getFullyQualifiedName(), false);
if (hasNoConflictingImport) {
maybeAddImport(newOwningType.getFullyQualifiedName(), false);
}
J.FieldAccess fieldAccess = (J.FieldAccess) expression;
Expression target = fieldAccess.getTarget();
J.Identifier name = fieldAccess.getName();
String realName = hasNoConflictingImport ? newOwningType.getClassName() : newOwningType.getFullyQualifiedName();
if (target instanceof J.Identifier) {
target = ((J.Identifier) target).withType(newOwningType).withSimpleName(newOwningType.getClassName());
target = ((J.Identifier) target).withType(newOwningType).withSimpleName(realName);
name = name
.withFieldType(fieldType.withOwner(newOwningType).withName(newConstantName))
.withSimpleName(newConstantName);
} else {
target = (((J.FieldAccess) target).getName()).withType(newOwningType).withSimpleName(newOwningType.getClassName());
target = (((J.FieldAccess) target).getName()).withType(newOwningType).withSimpleName(realName);
name = name
.withFieldType(fieldType.withOwner(newOwningType).withName(newConstantName))
.withSimpleName(newConstantName);
@@ -125,5 +134,35 @@ private boolean isConstant(JavaType.@Nullable Variable varType) {
return varType != null && TypeUtils.isOfClassType(varType.getOwner(), existingOwningType) &&
varType.getName().equals(constantName);
}

private boolean hasNoConflictingImport(@Nullable JavaSourceFile sf) {
return hasNoConflictingImport(sf, newOwningType) && hasNoConflictingImport(sf, newTargetType);
}

private boolean hasNoConflictingImport(@Nullable JavaSourceFile sf, JavaType.FullyQualified targetType) {
if (sf == null || targetType == null) {
return true;
}

for (J.Import anImport : sf.getImports()) {
if (anImport.isStatic()) {
JavaType.FullyQualified fqn = TypeUtils.asFullyQualified(anImport.getQualid().getTarget().getType());
if (anImport.getQualid().getSimpleName().equals(newConstantName) && !TypeUtils.isOfClassType(fqn, newOwningType.getFullyQualifiedName())) {
if (!anImport.getQualid().getSimpleName().equals(constantName) || !TypeUtils.isOfClassType(fqn, existingOwningTypeFqn.getFullyQualifiedName())) {
return false;
}
}
} else {
JavaType.FullyQualified currType = TypeUtils.asFullyQualified(anImport.getQualid().getType());
if (currType != null &&
!TypeUtils.isOfType(currType, targetType) &&
currType.getClassName().equals(targetType.getClassName())) {
return false;
}
}

}
return true;
}
}
}