Skip to content

Commit

Permalink
SemanticallyEqual should not require non-null J.Identifier#fieldType
Browse files Browse the repository at this point in the history
  • Loading branch information
@knutwannheden
knutwannheden committed Nov 26, 2024
1 parent 84b92ec commit e536ed2
Show file tree
Hide file tree
Showing 3 changed files with 49 additions and 20 deletions.
16 changes: 16 additions & 0 deletions ...test/src/test/java/org/openrewrite/java/cleanup/SimplifyBooleanExpressionVisitorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import org.openrewrite.Issue;
import org.openrewrite.test.RecipeSpec;
import org.openrewrite.test.RewriteTest;
import org.openrewrite.test.TypeValidation;

import static org.openrewrite.java.Assertions.java;
import static org.openrewrite.test.RewriteTest.toRecipe;
Expand Down Expand Up @@ -61,6 +62,21 @@ public class A {
);
}

@DocumentExample
@Test
void foo() {
rewriteRun(
spec -> spec.typeValidationOptions(TypeValidation.builder().identifiers(false).build()),
java(
"""
public class A {
boolean a = null instanceof Unknown || null instanceof java.lang.Unknown;
}
"""
)
);
}

@Test
void simplifyEqualsLiteralFalseIf() {
rewriteRun(
Expand Down
36 changes: 22 additions & 14 deletions rewrite-java/src/main/java/org/openrewrite/java/search/SemanticallyEqual.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -681,42 +681,49 @@ public J.ForLoop.Control visitForControl(J.ForLoop.Control control, J j) {
@Override
public J.Identifier visitIdentifier(J.Identifier identifier, J j) {
if (isEqual.get()) {
Map<String, String> scope = variableScope.peek();
if (j instanceof J.FieldAccess) {
J.FieldAccess field = (J.FieldAccess) j;
Expression fieldTarget = field.getTarget();

// Consider implicit-this "a" equivalent to "this.a"
// Definitely does not account for every possible edge case around "this", but handles the common case
if(fieldTarget instanceof J.Identifier && "this".equals(((J.Identifier) fieldTarget).getSimpleName())) {
if (fieldTarget instanceof J.Identifier && "this".equals(((J.Identifier) fieldTarget).getSimpleName())) {
visit(identifier, field.getName());
return identifier;
}

// Identifier name and type aren't the same as the field access they are obviously different
if(!identifier.getSimpleName().equals(field.getSimpleName()) || !TypeUtils.isOfType(identifier.getFieldType(), field.getName().getFieldType())) {
if (!identifier.getSimpleName().equals(field.getSimpleName()) || !TypeUtils.isOfType(identifier.getFieldType(), field.getName().getFieldType())) {
isEqual.set(false);
return identifier;
}

// Either both are variables or both are not (e.g. types)
if ((identifier.getFieldType() == null) ^ (field.getName().getFieldType() == null)) {
isEqual.set(false);
return identifier;
}

String identifierTypeFqn = identifier.getType() instanceof JavaType.FullyQualified ? ((JavaType.FullyQualified)identifier.getType()).getFullyQualifiedName() : "";
String identifierTypeFqn = identifier.getType() instanceof JavaType.FullyQualified ? ((JavaType.FullyQualified) identifier.getType()).getFullyQualifiedName() : "";
// If the field access is a static field access, the identifier must be a class reference
// e.g.: Pattern is semantically equal to java.util.regex.Pattern
if (field.isFullyQualifiedClassReference(identifierTypeFqn)) {
return identifier;
}

// Similarly, might be comparing a statically imported field to a fully qualified
// e.g. java.util.regex.Pattern.CASE_INSENSITIVE is semantically equal to CASE_INSENSITIVE when the latter is a static import of the former
JavaType identifierOwner = identifier.getFieldType().getOwner();
String identifierOwnerFqn = identifierOwner instanceof JavaType.FullyQualified ? ((JavaType.FullyQualified) identifierOwner).getFullyQualifiedName() : "";
if(field.getTarget() instanceof J.FieldAccess && ((J.FieldAccess) field.getTarget()).isFullyQualifiedClassReference(identifierOwnerFqn)) {
return identifier;
}
if (identifier.getFieldType() != null) {
// Similarly, might be comparing a statically imported field to a fully qualified
// e.g. java.util.regex.Pattern.CASE_INSENSITIVE is semantically equal to CASE_INSENSITIVE when the latter is a static import of the former
JavaType identifierOwner = identifier.getFieldType().getOwner();
String identifierOwnerFqn = identifierOwner instanceof JavaType.FullyQualified ? ((JavaType.FullyQualified) identifierOwner).getFullyQualifiedName() : "";
if (field.getTarget() instanceof J.FieldAccess && ((J.FieldAccess) field.getTarget()).isFullyQualifiedClassReference(identifierOwnerFqn)) {
return identifier;
}

// Identifier is statically imported field name "CASE_INSENSITIVE", field is field access "Pattern.CASE_INSENSITIVE"
if (identifierOwner instanceof JavaType.FullyQualified && ((JavaType.FullyQualified) identifierOwner).getClassName().equals(fieldTarget.toString()) && TypeUtils.isOfType(identifier.getFieldType().getOwner(), fieldTarget.getType())) {
return identifier;
// Identifier is statically imported field name "CASE_INSENSITIVE", field is field access "Pattern.CASE_INSENSITIVE"
if (identifierOwner instanceof JavaType.FullyQualified && ((JavaType.FullyQualified) identifierOwner).getClassName().equals(fieldTarget.toString()) && TypeUtils.isOfType(identifier.getFieldType().getOwner(), fieldTarget.getType())) {
return identifier;
}
}

isEqual.set(false);
Expand All @@ -729,6 +736,7 @@ public J.Identifier visitIdentifier(J.Identifier identifier, J j) {

J.Identifier compareTo = (J.Identifier) j;
if (identifier.getFieldType() != null) {
Map<String, String> scope = variableScope.peek();
if (scope != null && scope.containsKey(identifier.getSimpleName()) && scope.get(identifier.getSimpleName()).equals(compareTo.getSimpleName())) {
return identifier;
}
Expand Down
17 changes: 11 additions & 6 deletions rewrite-java/src/main/java/org/openrewrite/java/tree/J.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1979,21 +1979,26 @@ public List<J> getSideEffects() {
}

public boolean isFullyQualifiedClassReference(String className) {
return isFullyQualifiedClassReference(this, className);
if (!className.contains(".")) {
return false;
}
return isFullyQualifiedClassReference(this, className, className.length());
}

private boolean isFullyQualifiedClassReference(J.FieldAccess fieldAccess, String className) {
if (!className.contains(".")) {
private boolean isFullyQualifiedClassReference(J.FieldAccess fieldAccess, String className, int prevDotIndex) {
int dotIndex = className.lastIndexOf('.', prevDotIndex - 1);
if (dotIndex < 0) {
return false;
}
if (!fieldAccess.getName().getSimpleName().equals(className.substring(className.lastIndexOf('.') + 1))) {
String simpleName = fieldAccess.getName().getSimpleName();
if (!simpleName.regionMatches(0, className, dotIndex + 1, simpleName.length())) {
return false;
}
if (fieldAccess.getTarget() instanceof J.FieldAccess) {
return isFullyQualifiedClassReference((J.FieldAccess) fieldAccess.getTarget(), className.substring(0, className.lastIndexOf('.')));
return isFullyQualifiedClassReference((J.FieldAccess) fieldAccess.getTarget(), className, dotIndex);
}
if (fieldAccess.getTarget() instanceof Identifier) {
return ((Identifier) fieldAccess.getTarget()).getSimpleName().equals(className.substring(0, className.lastIndexOf('.')));
return ((Identifier) fieldAccess.getTarget()).getSimpleName().equals(className.substring(0, dotIndex));
}
return false;
}
Expand Down

0 comments on commit e536ed2

Please sign in to comment.