Skip to content

Commit

Permalink
Updating ReplaceDuplicateStringLiterals recipe to use VariableNameUti…
Browse files Browse the repository at this point in the history 
…ls to track variable names

VariableNameUtils already provides a list of used names in the scope :).
We need to handle some edges slightly different
1: When the constant already exists, in this case by default this existing constant name wouldn't be used because it's already being used. In this case, we need to allow the recipe to use this variable
2: We want to prevent namespace shadowing on existing constants (preventNamespaceShadowingOnExistingConstant is the associated test). When we solve edge 1, by allowing existing constants to be used, we break this edge case. As such we need to validate that we are assigning a name to the value which makes sense.
  • Loading branch information
@lkerford
lkerford committed Oct 22, 2024
1 parent 619b623 commit 8468b20
Show file tree
Hide file tree
Showing 2 changed files with 79 additions and 9 deletions.
26 changes: 17 additions & 9 deletions src/main/java/org/openrewrite/staticanalysis/ReplaceDuplicateStringLiterals.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@

import java.time.Duration;
import java.util.*;
import java.util.stream.Collectors;

import static java.util.Arrays.asList;
import static java.util.Collections.emptyList;
Expand Down Expand Up @@ -97,8 +98,9 @@ public J visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ct
if (duplicateLiteralsMap.isEmpty()) {
return classDecl;
}
Set<String> variableNames = duplicateLiteralInfo.getVariableNames();
Map<String, String> fieldValueToFieldName = duplicateLiteralInfo.getFieldValueToFieldName();
Set<String> variableNames = VariableNameUtils.findNamesInScope(getCursor()).stream()
.filter(i -> !fieldValueToFieldName.containsValue(i)).collect(Collectors.toSet());
String classFqn = classDecl.getType().getFullyQualifiedName();
Map<J.Literal, String> replacements = new HashMap<>();
for (Map.Entry<String, List<J.Literal>> entry : duplicateLiteralsMap.entrySet()) {
Expand All @@ -107,7 +109,13 @@ public J visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ct
String classFieldName = fieldValueToFieldName.get(valueOfLiteral);
String variableName;
if (classFieldName != null) {
variableName = getNameWithoutShadow(classFieldName, variableNames);
String maybeVariableName = getNameWithoutShadow(classFieldName, variableNames);
if (duplicateLiteralInfo.existingFieldValueToFieldName.get(maybeVariableName) != null) {
variableNames.add(maybeVariableName);
maybeVariableName = getNameWithoutShadow(classFieldName, variableNames);
}

variableName = maybeVariableName;
if (StringUtils.isBlank(variableName)) {
continue;
}
Expand Down Expand Up @@ -199,14 +207,14 @@ private static boolean isPrivateStaticFinalVariable(J.VariableDeclarations.Named

@Value
private static class DuplicateLiteralInfo {
Set<String> variableNames;
Map<String, String> fieldValueToFieldName;
Map<String, String> existingFieldValueToFieldName;

@NonFinal
Map<String, List<J.Literal>> duplicateLiterals;

public static DuplicateLiteralInfo find(J.ClassDeclaration inClass) {
DuplicateLiteralInfo result = new DuplicateLiteralInfo(new LinkedHashSet<>(), new LinkedHashMap<>(), new HashMap<>());
DuplicateLiteralInfo result = new DuplicateLiteralInfo(new LinkedHashMap<>(), new LinkedHashMap<>(), new HashMap<>());
new JavaIsoVisitor<Integer>() {

@Override
Expand All @@ -221,11 +229,11 @@ public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations
Cursor parentScope = getCursor().dropParentUntil(is -> is instanceof J.ClassDeclaration || is instanceof J.MethodDeclaration);
boolean privateStaticFinalVariable = isPrivateStaticFinalVariable(variable);
// `private static final String`(s) are handled separately by `FindExistingPrivateStaticFinalFields`.
if (parentScope.getValue() instanceof J.MethodDeclaration ||
parentScope.getValue() instanceof J.ClassDeclaration &&
!(privateStaticFinalVariable && v.getInitializer() instanceof J.Literal &&
((J.Literal) v.getInitializer()).getValue() instanceof String)) {
result.variableNames.add(v.getSimpleName());
if (v.getInitializer() instanceof J.Literal &&
(parentScope.getValue() instanceof J.MethodDeclaration || parentScope.getValue() instanceof J.ClassDeclaration) &&
!(privateStaticFinalVariable && ((J.Literal) v.getInitializer()).getValue() instanceof String)) {
String value = (((J.Literal) v.getInitializer()).getValue()).toString();
result.existingFieldValueToFieldName.put(v.getSimpleName(), value);
}
if (parentScope.getValue() instanceof J.ClassDeclaration &&
privateStaticFinalVariable && v.getInitializer() instanceof J.Literal &&
Expand Down
62 changes: 62 additions & 0 deletions src/test/java/org/openrewrite/staticanalysis/ReplaceDuplicateStringLiteralsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,68 @@ class A {
);
}

@Test
void fieldNameCollidesWithNewStringLiteral() {
rewriteRun(
//language=java
java(
"""
package org.foo;
class A {
final String val1 = "value";
final String val2 = "value";
final String val3 = "value";
final int VALUE = 1;
}
""",
"""
package org.foo;
class A {
private static final String VALUE_1 = "value";
final String val1 = VALUE_1;
final String val2 = VALUE_1;
final String val3 = VALUE_1;
final int VALUE = 1;
}
"""
)
);
}

@Test
void staticImportCollidesWithNewStringLiteral() {
rewriteRun(
//language=java
java(
"""
package org.foo;
import static java.lang.Long.MAX_VALUE;
class A {
final String val1 = "max_value";
final String val2 = "max_value";
final String val3 = "max_value";
final long value = MAX_VALUE;
}
""",
"""
package org.foo;
import static java.lang.Long.MAX_VALUE;
class A {
private static final String MAX_VALUE_1 = "max_value";
final String val1 = MAX_VALUE_1;
final String val2 = MAX_VALUE_1;
final String val3 = MAX_VALUE_1;
final long value = MAX_VALUE;
}
"""
)
);
}

@Test
void generatedNameIsVeryLong() {
rewriteRun(
Expand Down

0 comments on commit 8468b20

Please sign in to comment.