diff --git a/src/main/java/org/openrewrite/staticanalysis/ReplaceDuplicateStringLiterals.java b/src/main/java/org/openrewrite/staticanalysis/ReplaceDuplicateStringLiterals.java index 251dea221..e4f5e371d 100644 --- a/src/main/java/org/openrewrite/staticanalysis/ReplaceDuplicateStringLiterals.java +++ b/src/main/java/org/openrewrite/staticanalysis/ReplaceDuplicateStringLiterals.java @@ -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; @@ -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()) { @@ -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; } @@ -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 @@ -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 && diff --git a/src/test/java/org/openrewrite/staticanalysis/ReplaceDuplicateStringLiteralsTest.java b/src/test/java/org/openrewrite/staticanalysis/ReplaceDuplicateStringLiteralsTest.java index cec186f5f..f8e3e6958 100644 --- a/src/test/java/org/openrewrite/staticanalysis/ReplaceDuplicateStringLiteralsTest.java +++ b/src/test/java/org/openrewrite/staticanalysis/ReplaceDuplicateStringLiteralsTest.java @@ -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(