Skip to content

Commit

Permalink
Attempt to improve performance of ReplaceDuplicateStringLiterals so t…
Browse files Browse the repository at this point in the history 
…hat it can be included back into common static analysis by default
  • Loading branch information
@sambsnyd
sambsnyd committed May 10, 2024
1 parent e15d17b commit a7ed18f
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 124 deletions.
209 changes: 91 additions & 118 deletions src/main/java/org/openrewrite/staticanalysis/ReplaceDuplicateStringLiterals.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,9 +29,9 @@
import java.time.Duration;
import java.util.*;

import static java.util.Arrays.asList;
import static java.util.Collections.emptyList;
import static java.util.Collections.singletonList;
import static java.util.Objects.requireNonNull;
import static org.openrewrite.Tree.randomId;

@Value
Expand All @@ -51,12 +51,16 @@ public String getDisplayName() {

@Override
public String getDescription() {
return "Replaces `String` literals with a length of 5 or greater repeated a minimum of 3 times. Qualified `String` literals include final Strings, method invocations, and new class invocations. Adds a new `private static final String` or uses an existing equivalent class field. A new variable name will be generated based on the literal value if an existing field does not exist. The generated name will append a numeric value to the variable name if a name already exists in the compilation unit.";
return "Replaces `String` literals with a length of 5 or greater repeated a minimum of 3 times. " +
"Qualified `String` literals include final Strings, method invocations, and new class invocations. " +
"Adds a new `private static final String` or uses an existing equivalent class field. " +
"A new variable name will be generated based on the literal value if an existing field does not exist. " +
"The generated name will append a numeric value to the variable name if a name already exists in the compilation unit.";
}

@Override
public Set<String> getTags() {
return Collections.singleton("RSPEC-S1192");
return new LinkedHashSet<>(asList("RSPEC-1192", "RSPEC-1889"));
}

@Override
Expand All @@ -70,12 +74,11 @@ public TreeVisitor<?, ExecutionContext> getVisitor() {
@Override
public J visit(@Nullable Tree tree, ExecutionContext ctx) {
if (tree instanceof JavaSourceFile) {
JavaSourceFile cu = (JavaSourceFile) requireNonNull(tree);
JavaSourceFile cu = (JavaSourceFile) tree;
Optional<JavaSourceSet> sourceSet = cu.getMarkers().findFirst(JavaSourceSet.class);
if (Boolean.TRUE.equals(includeTestSources) || (sourceSet.isPresent() && "main".equals(sourceSet.get().getName()))) {
return super.visit(cu, ctx);
if(!Boolean.TRUE.equals(includeTestSources) && !(sourceSet.isPresent() && "main".equals(sourceSet.get().getName()))) {
return cu;
}
return cu;
}
return super.visit(tree, ctx);
}
Expand All @@ -86,14 +89,13 @@ public J visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ct
return classDecl;
}

Map<String, Set<J.Literal>> duplicateLiteralsMap = FindDuplicateStringLiterals.find(classDecl);
DuplicateLiteralInfo duplicateLiteralInfo = DuplicateLiteralInfo.find(classDecl);
Map<String, Set<J.Literal>> duplicateLiteralsMap = duplicateLiteralInfo.getDuplicateLiterals();
if (duplicateLiteralsMap.isEmpty()) {
return classDecl;
}

Set<String> variableNames = FindVariableNames.find(classDecl);
Map<String, String> fieldValueToFieldName = FindExistingPrivateStaticFinalFields.find(classDecl);

Set<String> variableNames = duplicateLiteralInfo.getVariableNames();
Map<String, String> fieldValueToFieldName = duplicateLiteralInfo.getFieldValueToFieldName();
String classFqn = classDecl.getType().getFullyQualifiedName();
for (String valueOfLiteral : duplicateLiteralsMap.keySet()) {
String variableName;
Expand All @@ -111,7 +113,7 @@ public J visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ct
if (StringUtils.isBlank(variableName)) {
continue;
}
J.Literal replaceLiteral = ((J.Literal) duplicateLiteralsMap.get(valueOfLiteral).toArray()[0]).withId(Tree.randomId());
J.Literal replaceLiteral = ((J.Literal) duplicateLiteralsMap.get(valueOfLiteral).toArray()[0]).withId(randomId());
String insertStatement = "private static final String " + variableName + " = #{any(String)};";
if (classDecl.getKind() == J.ClassDeclaration.Kind.Type.Enum) {
J.EnumValueSet enumValueSet = classDecl.getBody().getStatements().stream()
Expand All @@ -121,7 +123,7 @@ public J visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ct
.orElse(null);

if (enumValueSet != null) {
// Temporary work around due to an issue in the JavaTemplate related to BlockStatementTemplateGenerator#enumClassDeclaration.
// "Temporary" work around due to an issue in the JavaTemplate related to BlockStatementTemplateGenerator#enumClassDeclaration.
Space singleSpace = Space.build(" ", emptyList());
Expression literal = duplicateLiteralsMap.get(valueOfLiteral).toArray(new J.Literal[0])[0].withId(randomId());
J.Modifier privateModifier = new J.Modifier(randomId(), Space.build("\n", emptyList()), Markers.EMPTY, null, J.Modifier.Type.Private, emptyList());
Expand Down Expand Up @@ -174,7 +176,7 @@ public J visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ct
}
} else {
classDecl = classDecl.withBody(
JavaTemplate.builder(insertStatement).contextSensitive().build()
JavaTemplate.builder(insertStatement).build()
.apply(new Cursor(getCursor(), classDecl.getBody()), classDecl.getBody().getCoordinates().firstStatement(), replaceLiteral));
}
}
Expand Down Expand Up @@ -230,18 +232,20 @@ private String transformToVariableName(String valueOfLiteral) {
});
}

