From 8468b209c9bc7d76b1a419930c2f12d5372a9e27 Mon Sep 17 00:00:00 2001 From: lkerford Date: Tue, 22 Oct 2024 15:00:12 -0700 Subject: [PATCH] Updating ReplaceDuplicateStringLiterals recipe to use VariableNameUtils 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. --- .../ReplaceDuplicateStringLiterals.java | 26 +++++--- .../ReplaceDuplicateStringLiteralsTest.java | 62 +++++++++++++++++++ 2 files changed, 79 insertions(+), 9 deletions(-) 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 variableNames = duplicateLiteralInfo.getVariableNames(); Map fieldValueToFieldName = duplicateLiteralInfo.getFieldValueToFieldName(); + Set variableNames = VariableNameUtils.findNamesInScope(getCursor()).stream() + .filter(i -> !fieldValueToFieldName.containsValue(i)).collect(Collectors.toSet()); String classFqn = classDecl.getType().getFullyQualifiedName(); Map replacements = new HashMap<>(); for (Map.Entry> 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 variableNames; Map fieldValueToFieldName; + Map existingFieldValueToFieldName; @NonFinal Map> 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() { @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(