private static class FindDuplicateStringLiterals extends JavaIsoVisitor<Map<String, Set<J.Literal>>> {
private static boolean isPrivateStaticFinalVariable(J.VariableDeclarations declaration) {
return declaration.hasModifier(J.Modifier.Type.Private) &&
declaration.hasModifier(J.Modifier.Type.Static) &&
declaration.hasModifier(J.Modifier.Type.Final);
}

@Value
private static class DuplicateLiteralInfo {
Set<String> variableNames;
Map<String, String> fieldValueToFieldName;
Map<String, Set<J.Literal>> literalsMap = new HashMap<>();

/**
* Find duplicate `String` literals repeated 3 or more times and with a length of at least 3.
*
* @param inClass subtree to search in.
* @return `Map` of `String` literal values to the `J.Literal` AST elements.
*/
public static Map<String, Set<J.Literal>> find(J.ClassDeclaration inClass) {
Map<String, Set<J.Literal>> literalsMap = new HashMap<>();
public Map<String, Set<J.Literal>> getDuplicateLiterals() {
Map<String, Set<J.Literal>> filteredMap = new TreeMap<>(Comparator.reverseOrder());
new FindDuplicateStringLiterals().visit(inClass, literalsMap);
for (String valueOfLiteral : literalsMap.keySet()) {
if (literalsMap.get(valueOfLiteral).size() >= 3) {
filteredMap.put(valueOfLiteral, literalsMap.get(valueOfLiteral));
Expand All @@ -250,120 +254,89 @@ public static Map<String, Set<J.Literal>> find(J.ClassDeclaration inClass) {
return filteredMap;
}

@Override
public J.Literal visitLiteral(J.Literal literal, Map<String, Set<J.Literal>> literalsMap) {
if (JavaType.Primitive.String.equals(literal.getType()) &&
literal.getValue() instanceof String &&
((String) literal.getValue()).length() >= 5) {
public static DuplicateLiteralInfo find(J.ClassDeclaration inClass) {
DuplicateLiteralInfo result = new DuplicateLiteralInfo(new LinkedHashSet<>(), new LinkedHashMap<>());
new JavaIsoVisitor<Integer>() {

Cursor parent = getCursor().dropParentUntil(is -> is instanceof J.ClassDeclaration ||
is instanceof J.Annotation ||
is instanceof J.VariableDeclarations ||
is instanceof J.NewClass ||
is instanceof J.MethodInvocation);
// EnumValue can accept constructor arguments, including string literals
// But the static field can't be placed before them, so these literals are ineligible for replacement
if (parent.getValue() instanceof J.NewClass && parent.firstEnclosing(J.EnumValueSet.class) != null) {
return literal;
@Override
public J.Annotation visitAnnotation(J.Annotation annotation, Integer integer) {
// Literals in annotations cannot be replaced with variables and should be ignored
return annotation;
}

if ((parent.getValue() instanceof J.VariableDeclarations &&
((J.VariableDeclarations) parent.getValue()).hasModifier(J.Modifier.Type.Final) &&
!(((J.VariableDeclarations) parent.getValue()).hasModifier(J.Modifier.Type.Private) && ((J.VariableDeclarations) parent.getValue()).hasModifier(J.Modifier.Type.Static))) ||
parent.getValue() instanceof J.NewClass ||
parent.getValue() instanceof J.MethodInvocation) {

literalsMap.computeIfAbsent(((String) literal.getValue()), k -> new HashSet<>());
literalsMap.get((String) literal.getValue()).add(literal);
@Override
public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, Integer integer) {
J.VariableDeclarations.NamedVariable v = super.visitVariable(variable, integer);
Cursor parentScope = getCursor().dropParentUntil(is -> is instanceof J.ClassDeclaration || is instanceof J.MethodDeclaration);
J.VariableDeclarations declaration = getCursor().firstEnclosing(J.VariableDeclarations.class);
if (parentScope.getValue() instanceof J.MethodDeclaration ||
(parentScope.getValue() instanceof J.ClassDeclaration && declaration != null &&
// `private static final String`(s) are handled separately by `FindExistingPrivateStaticFinalFields`.
!(isPrivateStaticFinalVariable(declaration) && v.getInitializer() instanceof J.Literal &&
((J.Literal) v.getInitializer()).getValue() instanceof String))) {
result.variableNames.add(v.getSimpleName());
}
if (parentScope.getValue() instanceof J.ClassDeclaration &&
declaration != null && isPrivateStaticFinalVariable(declaration) &&
v.getInitializer() instanceof J.Literal &&
((J.Literal) v.getInitializer()).getValue() instanceof String) {
String value = (String) (((J.Literal) v.getInitializer()).getValue());
result.fieldValueToFieldName.putIfAbsent(value, v.getSimpleName());
}
return v;
}
}
return literal;
}
}

private static boolean isPrivateStaticFinalVariable(J.VariableDeclarations declaration) {
return declaration.hasModifier(J.Modifier.Type.Private) &&
declaration.hasModifier(J.Modifier.Type.Static) &&
declaration.hasModifier(J.Modifier.Type.Final);
}

private static class FindVariableNames extends JavaIsoVisitor<Set<String>> {

/**
* Find all the variable names that exist in the provided subtree.
*
* @param inClass subtree to search in.
* @return variable names that exist in the subtree.
*/
public static Set<String> find(J.ClassDeclaration inClass) {
Set<String> variableNames = new HashSet<>();
new FindVariableNames().visit(inClass, variableNames);
return variableNames;
}

@Override
public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, Set<String> variableNames) {
Cursor parentScope = getCursor().dropParentUntil(is -> is instanceof J.ClassDeclaration || is instanceof J.MethodDeclaration);
J.VariableDeclarations declaration = getCursor().firstEnclosing(J.VariableDeclarations.class);
if (parentScope.getValue() instanceof J.MethodDeclaration ||
(parentScope.getValue() instanceof J.ClassDeclaration && declaration != null &&
// `private static final String`(s) are handled separately by `FindExistingPrivateStaticFinalFields`.
!(isPrivateStaticFinalVariable(declaration) && variable.getInitializer() instanceof J.Literal &&
((J.Literal) variable.getInitializer()).getValue() instanceof String))) {
variableNames.add(variable.getSimpleName());
}
return variable;
}
}
@Override
public J.Literal visitLiteral(J.Literal literal, Integer integer) {
if (JavaType.Primitive.String.equals(literal.getType()) &&
literal.getValue() instanceof String &&
((String) literal.getValue()).length() >= 5) {

Cursor parent = getCursor().dropParentUntil(is -> is instanceof J.ClassDeclaration ||
is instanceof J.Annotation ||
is instanceof J.VariableDeclarations ||
is instanceof J.NewClass ||
is instanceof J.MethodInvocation);
// EnumValue can accept constructor arguments, including string literals
// But the static field can't be placed before them, so these literals are ineligible for replacement
if (parent.getValue() instanceof J.NewClass && parent.firstEnclosing(J.EnumValueSet.class) != null) {
return literal;
}

private static class FindExistingPrivateStaticFinalFields extends JavaIsoVisitor<Map<String, String>> {
if ((parent.getValue() instanceof J.VariableDeclarations &&
((J.VariableDeclarations) parent.getValue()).hasModifier(J.Modifier.Type.Final) &&
!(((J.VariableDeclarations) parent.getValue()).hasModifier(J.Modifier.Type.Private) && ((J.VariableDeclarations) parent.getValue()).hasModifier(J.Modifier.Type.Static))) ||
parent.getValue() instanceof J.NewClass ||
parent.getValue() instanceof J.MethodInvocation) {

/**
* Find existing `private static final String`(s) in a class.
*/
public static Map<String, String> find(J j) {
Map<String, String> fieldValueToFieldName = new LinkedHashMap<>();
new FindExistingPrivateStaticFinalFields().visit(j, fieldValueToFieldName);
return fieldValueToFieldName;
}
result.literalsMap.computeIfAbsent(((String) literal.getValue()), k -> new HashSet<>());
result.literalsMap.get((String) literal.getValue()).add(literal);
}
}
return literal;
}

@Override
public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, Map<String, String> valueToVariable) {
Cursor parentScope = getCursor().dropParentUntil(is -> is instanceof J.ClassDeclaration ||
// Prevent checks on most of the literals.
is instanceof J.MethodDeclaration);
J.VariableDeclarations declaration = getCursor().firstEnclosing(J.VariableDeclarations.class);
if (parentScope.getValue() instanceof J.ClassDeclaration &&
declaration != null && isPrivateStaticFinalVariable(declaration) &&
variable.getInitializer() instanceof J.Literal &&
((J.Literal) variable.getInitializer()).getValue() instanceof String) {
String value = (String) (((J.Literal) variable.getInitializer()).getValue());
valueToVariable.putIfAbsent(value, variable.getSimpleName());
}
return variable;
}.visit(inClass, 0);
return result;
}
}

/**
* ReplaceStringLiterals in a class with a reference to a `private static final String` with the provided variable name.
*/
@Value
@EqualsAndHashCode(callSuper = false)
private static class ReplaceStringLiterals extends JavaVisitor<ExecutionContext> {
private final J.ClassDeclaration isClass;
private final String variableName;
private final Set<J.Literal> literals;

private ReplaceStringLiterals(J.ClassDeclaration isClass, String variableName, Set<J.Literal> literals) {
this.isClass = isClass;
this.variableName = variableName;
this.literals = literals;
}
J.ClassDeclaration isClass;
String variableName;
Set<J.Literal> literals;

@Override
public J visitLiteral(J.Literal literal, ExecutionContext ctx) {
if (literals.contains(literal)) {
assert isClass.getType() != null;
return new J.Identifier(
Tree.randomId(),
randomId(),
literal.getPrefix(),
literal.getMarkers(),
emptyList(),
Expand Down
Loading

0 comments on commit a7ed18f

Please sign in to comment.