From d062ebf1fa00fdbc97b20e442dba7d8f2215d58f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Tue, 14 Oct 2025 15:06:28 +0200 Subject: [PATCH 01/10] basic recipe to find thread local variables that are never mutated --- .../FindImmutableThreadLocalVariables.java | 204 ++++++++++++ ...FindImmutableThreadLocalVariablesTest.java | 295 ++++++++++++++++++ 2 files changed, 499 insertions(+) create mode 100644 src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java create mode 100644 src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java b/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java new file mode 100644 index 0000000000..0ce9fa690d --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java @@ -0,0 +1,204 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.search; + +import lombok.EqualsAndHashCode; +import lombok.Value; +import org.openrewrite.ExecutionContext; +import org.openrewrite.ScanningRecipe; +import org.openrewrite.TreeVisitor; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaType; +import org.openrewrite.java.tree.TypeUtils; +import org.openrewrite.marker.SearchResult; + +import java.util.HashSet; +import java.util.Set; + +@EqualsAndHashCode(callSuper = false) +public class FindImmutableThreadLocalVariables extends ScanningRecipe { + + private static final MethodMatcher THREAD_LOCAL_SET = new MethodMatcher("java.lang.ThreadLocal set(..)"); + private static final MethodMatcher THREAD_LOCAL_REMOVE = new MethodMatcher("java.lang.ThreadLocal remove()"); + private static final MethodMatcher INHERITABLE_THREAD_LOCAL_SET = new MethodMatcher("java.lang.InheritableThreadLocal set(..)"); + private static final MethodMatcher INHERITABLE_THREAD_LOCAL_REMOVE = new MethodMatcher("java.lang.InheritableThreadLocal remove()"); + + @Override + public String getDisplayName() { + return "Find immutable ThreadLocal variables"; + } + + @Override + public String getDescription() { + return "Find `ThreadLocal` variables that are never mutated and could be candidates for migration to `ScopedValue` in Java 25+. " + + "This recipe identifies `ThreadLocal` variables that are only initialized but never reassigned or modified through `set()` or `remove()` methods."; + } + + @Value + static class ThreadLocalAccumulator { + Set allThreadLocals = new HashSet<>(); + Set mutatedThreadLocals = new HashSet<>(); + + public boolean isImmutable(ThreadLocalVariable var) { + return allThreadLocals.contains(var) && !mutatedThreadLocals.contains(var); + } + } + + @Value + static class ThreadLocalVariable { + String name; + String className; + + static ThreadLocalVariable fromVariable(J.VariableDeclarations.NamedVariable variable, J.ClassDeclaration classDecl) { + String className = classDecl != null && classDecl.getType() != null ? + classDecl.getType().getFullyQualifiedName() : ""; + return new ThreadLocalVariable(variable.getName().getSimpleName(), className); + } + } + + @Override + public ThreadLocalAccumulator getInitialValue(ExecutionContext ctx) { + return new ThreadLocalAccumulator(); + } + + @Override + public TreeVisitor getScanner(ThreadLocalAccumulator acc) { + return new JavaIsoVisitor() { + @Override + public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, ExecutionContext ctx) { + variable = super.visitVariable(variable, ctx); + + if (isThreadLocalType(variable.getType())) { + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + ThreadLocalVariable tlVar = ThreadLocalVariable.fromVariable(variable, classDecl); + acc.allThreadLocals.add(tlVar); + + // Check if this is a local variable - we don't mark local variables + J.Block enclosingBlock = getCursor().firstEnclosing(J.Block.class); + J.MethodDeclaration enclosingMethod = getCursor().firstEnclosing(J.MethodDeclaration.class); + if (enclosingMethod != null && enclosingBlock != null && enclosingBlock != enclosingMethod.getBody()) { + // This is a local variable inside a method + acc.mutatedThreadLocals.add(tlVar); + } + } + return variable; + } + + @Override + public J.Assignment visitAssignment(J.Assignment assignment, ExecutionContext ctx) { + assignment = super.visitAssignment(assignment, ctx); + + // Check if we're reassigning a ThreadLocal variable + if (assignment.getVariable() instanceof J.Identifier) { + J.Identifier id = (J.Identifier) assignment.getVariable(); + if (isThreadLocalType(id.getType())) { + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + String className = classDecl != null && classDecl.getType() != null ? + classDecl.getType().getFullyQualifiedName() : ""; + acc.mutatedThreadLocals.add(new ThreadLocalVariable(id.getSimpleName(), className)); + } + } + return assignment; + } + + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { + method = super.visitMethodInvocation(method, ctx); + + // Check for ThreadLocal.set() or ThreadLocal.remove() calls + if (THREAD_LOCAL_SET.matches(method) || THREAD_LOCAL_REMOVE.matches(method) || + INHERITABLE_THREAD_LOCAL_SET.matches(method) || INHERITABLE_THREAD_LOCAL_REMOVE.matches(method)) { + + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + String className = classDecl != null && classDecl.getType() != null ? + classDecl.getType().getFullyQualifiedName() : ""; + + // Get the ThreadLocal instance being mutated + String varName = null; + if (method.getSelect() instanceof J.Identifier) { + varName = ((J.Identifier) method.getSelect()).getSimpleName(); + } else if (method.getSelect() instanceof J.FieldAccess) { + varName = ((J.FieldAccess) method.getSelect()).getSimpleName(); + } + + if (varName != null) { + acc.mutatedThreadLocals.add(new ThreadLocalVariable(varName, className)); + } + } + return method; + } + + @Override + public J.Unary visitUnary(J.Unary unary, ExecutionContext ctx) { + unary = super.visitUnary(unary, ctx); + + // Check for operations that would mutate the ThreadLocal reference (unlikely but thorough) + if (unary.getExpression() instanceof J.Identifier) { + J.Identifier id = (J.Identifier) unary.getExpression(); + if (isThreadLocalType(id.getType())) { + switch (unary.getOperator()) { + case PreIncrement: + case PreDecrement: + case PostIncrement: + case PostDecrement: + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + String className = classDecl != null && classDecl.getType() != null ? + classDecl.getType().getFullyQualifiedName() : ""; + acc.mutatedThreadLocals.add(new ThreadLocalVariable(id.getSimpleName(), className)); + break; + default: + break; + } + } + } + return unary; + } + }; + } + + @Override + public TreeVisitor getVisitor(ThreadLocalAccumulator acc) { + return new JavaIsoVisitor() { + @Override + public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { + multiVariable = super.visitVariableDeclarations(multiVariable, ctx); + + // Check if any of the variables in this declaration are immutable ThreadLocals + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + for (J.VariableDeclarations.NamedVariable variable : multiVariable.getVariables()) { + if (isThreadLocalType(variable.getType())) { + ThreadLocalVariable tlVar = ThreadLocalVariable.fromVariable(variable, classDecl); + if (acc.isImmutable(tlVar)) { + return SearchResult.found( + multiVariable, + "ThreadLocal candidate for ScopedValue migration - never mutated after initialization" + ); + } + } + } + + return multiVariable; + } + }; + } + + private static boolean isThreadLocalType(JavaType type) { + return TypeUtils.isOfClassType(type, "java.lang.ThreadLocal") || + TypeUtils.isOfClassType(type, "java.lang.InheritableThreadLocal"); + } +} \ No newline at end of file diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java b/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java new file mode 100644 index 0000000000..b2191c1ec4 --- /dev/null +++ b/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java @@ -0,0 +1,295 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.search; + +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; + +class FindImmutableThreadLocalVariablesTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new FindImmutableThreadLocalVariables()); + } + + @DocumentExample + @Test + void identifySimpleImmutableThreadLocal() { + rewriteRun( + java( + """ + class Example { + private static final ThreadLocal TL = new ThreadLocal<>(); + + public String getValue() { + return TL.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal TL = new ThreadLocal<>(); + + public String getValue() { + return TL.get(); + } + } + """ + ) + ); + } + + @Test + void identifyThreadLocalWithInitialValue() { + rewriteRun( + java( + """ + class Example { + private static final ThreadLocal COUNTER = ThreadLocal.withInitial(() -> 0); + + public int getCount() { + return COUNTER.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal COUNTER = ThreadLocal.withInitial(() -> 0); + + public int getCount() { + return COUNTER.get(); + } + } + """ + ) + ); + } + + @Test + void doNotMarkThreadLocalWithSetCall() { + rewriteRun( + java( + """ + class Example { + private static final ThreadLocal TL = new ThreadLocal<>(); + + public void setValue(String value) { + TL.set(value); + } + + public String getValue() { + return TL.get(); + } + } + """ + ) + ); + } + + @Test + void doNotMarkThreadLocalWithRemoveCall() { + rewriteRun( + java( + """ + class Example { + private static final ThreadLocal TL = new ThreadLocal<>(); + + public void cleanup() { + TL.remove(); + } + + public String getValue() { + return TL.get(); + } + } + """ + ) + ); + } + + @Test + void doNotMarkReassignedThreadLocal() { + rewriteRun( + java( + """ + class Example { + private static ThreadLocal tl = new ThreadLocal<>(); + + public void reset() { + tl = new ThreadLocal<>(); + } + + public String getValue() { + return tl.get(); + } + } + """ + ) + ); + } + + @Test + void handleMultipleThreadLocalsWithMixedPatterns() { + rewriteRun( + java( + """ + class Example { + private static final ThreadLocal IMMUTABLE_TL = new ThreadLocal<>(); + private static final ThreadLocal MUTABLE_TL = new ThreadLocal<>(); + private static final ThreadLocal ANOTHER_IMMUTABLE = ThreadLocal.withInitial(() -> false); + + public void updateMutable(int value) { + MUTABLE_TL.set(value); + } + + public String getImmutable() { + return IMMUTABLE_TL.get(); + } + + public Boolean getAnother() { + return ANOTHER_IMMUTABLE.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal IMMUTABLE_TL = new ThreadLocal<>(); + private static final ThreadLocal MUTABLE_TL = new ThreadLocal<>(); + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal ANOTHER_IMMUTABLE = ThreadLocal.withInitial(() -> false); + + public void updateMutable(int value) { + MUTABLE_TL.set(value); + } + + public String getImmutable() { + return IMMUTABLE_TL.get(); + } + + public Boolean getAnother() { + return ANOTHER_IMMUTABLE.get(); + } + } + """ + ) + ); + } + + @Test + void identifyInstanceThreadLocal() { + rewriteRun( + java( + """ + class Example { + private final ThreadLocal instanceTL = new ThreadLocal<>(); + + public String getValue() { + return instanceTL.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private final ThreadLocal instanceTL = new ThreadLocal<>(); + + public String getValue() { + return instanceTL.get(); + } + } + """ + ) + ); + } + + @Test + void handleThreadLocalWithComplexInitialization() { + rewriteRun( + java( + """ + import java.text.SimpleDateFormat; + + class Example { + private static final ThreadLocal DATE_FORMAT = + ThreadLocal.withInitial(() -> new SimpleDateFormat("yyyy-MM-dd")); + + public String formatDate(java.util.Date date) { + return DATE_FORMAT.get().format(date); + } + } + """, + """ + import java.text.SimpleDateFormat; + + class Example { + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal DATE_FORMAT = + ThreadLocal.withInitial(() -> new SimpleDateFormat("yyyy-MM-dd")); + + public String formatDate(java.util.Date date) { + return DATE_FORMAT.get().format(date); + } + } + """ + ) + ); + } + + @Test + void doNotMarkLocalVariableThreadLocal() { + // Local ThreadLocals are unusual but should not be marked as they have different lifecycle + rewriteRun( + java( + """ + class Example { + public void method() { + ThreadLocal localTL = new ThreadLocal<>(); + localTL.set("value"); + System.out.println(localTL.get()); + } + } + """ + ) + ); + } + + @Test + void handleInheritableThreadLocal() { + rewriteRun( + java( + """ + class Example { + private static final InheritableThreadLocal ITL = new InheritableThreadLocal<>(); + + public String getValue() { + return ITL.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final InheritableThreadLocal ITL = new InheritableThreadLocal<>(); + + public String getValue() { + return ITL.get(); + } + } + """ + ) + ); + } +} \ No newline at end of file From 9120a12c76aefd40166cf443cbbdf832bf0a9d7f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Tue, 14 Oct 2025 15:10:54 +0200 Subject: [PATCH 02/10] Acknowledge that we only check file internal ATM --- .../ConvertImmutableClassToRecord.java | 288 ++++++ .../ConvertImmutableClassToRecordTest.java | 821 ++++++++++++++++++ 2 files changed, 1109 insertions(+) create mode 100644 src/main/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecord.java create mode 100644 src/test/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecordTest.java diff --git a/src/main/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecord.java b/src/main/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecord.java new file mode 100644 index 0000000000..fefe222a22 --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecord.java @@ -0,0 +1,288 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate; + +import lombok.EqualsAndHashCode; +import lombok.Value; +import org.jspecify.annotations.Nullable; +import org.openrewrite.ExecutionContext; +import org.openrewrite.Option; +import org.openrewrite.Recipe; +import org.openrewrite.TreeVisitor; +import org.openrewrite.java.ChangeMethodName; +import org.openrewrite.java.JavaTemplate; +import org.openrewrite.java.JavaVisitor; +import org.openrewrite.java.tree.J; + +import java.util.*; +import java.util.stream.Collectors; + +@Value +@EqualsAndHashCode(callSuper = false) +public class ConvertImmutableClassToRecord extends Recipe { + + @Option(displayName = "Package pattern", + description = "A glob pattern to match packages where classes should be converted to records. " + + "If not specified, the recipe will be applied to all packages.", + example = "com.example.**", + required = false) + @Nullable + String packagePattern; + + @Override + public String getDisplayName() { + return "Convert immutable class to record"; + } + + @Override + public String getDescription() { + //language=Markdown + return "Converts immutable classes to Java records. This is a composite recipe that:\n" + + " 1. Identifies classes that meet record conversion criteria\n" + + " 2. Converts eligible classes to records\n" + + " 3. Updates all getter method calls to use record accessor syntax\n" + + "\n" + + " A class is eligible for conversion if it:\n" + + " - Has only private fields\n" + + " - Has corresponding getter methods for all fields\n" + + " - Has no setter methods\n" + + " - Does not extend another class\n" + + " - Is effectively immutable"; + } + + @Override + public TreeVisitor getVisitor() { + return new ConvertImmutableClassToRecordVisitor(); + } + + private class ConvertImmutableClassToRecordVisitor extends JavaVisitor { + + @Override + public J visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ctx) { + if (!isEligibleForRecordConversion(classDecl) || !matchesPackagePattern(classDecl)) { + return classDecl; + } + + // Get all getter methods that will need to be updated + List getterMethods = getGetterMethods(classDecl); + String classTypeFqn = getFullyQualifiedName(classDecl); + + // Schedule method name changes for all getters after this transformation + for (J.MethodDeclaration getter : getterMethods) { + String fieldName = getFieldNameFromGetter(getter); + if (fieldName != null) { + String oldMethodPattern = classTypeFqn + " " + getter.getSimpleName() + "()"; + doAfterVisit(new ChangeMethodName(oldMethodPattern, fieldName, true, null).getVisitor()); + } + } + + // Transform the class to a record + return transformToRecord(classDecl); + } + + private J.ClassDeclaration transformToRecord(J.ClassDeclaration classDecl) { + List fields = getPrivateFields(classDecl); + List utilityMethods = extractUtilityMethods(classDecl); + + // Create record components string + StringBuilder recordComponents = new StringBuilder(); + for (int i = 0; i < fields.size(); i++) { + J.VariableDeclarations field = fields.get(i); + for (J.VariableDeclarations.NamedVariable var : field.getVariables()) { + if (recordComponents.length() > 0) { + recordComponents.append(", "); + } + + // Include field annotations + StringBuilder fieldComponent = new StringBuilder(); + if (!field.getLeadingAnnotations().isEmpty()) { + for (J.Annotation annotation : field.getLeadingAnnotations()) { + fieldComponent.append("@").append(annotation.getAnnotationType().toString()).append(" "); + } + } + fieldComponent.append(field.getTypeExpression()).append(" ").append(var.getSimpleName()); + recordComponents.append(fieldComponent); + } + } + + // Build record template + StringBuilder recordTemplate = new StringBuilder(); + + // Add class-level annotations and modifiers + if (!classDecl.getLeadingAnnotations().isEmpty() || !classDecl.getModifiers().isEmpty()) { + for (J.Annotation annotation : classDecl.getLeadingAnnotations()) { + recordTemplate.append("@").append(annotation.getAnnotationType().toString()).append(" "); + } + for (J.Modifier modifier : classDecl.getModifiers()) { + if (modifier.getType() != J.Modifier.Type.Final) { // Records are implicitly final + recordTemplate.append(modifier.getType().toString().toLowerCase()).append(" "); + } + } + } + + recordTemplate.append("record ").append(classDecl.getSimpleName()).append("(") + .append(recordComponents).append(")"); + + if (!classDecl.getImplements().isEmpty()) { + recordTemplate.append(" implements "); + for (int i = 0; i < classDecl.getImplements().size(); i++) { + if (i > 0) recordTemplate.append(", "); + recordTemplate.append("#{any()}"); + } + } + + recordTemplate.append(" { "); + for (int i = 0; i < utilityMethods.size(); i++) { + recordTemplate.append("#{any()} "); + } + recordTemplate.append("}"); + + // Prepare template parameters + List templateParams = new ArrayList<>(); + if (!classDecl.getImplements().isEmpty()) { + for (J j : classDecl.getImplements()) { + templateParams.add(j); + } + } + for (J.MethodDeclaration method : utilityMethods) { + templateParams.add(method); + } + + JavaTemplate template = JavaTemplate.builder(recordTemplate.toString()).build(); + return template.apply(updateCursor(classDecl), classDecl.getCoordinates().replace(), templateParams.toArray()); + } + + private boolean isEligibleForRecordConversion(J.ClassDeclaration classDecl) { + // Skip if class extends another class (records can't extend classes) + if (classDecl.getExtends() != null) { + return false; + } + + // Skip if class is not a regular class + if (classDecl.getKind() != J.ClassDeclaration.Kind.Type.Class) { + return false; + } + + List fields = getPrivateFields(classDecl); + if (fields.isEmpty()) { + return false; + } + + // Check if all fields have corresponding getters + Set fieldNames = fields.stream() + .flatMap(field -> field.getVariables().stream()) + .map(J.VariableDeclarations.NamedVariable::getSimpleName) + .collect(Collectors.toSet()); + + Set getterFields = getGetterMethods(classDecl).stream() + .map(this::getFieldNameFromGetter) + .filter(Objects::nonNull) + .collect(Collectors.toSet()); + + return fieldNames.equals(getterFields) && !hasSetterMethods(classDecl); + } + + private List extractUtilityMethods(J.ClassDeclaration classDecl) { + return classDecl.getBody().getStatements().stream() + .filter(J.MethodDeclaration.class::isInstance) + .map(J.MethodDeclaration.class::cast) + .filter(method -> !isConstructor(method) && !isGetterMethod(method) && !isSetterMethod(method)) + .collect(Collectors.toList()); + } + + private List getPrivateFields(J.ClassDeclaration classDecl) { + return classDecl.getBody().getStatements().stream() + .filter(J.VariableDeclarations.class::isInstance) + .map(J.VariableDeclarations.class::cast) + .filter(field -> field.hasModifier(J.Modifier.Type.Private)) + .collect(Collectors.toList()); + } + + private List getGetterMethods(J.ClassDeclaration classDecl) { + return classDecl.getBody().getStatements().stream() + .filter(J.MethodDeclaration.class::isInstance) + .map(J.MethodDeclaration.class::cast) + .filter(this::isGetterMethod) + .collect(Collectors.toList()); + } + + private boolean hasSetterMethods(J.ClassDeclaration classDecl) { + return classDecl.getBody().getStatements().stream() + .filter(J.MethodDeclaration.class::isInstance) + .map(J.MethodDeclaration.class::cast) + .anyMatch(this::isSetterMethod); + } + + private boolean isConstructor(J.MethodDeclaration method) { + return method.isConstructor(); + } + + private boolean isGetterMethod(J.MethodDeclaration method) { + String methodName = method.getSimpleName(); + return (methodName.startsWith("get") || methodName.startsWith("is")) + && method.getParameters().isEmpty() + && method.getReturnTypeExpression() != null; + } + + private boolean isSetterMethod(J.MethodDeclaration method) { + String methodName = method.getSimpleName(); + return methodName.startsWith("set") + && method.getParameters().size() == 1; + } + + @Nullable + private String getFieldNameFromGetter(J.MethodDeclaration getter) { + String methodName = getter.getSimpleName(); + if (methodName.startsWith("get") && methodName.length() > 3) { + return Character.toLowerCase(methodName.charAt(3)) + methodName.substring(4); + } else if (methodName.startsWith("is") && methodName.length() > 2) { + return Character.toLowerCase(methodName.charAt(2)) + methodName.substring(3); + } + return null; + } + + private boolean matchesPackagePattern(J.ClassDeclaration classDecl) { + if (packagePattern == null || packagePattern.trim().isEmpty()) { + return true; // No pattern specified, match all + } + + String packageName = getPackageName(classDecl); + if (packageName == null) { + return packagePattern.equals("*"); // Default package + } + + // Simple glob pattern matching + String pattern = packagePattern.replace("**", ".*").replace("*", "[^.]*"); + return packageName.matches(pattern); + } + + private String getPackageName(J.ClassDeclaration classDecl) { + J.CompilationUnit cu = getCursor().firstEnclosing(J.CompilationUnit.class); + if (cu != null && cu.getPackageDeclaration() != null) { + return cu.getPackageDeclaration().getExpression().toString(); + } + return null; + } + + private String getFullyQualifiedName(J.ClassDeclaration classDecl) { + String packageName = getPackageName(classDecl); + if (packageName != null) { + return packageName + "." + classDecl.getSimpleName(); + } + return classDecl.getSimpleName(); + } + } +} diff --git a/src/test/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecordTest.java b/src/test/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecordTest.java new file mode 100644 index 0000000000..169bc5bc4e --- /dev/null +++ b/src/test/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecordTest.java @@ -0,0 +1,821 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate; + +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; + +class ConvertImmutableClassToRecordTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new ConvertImmutableClassToRecord(null)); + } + + @DocumentExample + @Test + void simpleImmutableClass() { + rewriteRun( + //language=java + java( + """ + class Person { + private final String name; + private final int age; + + public Person(String name, int age) { + this.name = name; + this.age = age; + } + + public String getName() { + return name; + } + + public int getAge() { + return age; + } + } + """, + """ + record Person(String name, int age) { + } + """ + ) + ); + } + + @Test + void completeClassToRecordTransformationWithMethodCallUpdates() { + rewriteRun( + //language=java + java( + """ + class Person { + private final String name; + private final int age; + + public Person(String name, int age) { + this.name = name; + this.age = age; + } + + public String getName() { + return name; + } + + public int getAge() { + return age; + } + + public String getDisplayName() { + return name + " (" + age + ")"; + } + } + """, + """ + record Person(String name, int age) { + public String getDisplayName() { + return name + " (" + age + ")"; + } + } + """ + ), + //language=java + java( + """ + class PersonService { + public void processPerson(Person person) { + String name = person.getName(); + int age = person.getAge(); + String display = person.getDisplayName(); + System.out.println("Processing: " + display); + } + } + """, + """ + class PersonService { + public void processPerson(Person person) { + String name = person.name(); + int age = person.age(); + String display = person.getDisplayName(); + System.out.println("Processing: " + display); + } + } + """ + ) + ); + } + + @Test + void packagePatternRestriction() { + rewriteRun( + spec -> spec.recipe(new ConvertImmutableClassToRecord("com.example.model.**")), + //language=java + java( + """ + package com.example.model; + + class Person { + private final String name; + + public Person(String name) { + this.name = name; + } + + public String getName() { + return name; + } + } + """, + """ + package com.example.model; + + record Person(String name) { + } + """ + ), + //language=java + java( + """ + package com.example.service; + + class User { + private final String username; + + public User(String username) { + this.username = username; + } + + public String getUsername() { + return username; + } + } + """ + // Should not be transformed due to package pattern restriction + ) + ); + } + + @Test + void multipleClassesTransformation() { + rewriteRun( + //language=java + java( + """ + class Address { + private final String street; + private final String city; + + public Address(String street, String city) { + this.street = street; + this.city = city; + } + + public String getStreet() { + return street; + } + + public String getCity() { + return city; + } + } + + class Person { + private final String name; + private final Address address; + + public Person(String name, Address address) { + this.name = name; + this.address = address; + } + + public String getName() { + return name; + } + + public Address getAddress() { + return address; + } + } + """, + """ + record Address(String street, String city) { + } + + record Person(String name, Address address) { + } + """ + ), + //language=java + java( + """ + class AddressService { + public void printFullAddress(Person person) { + Address addr = person.getAddress(); + String street = addr.getStreet(); + String city = addr.getCity(); + System.out.println(street + ", " + city); + } + } + """, + """ + class AddressService { + public void printFullAddress(Person person) { + Address addr = person.address(); + String street = addr.street(); + String city = addr.city(); + System.out.println(street + ", " + city); + } + } + """ + ) + ); + } + + @Test + void complexBusinessLogicPreservation() { + rewriteRun( + //language=java + java( + """ + import java.util.Objects; + + class Product { + private final String id; + private final String name; + private final double price; + + public Product(String id, String name, double price) { + this.id = id; + this.name = name; + this.price = price; + } + + public String getId() { + return id; + } + + public String getName() { + return name; + } + + public double getPrice() { + return price; + } + + public String getFormattedPrice() { + return String.format("$%.2f", price); + } + + public boolean isExpensive() { + return price > 100.0; + } + + public Product withDiscount(double discountPercent) { + double newPrice = price * (1 - discountPercent / 100); + return new Product(id, name, newPrice); + } + } + """, + """ + import java.util.Objects; + + record Product(String id, String name, double price) { + public String getFormattedPrice() { + return String.format("$%.2f", price); + } + + public boolean isExpensive() { + return price > 100.0; + } + + public Product withDiscount(double discountPercent) { + double newPrice = price * (1 - discountPercent / 100); + return new Product(id, name, newPrice); + } + } + """ + ) + ); + } + + @Test + void nestedClassesHandling() { + rewriteRun( + //language=java + java( + """ + class OuterClass { + private String value; + + static class InnerData { + private final String data; + private final int count; + + public InnerData(String data, int count) { + this.data = data; + this.count = count; + } + + public String getData() { + return data; + } + + public int getCount() { + return count; + } + } + + public void process(InnerData inner) { + String data = inner.getData(); + int count = inner.getCount(); + System.out.println(data + ": " + count); + } + } + """, + """ + class OuterClass { + private String value; + + static record InnerData(String data, int count) { + } + + public void process(InnerData inner) { + String data = inner.data(); + int count = inner.count(); + System.out.println(data + ": " + count); + } + } + """ + ) + ); + } + + @Test + void genericTypesSupport() { + rewriteRun( + //language=java + java( + """ + import java.util.List; + import java.util.Optional; + + class Container { + private final T value; + private final List items; + + public Container(T value, List items) { + this.value = value; + this.items = items; + } + + public T getValue() { + return value; + } + + public List getItems() { + return items; + } + + public Optional findFirst() { + return items.isEmpty() ? Optional.empty() : Optional.of(items.get(0)); + } + } + """, + """ + import java.util.List; + import java.util.Optional; + + record Container(T value, List items) { + public Optional findFirst() { + return items.isEmpty() ? Optional.empty() : Optional.of(items.get(0)); + } + } + """ + ) + ); + } + + @Test + void integrationWithStreamOperations() { + rewriteRun( + //language=java + java( + """ + import java.util.List; + import java.util.stream.Collectors; + + class Employee { + private final String name; + private final String department; + private final double salary; + + public Employee(String name, String department, double salary) { + this.name = name; + this.department = department; + this.salary = salary; + } + + public String getName() { + return name; + } + + public String getDepartment() { + return department; + } + + public double getSalary() { + return salary; + } + } + + class EmployeeService { + public List getNamesByDepartment(List employees, String dept) { + return employees.stream() + .filter(emp -> emp.getDepartment().equals(dept)) + .map(Employee::getName) + .collect(Collectors.toList()); + } + + public double getTotalSalary(List employees) { + return employees.stream() + .mapToDouble(Employee::getSalary) + .sum(); + } + } + """, + """ + import java.util.List; + import java.util.stream.Collectors; + + record Employee(String name, String department, double salary) { + } + + class EmployeeService { + public List getNamesByDepartment(List employees, String dept) { + return employees.stream() + .filter(emp -> emp.department().equals(dept)) + .map(Employee::name) + .collect(Collectors.toList()); + } + + public double getTotalSalary(List employees) { + return employees.stream() + .mapToDouble(Employee::salary) + .sum(); + } + } + """ + ) + ); + } + + @Test + void immutableClassWithAnnotations() { + rewriteRun( + //language=java + java( + """ + import javax.validation.constraints.NotNull; + import javax.validation.constraints.Min; + + @Entity + public class Person { + @NotNull + private final String name; + + @Min(0) + private final int age; + + public Person(String name, int age) { + this.name = name; + this.age = age; + } + + public String getName() { + return name; + } + + public int getAge() { + return age; + } + } + """, + """ + import javax.validation.constraints.NotNull; + import javax.validation.constraints.Min; + + @Entity + public record Person(@NotNull String name, @Min(0) int age) { + } + """ + ) + ); + } + + @Test + void booleanGetterWithIsPrefix() { + rewriteRun( + //language=java + java( + """ + class User { + private final String name; + private final boolean active; + + public User(String name, boolean active) { + this.name = name; + this.active = active; + } + + public String getName() { + return name; + } + + public boolean isActive() { + return active; + } + } + """, + """ + record User(String name, boolean active) { + } + """ + ) + ); + } + + @Test + void classWithCollectionFields() { + rewriteRun( + //language=java + java( + """ + import java.util.List; + import java.util.Set; + + class Student { + private final String name; + private final List courses; + private final Set skills; + + public Student(String name, List courses, Set skills) { + this.name = name; + this.courses = courses; + this.skills = skills; + } + + public String getName() { + return name; + } + + public List getCourses() { + return courses; + } + + public Set getSkills() { + return skills; + } + } + """, + """ + import java.util.List; + import java.util.Set; + + record Student(String name, List courses, Set skills) { + } + """ + ) + ); + } + + @Test + void classWithInterfaceImplementation() { + rewriteRun( + //language=java + java( + """ + interface Identifiable { + String getId(); + } + + class Entity implements Identifiable { + private final String id; + private final String name; + + public Entity(String id, String name) { + this.id = id; + this.name = name; + } + + public String getId() { + return id; + } + + public String getName() { + return name; + } + } + """, + """ + interface Identifiable { + String getId(); + } + + record Entity(String id, String name) implements Identifiable { + } + """ + ) + ); + } + + @Test + void shouldNotConvertClassWithSetterMethods() { + rewriteRun( + //language=java + java( + """ + class MutablePerson { + private String name; + private int age; + + public String getName() { + return name; + } + + public void setName(String name) { + this.name = name; + } + + public int getAge() { + return age; + } + + public void setAge(int age) { + this.age = age; + } + } + """ + ) + ); + } + + @Test + void shouldNotConvertClassThatExtendsAnotherClass() { + rewriteRun( + //language=java + java( + """ + class BaseEntity { + private final String id; + + public BaseEntity(String id) { + this.id = id; + } + + public String getId() { + return id; + } + } + + class Person extends BaseEntity { + private final String name; + + public Person(String id, String name) { + super(id); + this.name = name; + } + + public String getName() { + return name; + } + } + """ + ) + ); + } + + @Test + void shouldNotConvertClassWithoutGetters() { + rewriteRun( + //language=java + java( + """ + class DataHolder { + private final String data; + + public DataHolder(String data) { + this.data = data; + } + + public void processData() { + System.out.println(data); + } + } + """ + ) + ); + } + + @Test + void shouldNotConvertClassWithMissingGetters() { + rewriteRun( + //language=java + java( + """ + class PartialPerson { + private final String name; + private final int age; + + public PartialPerson(String name, int age) { + this.name = name; + this.age = age; + } + + public String getName() { + return name; + } + + // Missing getAge() method + } + """ + ) + ); + } + + @Test + void shouldNotConvertEnum() { + rewriteRun( + //language=java + java( + """ + enum Status { + ACTIVE, + INACTIVE; + + public boolean isActive() { + return this == ACTIVE; + } + } + """ + ) + ); + } + + @Test + void shouldNotConvertInterface() { + rewriteRun( + //language=java + java( + """ + interface Repository { + String getName(); + int getVersion(); + } + """ + ) + ); + } + + @Test + void shouldNotConvertAbstractClass() { + rewriteRun( + //language=java + java( + """ + abstract class AbstractEntity { + private final String id; + + public AbstractEntity(String id) { + this.id = id; + } + + public String getId() { + return id; + } + + public abstract void process(); + } + """ + ) + ); + } +} From aae284988457e8beb70e6d93d15c4ade8d2efa34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Tue, 14 Oct 2025 15:15:58 +0200 Subject: [PATCH 03/10] Acknowledge that we only check file internal ATM --- .../FindImmutableThreadLocalVariables.java | 18 +++++-- ...FindImmutableThreadLocalVariablesTest.java | 52 +++++++++++++++++++ 2 files changed, 65 insertions(+), 5 deletions(-) diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java b/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java index 0ce9fa690d..1f84c563f7 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java +++ b/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java @@ -46,7 +46,9 @@ public String getDisplayName() { @Override public String getDescription() { return "Find `ThreadLocal` variables that are never mutated and could be candidates for migration to `ScopedValue` in Java 25+. " + - "This recipe identifies `ThreadLocal` variables that are only initialized but never reassigned or modified through `set()` or `remove()` methods."; + "This recipe identifies `ThreadLocal` variables that are only initialized but never reassigned or modified through `set()` or `remove()` methods. " + + "Note: This recipe only analyzes mutations within the same source file. ThreadLocal fields accessible from other classes " + + "(public, protected, or package-private) may be mutated elsewhere in the codebase."; } @Value @@ -184,10 +186,16 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m if (isThreadLocalType(variable.getType())) { ThreadLocalVariable tlVar = ThreadLocalVariable.fromVariable(variable, classDecl); if (acc.isImmutable(tlVar)) { - return SearchResult.found( - multiVariable, - "ThreadLocal candidate for ScopedValue migration - never mutated after initialization" - ); + // Only mark private fields as candidates, since public/protected/package-private + // fields could be mutated from other classes + boolean isPrivate = multiVariable.getModifiers().stream() + .anyMatch(mod -> mod.getType() == J.Modifier.Type.Private); + + String message = isPrivate + ? "ThreadLocal candidate for ScopedValue migration - never mutated after initialization" + : "ThreadLocal candidate for ScopedValue migration - never mutated in this file (but may be mutated elsewhere due to non-private access)"; + + return SearchResult.found(multiVariable, message); } } } diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java b/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java index b2191c1ec4..6ec5696858 100644 --- a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java +++ b/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java @@ -267,6 +267,58 @@ public void method() { ); } + @Test + void warnAboutPackagePrivateThreadLocal() { + rewriteRun( + java( + """ + class Example { + static final ThreadLocal PACKAGE_TL = new ThreadLocal<>(); + + public String getValue() { + return PACKAGE_TL.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated in this file (but may be mutated elsewhere due to non-private access))~~>*/static final ThreadLocal PACKAGE_TL = new ThreadLocal<>(); + + public String getValue() { + return PACKAGE_TL.get(); + } + } + """ + ) + ); + } + + @Test + void warnAboutProtectedThreadLocal() { + rewriteRun( + java( + """ + class Example { + protected static final ThreadLocal PROTECTED_TL = new ThreadLocal<>(); + + public String getValue() { + return PROTECTED_TL.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated in this file (but may be mutated elsewhere due to non-private access))~~>*/protected static final ThreadLocal PROTECTED_TL = new ThreadLocal<>(); + + public String getValue() { + return PROTECTED_TL.get(); + } + } + """ + ) + ); + } + @Test void handleInheritableThreadLocal() { rewriteRun( From 92368e889ed77c0e2c45f1ab09039ccbb4772b48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Wed, 15 Oct 2025 10:33:57 +0200 Subject: [PATCH 04/10] enable scanning capabilities to detect ThreadLocals that are mutated in outher files --- .../FindImmutableThreadLocalVariables.java | 310 +++++++++---- ...ableThreadLocalVariablesCrossFileTest.java | 419 ++++++++++++++++++ ...FindImmutableThreadLocalVariablesTest.java | 4 +- 3 files changed, 652 insertions(+), 81 deletions(-) create mode 100644 src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesCrossFileTest.java diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java b/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java index 1f84c563f7..71389ebb15 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java +++ b/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java @@ -19,19 +19,21 @@ import lombok.Value; import org.openrewrite.ExecutionContext; import org.openrewrite.ScanningRecipe; +import org.openrewrite.SourceFile; import org.openrewrite.TreeVisitor; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JavaType; import org.openrewrite.java.tree.TypeUtils; import org.openrewrite.marker.SearchResult; -import java.util.HashSet; -import java.util.Set; +import java.nio.file.Path; +import java.util.*; @EqualsAndHashCode(callSuper = false) -public class FindImmutableThreadLocalVariables extends ScanningRecipe { +public class FindImmutableThreadLocalVariables extends ScanningRecipe { private static final MethodMatcher THREAD_LOCAL_SET = new MethodMatcher("java.lang.ThreadLocal set(..)"); private static final MethodMatcher THREAD_LOCAL_REMOVE = new MethodMatcher("java.lang.ThreadLocal remove()"); @@ -47,55 +49,153 @@ public String getDisplayName() { public String getDescription() { return "Find `ThreadLocal` variables that are never mutated and could be candidates for migration to `ScopedValue` in Java 25+. " + "This recipe identifies `ThreadLocal` variables that are only initialized but never reassigned or modified through `set()` or `remove()` methods. " + - "Note: This recipe only analyzes mutations within the same source file. ThreadLocal fields accessible from other classes " + - "(public, protected, or package-private) may be mutated elsewhere in the codebase."; + "The recipe analyzes mutations across all source files in the project to provide accurate results."; } @Value - static class ThreadLocalAccumulator { - Set allThreadLocals = new HashSet<>(); - Set mutatedThreadLocals = new HashSet<>(); + static class ThreadLocalUsageAccumulator { + // Map of ThreadLocal declarations: fullyQualifiedName -> declaration info + Map declarations = new HashMap<>(); + // Map of ThreadLocal mutations: fullyQualifiedName -> list of mutation locations + Map> mutations = new HashMap<>(); + // Map of ThreadLocal accesses: fullyQualifiedName -> list of access locations + Map> accesses = new HashMap<>(); - public boolean isImmutable(ThreadLocalVariable var) { - return allThreadLocals.contains(var) && !mutatedThreadLocals.contains(var); + void addDeclaration(String fullyQualifiedName, ThreadLocalDeclaration declaration) { + declarations.put(fullyQualifiedName, declaration); + } + + void addMutation(String fullyQualifiedName, MutationLocation mutation) { + mutations.computeIfAbsent(fullyQualifiedName, k -> new ArrayList<>()).add(mutation); + } + + void addAccess(String fullyQualifiedName, AccessLocation access) { + accesses.computeIfAbsent(fullyQualifiedName, k -> new ArrayList<>()).add(access); + } + + boolean hasExternalMutations(String fullyQualifiedName) { + List mutationList = mutations.get(fullyQualifiedName); + if (mutationList == null || mutationList.isEmpty()) { + return false; + } + + ThreadLocalDeclaration declaration = declarations.get(fullyQualifiedName); + if (declaration == null) { + return true; // Conservative: if we can't find the declaration, assume it can be mutated + } + + // Check if any mutation is from a different file than the declaration + for (MutationLocation mutation : mutationList) { + if (!mutation.getSourcePath().equals(declaration.getSourcePath())) { + return true; + } + } + return false; + } + + boolean hasMutations(String fullyQualifiedName) { + return mutations.containsKey(fullyQualifiedName) && !mutations.get(fullyQualifiedName).isEmpty(); } } @Value - static class ThreadLocalVariable { - String name; + static class ThreadLocalDeclaration { + String fullyQualifiedName; // e.g., "com.example.MyClass.MY_THREAD_LOCAL" String className; + String fieldName; + boolean isPrivate; + boolean isStatic; + boolean isFinal; + Path sourcePath; + } - static ThreadLocalVariable fromVariable(J.VariableDeclarations.NamedVariable variable, J.ClassDeclaration classDecl) { - String className = classDecl != null && classDecl.getType() != null ? - classDecl.getType().getFullyQualifiedName() : ""; - return new ThreadLocalVariable(variable.getName().getSimpleName(), className); + @Value + static class MutationLocation { + Path sourcePath; + String className; + String methodName; + MutationType type; + + enum MutationType { + REASSIGNMENT, + SET_CALL, + REMOVE_CALL, + OTHER + } + } + + @Value + static class AccessLocation { + Path sourcePath; + String className; + String methodName; + AccessType type; + + enum AccessType { + GET_CALL, + FIELD_READ, + OTHER } } @Override - public ThreadLocalAccumulator getInitialValue(ExecutionContext ctx) { - return new ThreadLocalAccumulator(); + public ThreadLocalUsageAccumulator getInitialValue(ExecutionContext ctx) { + return new ThreadLocalUsageAccumulator(); } @Override - public TreeVisitor getScanner(ThreadLocalAccumulator acc) { + public TreeVisitor getScanner(ThreadLocalUsageAccumulator acc) { return new JavaIsoVisitor() { + @Override + public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionContext ctx) { + // Store the current compilation unit's path for tracking locations + getCursor().putMessage("currentSourcePath", cu.getSourcePath()); + return super.visitCompilationUnit(cu, ctx); + } + @Override public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, ExecutionContext ctx) { variable = super.visitVariable(variable, ctx); if (isThreadLocalType(variable.getType())) { + // Check if this is a field (not a local variable) + // A field is declared directly in a class, not inside a method + J.MethodDeclaration enclosingMethod = getCursor().firstEnclosing(J.MethodDeclaration.class); J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - ThreadLocalVariable tlVar = ThreadLocalVariable.fromVariable(variable, classDecl); - acc.allThreadLocals.add(tlVar); - // Check if this is a local variable - we don't mark local variables - J.Block enclosingBlock = getCursor().firstEnclosing(J.Block.class); - J.MethodDeclaration enclosingMethod = getCursor().firstEnclosing(J.MethodDeclaration.class); - if (enclosingMethod != null && enclosingBlock != null && enclosingBlock != enclosingMethod.getBody()) { - // This is a local variable inside a method - acc.mutatedThreadLocals.add(tlVar); + // It's a field if there's no enclosing method, or if the variable is declared + // directly in the class body (not inside a method body) + boolean isField = classDecl != null && (enclosingMethod == null || + getCursor().getPathAsStream() + .filter(J.Block.class::isInstance) + .map(J.Block.class::cast) + .noneMatch(block -> block == enclosingMethod.getBody())); + + if (isField) { + J.VariableDeclarations variableDecls = getCursor().firstEnclosing(J.VariableDeclarations.class); + + if (classDecl != null && variableDecls != null) { + String className = classDecl.getType() != null ? + classDecl.getType().getFullyQualifiedName() : "UnknownClass"; + String fieldName = variable.getName().getSimpleName(); + String fullyQualifiedName = className + "." + fieldName; + + boolean isPrivate = variableDecls.getModifiers().stream() + .anyMatch(mod -> mod.getType() == J.Modifier.Type.Private); + boolean isStatic = variableDecls.getModifiers().stream() + .anyMatch(mod -> mod.getType() == J.Modifier.Type.Static); + boolean isFinal = variableDecls.getModifiers().stream() + .anyMatch(mod -> mod.getType() == J.Modifier.Type.Final); + + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); + + ThreadLocalDeclaration declaration = new ThreadLocalDeclaration( + fullyQualifiedName, className, fieldName, + isPrivate, isStatic, isFinal, sourcePath + ); + + acc.addDeclaration(fullyQualifiedName, declaration); + } } } return variable; @@ -105,14 +205,22 @@ public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations public J.Assignment visitAssignment(J.Assignment assignment, ExecutionContext ctx) { assignment = super.visitAssignment(assignment, ctx); - // Check if we're reassigning a ThreadLocal variable - if (assignment.getVariable() instanceof J.Identifier) { - J.Identifier id = (J.Identifier) assignment.getVariable(); - if (isThreadLocalType(id.getType())) { + // Check if we're reassigning a ThreadLocal field + if (isThreadLocalFieldAccess(assignment.getVariable())) { + String fullyQualifiedName = getFieldFullyQualifiedName(assignment.getVariable()); + if (fullyQualifiedName != null) { + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + J.MethodDeclaration methodDecl = getCursor().firstEnclosing(J.MethodDeclaration.class); + String className = classDecl != null && classDecl.getType() != null ? - classDecl.getType().getFullyQualifiedName() : ""; - acc.mutatedThreadLocals.add(new ThreadLocalVariable(id.getSimpleName(), className)); + classDecl.getType().getFullyQualifiedName() : "UnknownClass"; + String methodName = methodDecl != null ? methodDecl.getSimpleName() : "UnknownMethod"; + + MutationLocation mutation = new MutationLocation( + sourcePath, className, methodName, MutationLocation.MutationType.REASSIGNMENT + ); + acc.addMutation(fullyQualifiedName, mutation); } } return assignment; @@ -126,55 +234,80 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu if (THREAD_LOCAL_SET.matches(method) || THREAD_LOCAL_REMOVE.matches(method) || INHERITABLE_THREAD_LOCAL_SET.matches(method) || INHERITABLE_THREAD_LOCAL_REMOVE.matches(method)) { - J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - String className = classDecl != null && classDecl.getType() != null ? - classDecl.getType().getFullyQualifiedName() : ""; - - // Get the ThreadLocal instance being mutated - String varName = null; - if (method.getSelect() instanceof J.Identifier) { - varName = ((J.Identifier) method.getSelect()).getSimpleName(); - } else if (method.getSelect() instanceof J.FieldAccess) { - varName = ((J.FieldAccess) method.getSelect()).getSimpleName(); + String fullyQualifiedName = getFieldFullyQualifiedName(method.getSelect()); + if (fullyQualifiedName != null) { + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + J.MethodDeclaration methodDecl = getCursor().firstEnclosing(J.MethodDeclaration.class); + + String className = classDecl != null && classDecl.getType() != null ? + classDecl.getType().getFullyQualifiedName() : "UnknownClass"; + String methodName = methodDecl != null ? methodDecl.getSimpleName() : "UnknownMethod"; + + MutationLocation.MutationType mutationType = + (THREAD_LOCAL_SET.matches(method) || INHERITABLE_THREAD_LOCAL_SET.matches(method)) ? + MutationLocation.MutationType.SET_CALL : MutationLocation.MutationType.REMOVE_CALL; + + MutationLocation mutation = new MutationLocation( + sourcePath, className, methodName, mutationType + ); + acc.addMutation(fullyQualifiedName, mutation); } + } else if (method.getSimpleName().equals("get") && isThreadLocalType(method.getType())) { + // Track get() calls for completeness + String fullyQualifiedName = getFieldFullyQualifiedName(method.getSelect()); + if (fullyQualifiedName != null) { + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + J.MethodDeclaration methodDecl = getCursor().firstEnclosing(J.MethodDeclaration.class); - if (varName != null) { - acc.mutatedThreadLocals.add(new ThreadLocalVariable(varName, className)); + String className = classDecl != null && classDecl.getType() != null ? + classDecl.getType().getFullyQualifiedName() : "UnknownClass"; + String methodName = methodDecl != null ? methodDecl.getSimpleName() : "UnknownMethod"; + + AccessLocation access = new AccessLocation( + sourcePath, className, methodName, AccessLocation.AccessType.GET_CALL + ); + acc.addAccess(fullyQualifiedName, access); } } return method; } - @Override - public J.Unary visitUnary(J.Unary unary, ExecutionContext ctx) { - unary = super.visitUnary(unary, ctx); - - // Check for operations that would mutate the ThreadLocal reference (unlikely but thorough) - if (unary.getExpression() instanceof J.Identifier) { - J.Identifier id = (J.Identifier) unary.getExpression(); - if (isThreadLocalType(id.getType())) { - switch (unary.getOperator()) { - case PreIncrement: - case PreDecrement: - case PostIncrement: - case PostDecrement: - J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - String className = classDecl != null && classDecl.getType() != null ? - classDecl.getType().getFullyQualifiedName() : ""; - acc.mutatedThreadLocals.add(new ThreadLocalVariable(id.getSimpleName(), className)); - break; - default: - break; - } + private boolean isThreadLocalFieldAccess(Expression expression) { + if (expression instanceof J.Identifier) { + J.Identifier id = (J.Identifier) expression; + return isThreadLocalType(id.getType()); + } else if (expression instanceof J.FieldAccess) { + J.FieldAccess fieldAccess = (J.FieldAccess) expression; + return isThreadLocalType(fieldAccess.getType()); + } + return false; + } + + private String getFieldFullyQualifiedName(Expression expression) { + if (expression instanceof J.Identifier) { + J.Identifier id = (J.Identifier) expression; + JavaType.Variable varType = id.getFieldType(); + if (varType != null && varType.getOwner() instanceof JavaType.FullyQualified) { + JavaType.FullyQualified owner = (JavaType.FullyQualified) varType.getOwner(); + return owner.getFullyQualifiedName() + "." + varType.getName(); + } + } else if (expression instanceof J.FieldAccess) { + J.FieldAccess fieldAccess = (J.FieldAccess) expression; + JavaType.Variable varType = fieldAccess.getName().getFieldType(); + if (varType != null && varType.getOwner() instanceof JavaType.FullyQualified) { + JavaType.FullyQualified owner = (JavaType.FullyQualified) varType.getOwner(); + return owner.getFullyQualifiedName() + "." + varType.getName(); } } - return unary; + return null; } }; } @Override - public TreeVisitor getVisitor(ThreadLocalAccumulator acc) { + public TreeVisitor getVisitor(ThreadLocalUsageAccumulator acc) { return new JavaIsoVisitor() { @Override public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { @@ -182,20 +315,39 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m // Check if any of the variables in this declaration are immutable ThreadLocals J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + for (J.VariableDeclarations.NamedVariable variable : multiVariable.getVariables()) { - if (isThreadLocalType(variable.getType())) { - ThreadLocalVariable tlVar = ThreadLocalVariable.fromVariable(variable, classDecl); - if (acc.isImmutable(tlVar)) { - // Only mark private fields as candidates, since public/protected/package-private - // fields could be mutated from other classes - boolean isPrivate = multiVariable.getModifiers().stream() - .anyMatch(mod -> mod.getType() == J.Modifier.Type.Private); + if (isThreadLocalType(variable.getType()) && classDecl != null) { + String className = classDecl.getType() != null ? + classDecl.getType().getFullyQualifiedName() : "UnknownClass"; + String fieldName = variable.getName().getSimpleName(); + String fullyQualifiedName = className + "." + fieldName; + + ThreadLocalDeclaration declaration = acc.declarations.get(fullyQualifiedName); + if (declaration != null) { + boolean hasMutations = acc.hasMutations(fullyQualifiedName); + boolean hasExternalMutations = acc.hasExternalMutations(fullyQualifiedName); - String message = isPrivate - ? "ThreadLocal candidate for ScopedValue migration - never mutated after initialization" - : "ThreadLocal candidate for ScopedValue migration - never mutated in this file (but may be mutated elsewhere due to non-private access)"; + if (!hasMutations) { + // No mutations at all + String message = declaration.isPrivate() ? + "ThreadLocal candidate for ScopedValue migration - never mutated after initialization" : + "ThreadLocal candidate for ScopedValue migration - never mutated in project (but accessible from outside due to non-private access)"; + return SearchResult.found(multiVariable, message); + } else if (!hasExternalMutations && declaration.isPrivate()) { + // Has mutations, but only in the declaring file and it's private + // This is still safe since no external access is possible + // However, we should check if mutations are only in the initializer + List mutations = acc.mutations.get(fullyQualifiedName); + boolean onlyLocalMutations = mutations.stream() + .allMatch(m -> m.getSourcePath().equals(declaration.getSourcePath())); - return SearchResult.found(multiVariable, message); + if (onlyLocalMutations) { + // Check if it's only mutated in the same class + // For now, we'll be conservative and not mark it + continue; + } + } } } } diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesCrossFileTest.java b/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesCrossFileTest.java new file mode 100644 index 0000000000..61dec9d617 --- /dev/null +++ b/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesCrossFileTest.java @@ -0,0 +1,419 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.search; + +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; + +class FindImmutableThreadLocalVariablesCrossFileTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new FindImmutableThreadLocalVariables()); + } + + @DocumentExample + @Test + void detectMutationFromAnotherClassInSamePackage() { + rewriteRun( + // First class with package-private ThreadLocal + java( + """ + package com.example; + + class ThreadLocalHolder { + static final ThreadLocal SHARED_TL = new ThreadLocal<>(); + + public String getValue() { + return SHARED_TL.get(); + } + } + """ + ), + // Second class that mutates the ThreadLocal + java( + """ + package com.example; + + class ThreadLocalMutator { + public void mutate() { + ThreadLocalHolder.SHARED_TL.set("mutated"); + } + + public void cleanup() { + ThreadLocalHolder.SHARED_TL.remove(); + } + } + """ + ) + ); + // The ThreadLocal should NOT be marked as immutable because it's mutated in ThreadLocalMutator + } + + @Test + void detectNoMutationAcrossMultipleClasses() { + rewriteRun( + java( + """ + package com.example; + + public class ReadOnlyHolder { + public static final ThreadLocal COUNTER = ThreadLocal.withInitial(() -> 0); + + public int getCount() { + return COUNTER.get(); + } + } + """, + """ + package com.example; + + public class ReadOnlyHolder { + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated in project (but accessible from outside due to non-private access))~~>*/public static final ThreadLocal COUNTER = ThreadLocal.withInitial(() -> 0); + + public int getCount() { + return COUNTER.get(); + } + } + """ + ), + java( + """ + package com.example; + + class Reader1 { + public void readValue() { + Integer value = ReadOnlyHolder.COUNTER.get(); + System.out.println(value); + } + } + """ + ), + java( + """ + package com.example; + + class Reader2 { + public int calculate() { + return ReadOnlyHolder.COUNTER.get() + 10; + } + } + """ + ) + ); + // The ThreadLocal should be marked with a warning since it's public but never mutated + } + + @Test + void detectMutationThroughInheritance() { + rewriteRun( + java( + """ + package com.example; + + public class BaseClass { + protected static final ThreadLocal PROTECTED_TL = new ThreadLocal<>(); + + public String getValue() { + return PROTECTED_TL.get(); + } + } + """ + ), + java( + """ + package com.example.sub; + + import com.example.BaseClass; + + public class SubClass extends BaseClass { + public void modifyThreadLocal() { + PROTECTED_TL.set("modified by subclass"); + } + } + """ + ) + ); + // The ThreadLocal should NOT be marked as immutable because it's mutated in SubClass + } + + @Test + void privateThreadLocalNotAccessibleFromOtherClass() { + rewriteRun( + java( + """ + package com.example; + + public class PrivateHolder { + private static final ThreadLocal PRIVATE_TL = new ThreadLocal<>(); + + public String getValue() { + return PRIVATE_TL.get(); + } + + public static class NestedClass { + public void tryToAccess() { + // Can access private field from nested class + String value = PRIVATE_TL.get(); + } + } + } + """, + """ + package com.example; + + public class PrivateHolder { + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal PRIVATE_TL = new ThreadLocal<>(); + + public String getValue() { + return PRIVATE_TL.get(); + } + + public static class NestedClass { + public void tryToAccess() { + // Can access private field from nested class + String value = PRIVATE_TL.get(); + } + } + } + """ + ), + java( + """ + package com.example; + + class ExternalClass { + public void cannotAccess() { + // Cannot access private ThreadLocal from PrivateHolder + // This class cannot mutate PRIVATE_TL + } + } + """ + ) + ); + // Private ThreadLocal should be marked as immutable since it can't be mutated externally + } + + @Test + void detectMutationInNestedClass() { + rewriteRun( + java( + """ + package com.example; + + public class OuterClass { + private static final ThreadLocal TL = new ThreadLocal<>(); + + public String getValue() { + return TL.get(); + } + + public static class InnerClass { + public void mutate() { + TL.set("mutated by inner class"); + } + } + } + """ + ) + ); + // ThreadLocal should NOT be marked as immutable because it's mutated in InnerClass + } + + @Test + void detectMutationThroughStaticImport() { + rewriteRun( + java( + """ + package com.example; + + public class ThreadLocalProvider { + public static final ThreadLocal STATIC_TL = new ThreadLocal<>(); + } + """ + ), + java( + """ + package com.example.user; + + import static com.example.ThreadLocalProvider.STATIC_TL; + + public class StaticImportUser { + public void mutate() { + STATIC_TL.set("mutated through static import"); + } + } + """ + ) + ); + // ThreadLocal should NOT be marked as immutable due to mutation through static import + } + + @Test + void multipleThreadLocalsWithMixedAccessPatterns() { + rewriteRun( + java( + """ + package com.example; + + public class MultipleThreadLocals { + private static final ThreadLocal PRIVATE_IMMUTABLE = new ThreadLocal<>(); + static final ThreadLocal PACKAGE_MUTATED = new ThreadLocal<>(); + public static final ThreadLocal PUBLIC_READ_ONLY = new ThreadLocal<>(); + protected static final ThreadLocal PROTECTED_MUTATED = new ThreadLocal<>(); + + public void readAll() { + String p1 = PRIVATE_IMMUTABLE.get(); + String p2 = PACKAGE_MUTATED.get(); + String p3 = PUBLIC_READ_ONLY.get(); + String p4 = PROTECTED_MUTATED.get(); + } + } + """, + """ + package com.example; + + public class MultipleThreadLocals { + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal PRIVATE_IMMUTABLE = new ThreadLocal<>(); + static final ThreadLocal PACKAGE_MUTATED = new ThreadLocal<>(); + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated in project (but accessible from outside due to non-private access))~~>*/public static final ThreadLocal PUBLIC_READ_ONLY = new ThreadLocal<>(); + protected static final ThreadLocal PROTECTED_MUTATED = new ThreadLocal<>(); + + public void readAll() { + String p1 = PRIVATE_IMMUTABLE.get(); + String p2 = PACKAGE_MUTATED.get(); + String p3 = PUBLIC_READ_ONLY.get(); + String p4 = PROTECTED_MUTATED.get(); + } + } + """ + ), + java( + """ + package com.example; + + class Mutator { + public void mutate() { + MultipleThreadLocals.PACKAGE_MUTATED.set("mutated"); + MultipleThreadLocals.PROTECTED_MUTATED.remove(); + } + + public void readOnly() { + String value = MultipleThreadLocals.PUBLIC_READ_ONLY.get(); + } + } + """ + ) + ); + } + + @Test + void detectMutationThroughMethodReference() { + rewriteRun( + java( + """ + package com.example; + + import java.util.function.Consumer; + + public class MethodReferenceExample { + public static final ThreadLocal TL = new ThreadLocal<>(); + + public static void setValue(String value) { + TL.set(value); + } + + public void useMethodReference() { + Consumer setter = MethodReferenceExample::setValue; + setter.accept("value"); + } + } + """ + ) + ); + // ThreadLocal is mutated through setValue method, should NOT be marked as immutable + } + + @Test + void detectIndirectMutationThroughPublicSetter() { + rewriteRun( + java( + """ + package com.example; + + public class IndirectMutation { + private static final ThreadLocal PRIVATE_TL = new ThreadLocal<>(); + + public static void setThreadLocalValue(String value) { + PRIVATE_TL.set(value); + } + + public static String getThreadLocalValue() { + return PRIVATE_TL.get(); + } + } + """ + ), + java( + """ + package com.example; + + class ExternalSetter { + public void mutateIndirectly() { + IndirectMutation.setThreadLocalValue("mutated"); + } + } + """ + ) + ); + // ThreadLocal should NOT be marked as immutable because setThreadLocalValue mutates it + } + + @Test + void instanceThreadLocalWithCrossFileAccess() { + rewriteRun( + java( + """ + package com.example; + + public class InstanceThreadLocalHolder { + public final ThreadLocal instanceTL = new ThreadLocal<>(); + + public String getValue() { + return instanceTL.get(); + } + } + """ + ), + java( + """ + package com.example; + + class InstanceMutator { + public void mutate(InstanceThreadLocalHolder holder) { + holder.instanceTL.set("mutated"); + } + } + """ + ) + ); + // Instance ThreadLocal should NOT be marked as immutable due to external mutation + } +} \ No newline at end of file diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java b/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java index 6ec5696858..fc08a334b3 100644 --- a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java +++ b/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java @@ -282,7 +282,7 @@ public String getValue() { """, """ class Example { - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated in this file (but may be mutated elsewhere due to non-private access))~~>*/static final ThreadLocal PACKAGE_TL = new ThreadLocal<>(); + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated in project (but accessible from outside due to non-private access))~~>*/static final ThreadLocal PACKAGE_TL = new ThreadLocal<>(); public String getValue() { return PACKAGE_TL.get(); @@ -308,7 +308,7 @@ public String getValue() { """, """ class Example { - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated in this file (but may be mutated elsewhere due to non-private access))~~>*/protected static final ThreadLocal PROTECTED_TL = new ThreadLocal<>(); + /*~~(ThreadLocal candidate for ScopedValue migration - never mutated in project (but accessible from outside due to non-private access))~~>*/protected static final ThreadLocal PROTECTED_TL = new ThreadLocal<>(); public String getValue() { return PROTECTED_TL.get(); From 347870f1dd45f0d311b8ef181f58c245bf1c1252 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Wed, 15 Oct 2025 11:01:03 +0200 Subject: [PATCH 05/10] add preconditions --- .../ConvertImmutableClassToRecord.java | 7 +- .../FindImmutableThreadLocalVariables.java | 382 +++++++++--------- ...ableThreadLocalVariablesCrossFileTest.java | 2 +- ...FindImmutableThreadLocalVariablesTest.java | 2 +- 4 files changed, 187 insertions(+), 206 deletions(-) diff --git a/src/main/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecord.java b/src/main/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecord.java index fefe222a22..aa7c2496ed 100644 --- a/src/main/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecord.java +++ b/src/main/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecord.java @@ -139,7 +139,9 @@ private J.ClassDeclaration transformToRecord(J.ClassDeclaration classDecl) { if (!classDecl.getImplements().isEmpty()) { recordTemplate.append(" implements "); for (int i = 0; i < classDecl.getImplements().size(); i++) { - if (i > 0) recordTemplate.append(", "); + if (i > 0) { + recordTemplate.append( ", " ); + } recordTemplate.append("#{any()}"); } } @@ -243,8 +245,7 @@ private boolean isSetterMethod(J.MethodDeclaration method) { && method.getParameters().size() == 1; } - @Nullable - private String getFieldNameFromGetter(J.MethodDeclaration getter) { + private @Nullable String getFieldNameFromGetter(J.MethodDeclaration getter) { String methodName = getter.getSimpleName(); if (methodName.startsWith("get") && methodName.length() > 3) { return Character.toLowerCase(methodName.charAt(3)) + methodName.substring(4); diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java b/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java index 71389ebb15..2adbfcfed5 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java +++ b/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java @@ -23,6 +23,8 @@ import org.openrewrite.TreeVisitor; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.search.UsesJavaVersion; +import org.openrewrite.java.search.UsesType; import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; import org.openrewrite.java.tree.JavaType; @@ -30,15 +32,22 @@ import org.openrewrite.marker.SearchResult; import java.nio.file.Path; -import java.util.*; +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.openrewrite.Preconditions.*; @EqualsAndHashCode(callSuper = false) public class FindImmutableThreadLocalVariables extends ScanningRecipe { - private static final MethodMatcher THREAD_LOCAL_SET = new MethodMatcher("java.lang.ThreadLocal set(..)"); - private static final MethodMatcher THREAD_LOCAL_REMOVE = new MethodMatcher("java.lang.ThreadLocal remove()"); - private static final MethodMatcher INHERITABLE_THREAD_LOCAL_SET = new MethodMatcher("java.lang.InheritableThreadLocal set(..)"); - private static final MethodMatcher INHERITABLE_THREAD_LOCAL_REMOVE = new MethodMatcher("java.lang.InheritableThreadLocal remove()"); + private static final String THREAD_LOCAL_FQN = "java.lang.ThreadLocal"; + private static final String INHERITED_THREAD_LOCAL_FQN = "java.lang.InheritableThreadLocal"; + private static final MethodMatcher THREAD_LOCAL_SET = new MethodMatcher(THREAD_LOCAL_FQN + " set(..)"); + private static final MethodMatcher THREAD_LOCAL_REMOVE = new MethodMatcher(THREAD_LOCAL_FQN + " remove()"); + private static final MethodMatcher INHERITABLE_THREAD_LOCAL_SET = new MethodMatcher(INHERITED_THREAD_LOCAL_FQN + " set(..)"); + private static final MethodMatcher INHERITABLE_THREAD_LOCAL_REMOVE = new MethodMatcher(INHERITED_THREAD_LOCAL_FQN + " remove()"); @Override public String getDisplayName() { @@ -47,9 +56,7 @@ public String getDisplayName() { @Override public String getDescription() { - return "Find `ThreadLocal` variables that are never mutated and could be candidates for migration to `ScopedValue` in Java 25+. " + - "This recipe identifies `ThreadLocal` variables that are only initialized but never reassigned or modified through `set()` or `remove()` methods. " + - "The recipe analyzes mutations across all source files in the project to provide accurate results."; + return "Find `ThreadLocal` variables that are never mutated and could be candidates for migration to `ScopedValue` in Java 25+. " + "This recipe identifies `ThreadLocal` variables that are only initialized but never reassigned or modified through `set()` or `remove()` methods. " + "The recipe analyzes mutations across all source files in the project to provide accurate results."; } @Value @@ -117,10 +124,7 @@ static class MutationLocation { MutationType type; enum MutationType { - REASSIGNMENT, - SET_CALL, - REMOVE_CALL, - OTHER + REASSIGNMENT, SET_CALL, REMOVE_CALL, OTHER } } @@ -132,9 +136,7 @@ static class AccessLocation { AccessType type; enum AccessType { - GET_CALL, - FIELD_READ, - OTHER + GET_CALL, FIELD_READ, OTHER } } @@ -145,220 +147,198 @@ public ThreadLocalUsageAccumulator getInitialValue(ExecutionContext ctx) { @Override public TreeVisitor getScanner(ThreadLocalUsageAccumulator acc) { - return new JavaIsoVisitor() { - @Override - public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionContext ctx) { - // Store the current compilation unit's path for tracking locations - getCursor().putMessage("currentSourcePath", cu.getSourcePath()); - return super.visitCompilationUnit(cu, ctx); - } + return check( + and(new UsesJavaVersion<>(25), + or(new UsesType<>(THREAD_LOCAL_FQN, true), new UsesType<>(INHERITED_THREAD_LOCAL_FQN, true))), + new JavaIsoVisitor() { + @Override + public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionContext ctx) { + // Store the current compilation unit's path for tracking locations + getCursor().putMessage("currentSourcePath", cu.getSourcePath()); + return super.visitCompilationUnit(cu, ctx); + } + + @Override + public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, ExecutionContext ctx) { + variable = super.visitVariable(variable, ctx); + + if (isThreadLocalType(variable.getType())) { + // Check if this is a field (not a local variable) + // A field is declared directly in a class, not inside a method + J.MethodDeclaration enclosingMethod = getCursor().firstEnclosing(J.MethodDeclaration.class); + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + + // It's a field if there's no enclosing method, or if the variable is declared + // directly in the class body (not inside a method body) + boolean isField = classDecl != null && (enclosingMethod == null || getCursor().getPathAsStream().filter(J.Block.class::isInstance).map(J.Block.class::cast).noneMatch(block -> block == enclosingMethod.getBody())); - @Override - public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, ExecutionContext ctx) { - variable = super.visitVariable(variable, ctx); - - if (isThreadLocalType(variable.getType())) { - // Check if this is a field (not a local variable) - // A field is declared directly in a class, not inside a method - J.MethodDeclaration enclosingMethod = getCursor().firstEnclosing(J.MethodDeclaration.class); - J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - - // It's a field if there's no enclosing method, or if the variable is declared - // directly in the class body (not inside a method body) - boolean isField = classDecl != null && (enclosingMethod == null || - getCursor().getPathAsStream() - .filter(J.Block.class::isInstance) - .map(J.Block.class::cast) - .noneMatch(block -> block == enclosingMethod.getBody())); - - if (isField) { - J.VariableDeclarations variableDecls = getCursor().firstEnclosing(J.VariableDeclarations.class); - - if (classDecl != null && variableDecls != null) { - String className = classDecl.getType() != null ? - classDecl.getType().getFullyQualifiedName() : "UnknownClass"; - String fieldName = variable.getName().getSimpleName(); - String fullyQualifiedName = className + "." + fieldName; - - boolean isPrivate = variableDecls.getModifiers().stream() - .anyMatch(mod -> mod.getType() == J.Modifier.Type.Private); - boolean isStatic = variableDecls.getModifiers().stream() - .anyMatch(mod -> mod.getType() == J.Modifier.Type.Static); - boolean isFinal = variableDecls.getModifiers().stream() - .anyMatch(mod -> mod.getType() == J.Modifier.Type.Final); - - Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); - - ThreadLocalDeclaration declaration = new ThreadLocalDeclaration( - fullyQualifiedName, className, fieldName, - isPrivate, isStatic, isFinal, sourcePath - ); - - acc.addDeclaration(fullyQualifiedName, declaration); + if (isField) { + J.VariableDeclarations variableDecls = getCursor().firstEnclosing(J.VariableDeclarations.class); + + if (classDecl != null && variableDecls != null) { + String className = classDecl.getType() != null ? classDecl.getType().getFullyQualifiedName() : "UnknownClass"; + String fieldName = variable.getName().getSimpleName(); + String fullyQualifiedName = className + "." + fieldName; + + boolean isPrivate = variableDecls.getModifiers().stream().anyMatch(mod -> mod.getType() == J.Modifier.Type.Private); + boolean isStatic = variableDecls.getModifiers().stream().anyMatch(mod -> mod.getType() == J.Modifier.Type.Static); + boolean isFinal = variableDecls.getModifiers().stream().anyMatch(mod -> mod.getType() == J.Modifier.Type.Final); + + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); + + ThreadLocalDeclaration declaration = new ThreadLocalDeclaration(fullyQualifiedName, className, fieldName, isPrivate, isStatic, isFinal, sourcePath); + + acc.addDeclaration(fullyQualifiedName, declaration); + } + } } + return variable; } - } - return variable; - } - @Override - public J.Assignment visitAssignment(J.Assignment assignment, ExecutionContext ctx) { - assignment = super.visitAssignment(assignment, ctx); + @Override + public J.Assignment visitAssignment(J.Assignment assignment, ExecutionContext ctx) { + assignment = super.visitAssignment(assignment, ctx); - // Check if we're reassigning a ThreadLocal field - if (isThreadLocalFieldAccess(assignment.getVariable())) { - String fullyQualifiedName = getFieldFullyQualifiedName(assignment.getVariable()); - if (fullyQualifiedName != null) { - Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); - J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - J.MethodDeclaration methodDecl = getCursor().firstEnclosing(J.MethodDeclaration.class); + // Check if we're reassigning a ThreadLocal field + if (isThreadLocalFieldAccess(assignment.getVariable())) { + String fullyQualifiedName = getFieldFullyQualifiedName(assignment.getVariable()); + if (fullyQualifiedName != null) { + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + J.MethodDeclaration methodDecl = getCursor().firstEnclosing(J.MethodDeclaration.class); - String className = classDecl != null && classDecl.getType() != null ? - classDecl.getType().getFullyQualifiedName() : "UnknownClass"; - String methodName = methodDecl != null ? methodDecl.getSimpleName() : "UnknownMethod"; + String className = classDecl != null && classDecl.getType() != null ? classDecl.getType().getFullyQualifiedName() : "UnknownClass"; + String methodName = methodDecl != null ? methodDecl.getSimpleName() : "UnknownMethod"; - MutationLocation mutation = new MutationLocation( - sourcePath, className, methodName, MutationLocation.MutationType.REASSIGNMENT - ); - acc.addMutation(fullyQualifiedName, mutation); + MutationLocation mutation = new MutationLocation(sourcePath, className, methodName, MutationLocation.MutationType.REASSIGNMENT); + acc.addMutation(fullyQualifiedName, mutation); + } + } + return assignment; } - } - return assignment; - } - @Override - public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { - method = super.visitMethodInvocation(method, ctx); + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { + method = super.visitMethodInvocation(method, ctx); - // Check for ThreadLocal.set() or ThreadLocal.remove() calls - if (THREAD_LOCAL_SET.matches(method) || THREAD_LOCAL_REMOVE.matches(method) || - INHERITABLE_THREAD_LOCAL_SET.matches(method) || INHERITABLE_THREAD_LOCAL_REMOVE.matches(method)) { + // Check for ThreadLocal.set() or ThreadLocal.remove() calls + if (THREAD_LOCAL_SET.matches(method) || THREAD_LOCAL_REMOVE.matches(method) || INHERITABLE_THREAD_LOCAL_SET.matches(method) || INHERITABLE_THREAD_LOCAL_REMOVE.matches(method)) { - String fullyQualifiedName = getFieldFullyQualifiedName(method.getSelect()); - if (fullyQualifiedName != null) { - Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); - J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - J.MethodDeclaration methodDecl = getCursor().firstEnclosing(J.MethodDeclaration.class); + String fullyQualifiedName = getFieldFullyQualifiedName(method.getSelect()); + if (fullyQualifiedName != null) { + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + J.MethodDeclaration methodDecl = getCursor().firstEnclosing(J.MethodDeclaration.class); - String className = classDecl != null && classDecl.getType() != null ? - classDecl.getType().getFullyQualifiedName() : "UnknownClass"; - String methodName = methodDecl != null ? methodDecl.getSimpleName() : "UnknownMethod"; + String className = classDecl != null && classDecl.getType() != null ? classDecl.getType().getFullyQualifiedName() : "UnknownClass"; + String methodName = methodDecl != null ? methodDecl.getSimpleName() : "UnknownMethod"; - MutationLocation.MutationType mutationType = - (THREAD_LOCAL_SET.matches(method) || INHERITABLE_THREAD_LOCAL_SET.matches(method)) ? - MutationLocation.MutationType.SET_CALL : MutationLocation.MutationType.REMOVE_CALL; + MutationLocation.MutationType mutationType = (THREAD_LOCAL_SET.matches(method) || INHERITABLE_THREAD_LOCAL_SET.matches(method)) ? MutationLocation.MutationType.SET_CALL : MutationLocation.MutationType.REMOVE_CALL; - MutationLocation mutation = new MutationLocation( - sourcePath, className, methodName, mutationType - ); - acc.addMutation(fullyQualifiedName, mutation); + MutationLocation mutation = new MutationLocation(sourcePath, className, methodName, mutationType); + acc.addMutation(fullyQualifiedName, mutation); + } + } else if (method.getSimpleName().equals("get") && isThreadLocalType(method.getType())) { + // Track get() calls for completeness + String fullyQualifiedName = getFieldFullyQualifiedName(method.getSelect()); + if (fullyQualifiedName != null) { + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + J.MethodDeclaration methodDecl = getCursor().firstEnclosing(J.MethodDeclaration.class); + + String className = classDecl != null && classDecl.getType() != null ? classDecl.getType().getFullyQualifiedName() : "UnknownClass"; + String methodName = methodDecl != null ? methodDecl.getSimpleName() : "UnknownMethod"; + + AccessLocation access = new AccessLocation(sourcePath, className, methodName, AccessLocation.AccessType.GET_CALL); + acc.addAccess(fullyQualifiedName, access); + } + } + return method; } - } else if (method.getSimpleName().equals("get") && isThreadLocalType(method.getType())) { - // Track get() calls for completeness - String fullyQualifiedName = getFieldFullyQualifiedName(method.getSelect()); - if (fullyQualifiedName != null) { - Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); - J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - J.MethodDeclaration methodDecl = getCursor().firstEnclosing(J.MethodDeclaration.class); - - String className = classDecl != null && classDecl.getType() != null ? - classDecl.getType().getFullyQualifiedName() : "UnknownClass"; - String methodName = methodDecl != null ? methodDecl.getSimpleName() : "UnknownMethod"; - AccessLocation access = new AccessLocation( - sourcePath, className, methodName, AccessLocation.AccessType.GET_CALL - ); - acc.addAccess(fullyQualifiedName, access); + private boolean isThreadLocalFieldAccess(Expression expression) { + if (expression instanceof J.Identifier) { + J.Identifier id = (J.Identifier) expression; + return isThreadLocalType(id.getType()); + } else if (expression instanceof J.FieldAccess) { + J.FieldAccess fieldAccess = (J.FieldAccess) expression; + return isThreadLocalType(fieldAccess.getType()); + } + return false; } - } - return method; - } - private boolean isThreadLocalFieldAccess(Expression expression) { - if (expression instanceof J.Identifier) { - J.Identifier id = (J.Identifier) expression; - return isThreadLocalType(id.getType()); - } else if (expression instanceof J.FieldAccess) { - J.FieldAccess fieldAccess = (J.FieldAccess) expression; - return isThreadLocalType(fieldAccess.getType()); - } - return false; - } - - private String getFieldFullyQualifiedName(Expression expression) { - if (expression instanceof J.Identifier) { - J.Identifier id = (J.Identifier) expression; - JavaType.Variable varType = id.getFieldType(); - if (varType != null && varType.getOwner() instanceof JavaType.FullyQualified) { - JavaType.FullyQualified owner = (JavaType.FullyQualified) varType.getOwner(); - return owner.getFullyQualifiedName() + "." + varType.getName(); - } - } else if (expression instanceof J.FieldAccess) { - J.FieldAccess fieldAccess = (J.FieldAccess) expression; - JavaType.Variable varType = fieldAccess.getName().getFieldType(); - if (varType != null && varType.getOwner() instanceof JavaType.FullyQualified) { - JavaType.FullyQualified owner = (JavaType.FullyQualified) varType.getOwner(); - return owner.getFullyQualifiedName() + "." + varType.getName(); + private String getFieldFullyQualifiedName(Expression expression) { + if (expression instanceof J.Identifier) { + J.Identifier id = (J.Identifier) expression; + JavaType.Variable varType = id.getFieldType(); + if (varType != null && varType.getOwner() instanceof JavaType.FullyQualified) { + JavaType.FullyQualified owner = (JavaType.FullyQualified) varType.getOwner(); + return owner.getFullyQualifiedName() + "." + varType.getName(); + } + } else if (expression instanceof J.FieldAccess) { + J.FieldAccess fieldAccess = (J.FieldAccess) expression; + JavaType.Variable varType = fieldAccess.getName().getFieldType(); + if (varType != null && varType.getOwner() instanceof JavaType.FullyQualified) { + JavaType.FullyQualified owner = (JavaType.FullyQualified) varType.getOwner(); + return owner.getFullyQualifiedName() + "." + varType.getName(); + } + } + return null; } - } - return null; - } - }; + }); } @Override public TreeVisitor getVisitor(ThreadLocalUsageAccumulator acc) { - return new JavaIsoVisitor() { - @Override - public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { - multiVariable = super.visitVariableDeclarations(multiVariable, ctx); - - // Check if any of the variables in this declaration are immutable ThreadLocals - J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - - for (J.VariableDeclarations.NamedVariable variable : multiVariable.getVariables()) { - if (isThreadLocalType(variable.getType()) && classDecl != null) { - String className = classDecl.getType() != null ? - classDecl.getType().getFullyQualifiedName() : "UnknownClass"; - String fieldName = variable.getName().getSimpleName(); - String fullyQualifiedName = className + "." + fieldName; - - ThreadLocalDeclaration declaration = acc.declarations.get(fullyQualifiedName); - if (declaration != null) { - boolean hasMutations = acc.hasMutations(fullyQualifiedName); - boolean hasExternalMutations = acc.hasExternalMutations(fullyQualifiedName); - - if (!hasMutations) { - // No mutations at all - String message = declaration.isPrivate() ? - "ThreadLocal candidate for ScopedValue migration - never mutated after initialization" : - "ThreadLocal candidate for ScopedValue migration - never mutated in project (but accessible from outside due to non-private access)"; - return SearchResult.found(multiVariable, message); - } else if (!hasExternalMutations && declaration.isPrivate()) { - // Has mutations, but only in the declaring file and it's private - // This is still safe since no external access is possible - // However, we should check if mutations are only in the initializer - List mutations = acc.mutations.get(fullyQualifiedName); - boolean onlyLocalMutations = mutations.stream() - .allMatch(m -> m.getSourcePath().equals(declaration.getSourcePath())); - - if (onlyLocalMutations) { - // Check if it's only mutated in the same class - // For now, we'll be conservative and not mark it - continue; + return check(!acc.declarations.isEmpty(), check( + and(new UsesJavaVersion<>(25), + or(new UsesType<>(THREAD_LOCAL_FQN, true), new UsesType<>(INHERITED_THREAD_LOCAL_FQN, true))), + new JavaIsoVisitor() { + @Override + public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { + multiVariable = super.visitVariableDeclarations(multiVariable, ctx); + + // Check if any of the variables in this declaration are immutable ThreadLocals + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + + for (J.VariableDeclarations.NamedVariable variable : multiVariable.getVariables()) { + if (isThreadLocalType(variable.getType()) && classDecl != null) { + String className = classDecl.getType() != null ? classDecl.getType().getFullyQualifiedName() : "UnknownClass"; + String fieldName = variable.getName().getSimpleName(); + String fullyQualifiedName = className + "." + fieldName; + + ThreadLocalDeclaration declaration = acc.declarations.get(fullyQualifiedName); + if (declaration != null) { + boolean hasMutations = acc.hasMutations(fullyQualifiedName); + boolean hasExternalMutations = acc.hasExternalMutations(fullyQualifiedName); + + if (!hasMutations) { + // No mutations at all + String message = declaration.isPrivate() ? "ThreadLocal candidate for ScopedValue migration - never mutated after initialization" : "ThreadLocal candidate for ScopedValue migration - never mutated in project (but accessible from outside due to non-private access)"; + return SearchResult.found(multiVariable, message); + } else if (!hasExternalMutations && declaration.isPrivate()) { + // Has mutations, but only in the declaring file and it's private + // This is still safe since no external access is possible + // However, we should check if mutations are only in the initializer + List mutations = acc.mutations.get(fullyQualifiedName); + boolean onlyLocalMutations = mutations.stream().allMatch(m -> m.getSourcePath().equals(declaration.getSourcePath())); + + if (onlyLocalMutations) { + // Check if it's only mutated in the same class + // For now, we'll be conservative and not mark it + continue; + } + } } } } - } - } - return multiVariable; - } - }; + return multiVariable; + } + })); } private static boolean isThreadLocalType(JavaType type) { - return TypeUtils.isOfClassType(type, "java.lang.ThreadLocal") || - TypeUtils.isOfClassType(type, "java.lang.InheritableThreadLocal"); + return TypeUtils.isOfClassType(type, THREAD_LOCAL_FQN) || TypeUtils.isOfClassType(type, INHERITED_THREAD_LOCAL_FQN); } -} \ No newline at end of file +} diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesCrossFileTest.java b/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesCrossFileTest.java index 61dec9d617..52f7465778 100644 --- a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesCrossFileTest.java +++ b/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesCrossFileTest.java @@ -416,4 +416,4 @@ public void mutate(InstanceThreadLocalHolder holder) { ); // Instance ThreadLocal should NOT be marked as immutable due to external mutation } -} \ No newline at end of file +} diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java b/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java index fc08a334b3..821ce41a6e 100644 --- a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java +++ b/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java @@ -344,4 +344,4 @@ public String getValue() { ) ); } -} \ No newline at end of file +} From a3bd2a9ac1f53906bbe8c2fb2799489836e14eb7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Thu, 16 Oct 2025 23:45:22 +0200 Subject: [PATCH 06/10] replace recipe with 3 independent recipes --- .../search/AbstractFindThreadLocals.java | 356 ++++++++++++++++++ .../FindImmutableThreadLocalVariables.java | 344 ----------------- .../search/FindNeverMutatedThreadLocals.java | 50 +++ .../FindThreadLocalsMutatableFromOutside.java | 62 +++ ...hreadLocalsMutatedOnlyInDefiningScope.java | 60 +++ .../java/migrate/search/ThreadLocalTable.java | 61 +++ ...everMutatedThreadLocalsCrossFileTest.java} | 12 +- ... => FindNeverMutatedThreadLocalsTest.java} | 25 +- ...dThreadLocalsMutatableFromOutsideTest.java | 254 +++++++++++++ ...dLocalsMutatedOnlyInDefiningScopeTest.java | 254 +++++++++++++ 10 files changed, 1117 insertions(+), 361 deletions(-) create mode 100644 src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java delete mode 100644 src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java create mode 100644 src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java create mode 100644 src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutside.java create mode 100644 src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java create mode 100644 src/main/java/org/openrewrite/java/migrate/search/ThreadLocalTable.java rename src/test/java/org/openrewrite/java/migrate/search/{FindImmutableThreadLocalVariablesCrossFileTest.java => FindNeverMutatedThreadLocalsCrossFileTest.java} (92%) rename src/test/java/org/openrewrite/java/migrate/search/{FindImmutableThreadLocalVariablesTest.java => FindNeverMutatedThreadLocalsTest.java} (82%) create mode 100644 src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutsideTest.java create mode 100644 src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java diff --git a/src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java b/src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java new file mode 100644 index 0000000000..440d666050 --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java @@ -0,0 +1,356 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.search; + +import lombok.Value; +import org.openrewrite.ExecutionContext; +import org.openrewrite.ScanningRecipe; +import org.openrewrite.SourceFile; +import org.openrewrite.TreeVisitor; +import org.openrewrite.internal.lang.Nullable; +import org.openrewrite.java.JavaIsoVisitor; +import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.search.UsesType; +import org.openrewrite.java.tree.Expression; +import org.openrewrite.java.tree.J; +import org.openrewrite.java.tree.JavaType; +import org.openrewrite.java.tree.TypeUtils; +import org.openrewrite.marker.SearchResult; + +import java.nio.file.Path; +import java.util.*; + +import static org.openrewrite.Preconditions.*; + +public abstract class AbstractFindThreadLocals extends ScanningRecipe { + + protected static final String THREAD_LOCAL_FQN = "java.lang.ThreadLocal"; + protected static final String INHERITED_THREAD_LOCAL_FQN = "java.lang.InheritableThreadLocal"; + + private static final MethodMatcher THREAD_LOCAL_SET = new MethodMatcher(THREAD_LOCAL_FQN + " set(..)"); + private static final MethodMatcher THREAD_LOCAL_REMOVE = new MethodMatcher(THREAD_LOCAL_FQN + " remove()"); + private static final MethodMatcher INHERITABLE_THREAD_LOCAL_SET = new MethodMatcher(INHERITED_THREAD_LOCAL_FQN + " set(..)"); + private static final MethodMatcher INHERITABLE_THREAD_LOCAL_REMOVE = new MethodMatcher(INHERITED_THREAD_LOCAL_FQN + " remove()"); + + @Nullable + protected transient ThreadLocalTable dataTable; + + @Value + public static class ThreadLocalAccumulator { + Map threadLocals = new HashMap<>(); + + public void recordDeclaration(String fqn, Path sourcePath, boolean isPrivate, boolean isStatic, boolean isFinal) { + threadLocals.computeIfAbsent(fqn, k -> new ThreadLocalInfo()) + .setDeclaration(sourcePath, isPrivate, isStatic, isFinal); + } + + public void recordMutation(String fqn, Path sourcePath, boolean isInitContext) { + ThreadLocalInfo info = threadLocals.computeIfAbsent(fqn, k -> new ThreadLocalInfo()); + if (isInitContext) { + info.addInitMutation(sourcePath); + } else { + info.addRegularMutation(sourcePath); + } + } + + public ThreadLocalInfo getInfo(String fqn) { + return threadLocals.get(fqn); + } + + public boolean hasDeclarations() { + return threadLocals.values().stream().anyMatch(ThreadLocalInfo::isDeclared); + } + } + + public static class ThreadLocalInfo { + private Path declarationPath; + private boolean isPrivate; + private boolean isStatic; + private boolean isFinal; + private boolean declared; + private final Set initMutationPaths = new HashSet<>(); + private final Set regularMutationPaths = new HashSet<>(); + + void setDeclaration(Path path, boolean priv, boolean stat, boolean fin) { + this.declarationPath = path; + this.isPrivate = priv; + this.isStatic = stat; + this.isFinal = fin; + this.declared = true; + } + + void addInitMutation(Path path) { + initMutationPaths.add(path); + } + + void addRegularMutation(Path path) { + regularMutationPaths.add(path); + } + + public boolean isPrivate() { + return isPrivate; + } + + public boolean isStatic() { + return isStatic; + } + + public boolean isFinal() { + return isFinal; + } + + public boolean isDeclared() { + return declared; + } + + public boolean hasAnyMutation() { + return !initMutationPaths.isEmpty() || !regularMutationPaths.isEmpty(); + } + + public boolean hasOnlyInitMutations() { + return !initMutationPaths.isEmpty() && regularMutationPaths.isEmpty(); + } + + public boolean hasExternalMutations() { + if (!declared || declarationPath == null) { + return true; // Conservative + } + + // Check if any mutation is from a different file + return initMutationPaths.stream().anyMatch(p -> !p.equals(declarationPath)) || + regularMutationPaths.stream().anyMatch(p -> !p.equals(declarationPath)); + } + + public boolean isOnlyLocallyMutated() { + if (!declared || declarationPath == null) { + return false; + } + + // All mutations must be from the same file as declaration + return initMutationPaths.stream().allMatch(p -> p.equals(declarationPath)) && + regularMutationPaths.stream().allMatch(p -> p.equals(declarationPath)); + } + } + + @Override + public ThreadLocalAccumulator getInitialValue(ExecutionContext ctx) { + return new ThreadLocalAccumulator(); + } + + @Override + public TreeVisitor getScanner(ThreadLocalAccumulator acc) { + return check( + or(new UsesType<>(THREAD_LOCAL_FQN, true), + new UsesType<>(INHERITED_THREAD_LOCAL_FQN, true)), + new JavaIsoVisitor() { + @Override + public J.VariableDeclarations.NamedVariable visitVariable( + J.VariableDeclarations.NamedVariable variable, ExecutionContext ctx) { + variable = super.visitVariable(variable, ctx); + + if (isThreadLocalType(variable.getType())) { + // Only track fields, not local variables + J.MethodDeclaration enclosingMethod = getCursor().firstEnclosing(J.MethodDeclaration.class); + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + + if (classDecl != null && enclosingMethod == null) { + // It's a field + J.VariableDeclarations variableDecls = getCursor().firstEnclosing(J.VariableDeclarations.class); + if (variableDecls != null) { + String className = classDecl.getType() != null ? + classDecl.getType().getFullyQualifiedName() : "UnknownClass"; + String fqn = className + "." + variable.getName().getSimpleName(); + + boolean isPrivate = hasModifier(variableDecls.getModifiers(), J.Modifier.Type.Private); + boolean isStatic = hasModifier(variableDecls.getModifiers(), J.Modifier.Type.Static); + boolean isFinal = hasModifier(variableDecls.getModifiers(), J.Modifier.Type.Final); + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); + + acc.recordDeclaration(fqn, sourcePath, isPrivate, isStatic, isFinal); + } + } + } + return variable; + } + + @Override + public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { + method = super.visitMethodInvocation(method, ctx); + + // Check for ThreadLocal.set() or ThreadLocal.remove() calls + if (THREAD_LOCAL_SET.matches(method) || THREAD_LOCAL_REMOVE.matches(method) || + INHERITABLE_THREAD_LOCAL_SET.matches(method) || INHERITABLE_THREAD_LOCAL_REMOVE.matches(method)) { + + String fqn = getFieldFullyQualifiedName(method.getSelect()); + if (fqn != null) { + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); + boolean isInitContext = isInInitializationContext(); + acc.recordMutation(fqn, sourcePath, isInitContext); + } + } + return method; + } + + @Override + public J.Assignment visitAssignment(J.Assignment assignment, ExecutionContext ctx) { + assignment = super.visitAssignment(assignment, ctx); + + // Check if we're reassigning a ThreadLocal field + if (isThreadLocalFieldAccess(assignment.getVariable())) { + String fqn = getFieldFullyQualifiedName(assignment.getVariable()); + if (fqn != null) { + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); + boolean isInitContext = isInInitializationContext(); + acc.recordMutation(fqn, sourcePath, isInitContext); + } + } + return assignment; + } + + private boolean isInInitializationContext() { + J.MethodDeclaration methodDecl = getCursor().firstEnclosing(J.MethodDeclaration.class); + + if (methodDecl == null) { + // Check if we're in a static initializer block + return getCursor().getPathAsStream() + .filter(J.Block.class::isInstance) + .map(J.Block.class::cast) + .anyMatch(J.Block::isStatic); + } + + // Check if it's a constructor + return methodDecl.isConstructor(); + } + + private boolean isThreadLocalFieldAccess(Expression expression) { + if (expression instanceof J.Identifier) { + return isThreadLocalType(((J.Identifier) expression).getType()); + } else if (expression instanceof J.FieldAccess) { + return isThreadLocalType(((J.FieldAccess) expression).getType()); + } + return false; + } + + private String getFieldFullyQualifiedName(Expression expression) { + JavaType.Variable varType = null; + + if (expression instanceof J.Identifier) { + varType = ((J.Identifier) expression).getFieldType(); + } else if (expression instanceof J.FieldAccess) { + varType = ((J.FieldAccess) expression).getName().getFieldType(); + } + + if (varType != null && varType.getOwner() instanceof JavaType.FullyQualified) { + JavaType.FullyQualified owner = (JavaType.FullyQualified) varType.getOwner(); + return owner.getFullyQualifiedName() + "." + varType.getName(); + } + return null; + } + + private boolean hasModifier(List modifiers, J.Modifier.Type type) { + return modifiers.stream().anyMatch(m -> m.getType() == type); + } + }); + } + + @Override + public TreeVisitor getVisitor(ThreadLocalAccumulator acc) { + return check(acc.hasDeclarations(), + new JavaIsoVisitor() { + @Override + public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionContext ctx) { + if (dataTable == null) { + dataTable = new ThreadLocalTable(AbstractFindThreadLocals.this); + } + return super.visitCompilationUnit(cu, ctx); + } + + @Override + public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { + multiVariable = super.visitVariableDeclarations(multiVariable, ctx); + + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + + for (J.VariableDeclarations.NamedVariable variable : multiVariable.getVariables()) { + if (isThreadLocalType(variable.getType()) && classDecl != null) { + String className = classDecl.getType() != null ? + classDecl.getType().getFullyQualifiedName() : "UnknownClass"; + String fieldName = variable.getName().getSimpleName(); + String fqn = className + "." + fieldName; + + ThreadLocalInfo info = acc.getInfo(fqn); + if (info != null && shouldMarkThreadLocal(info)) { + String message = getMessage(info); + String mutationType = getMutationType(info); + + // Add to data table + if (dataTable != null) { + dataTable.insertRow(ctx, new ThreadLocalTable.Row( + getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath().toString(), + className, + fieldName, + getAccessModifier(multiVariable.getModifiers()), + getModifiers(multiVariable.getModifiers()), + mutationType, + message + )); + } + + return SearchResult.found(multiVariable, message); + } + } + } + + return multiVariable; + } + + private String getAccessModifier(List modifiers) { + if (hasModifier(modifiers, J.Modifier.Type.Private)) { + return "private"; + } else if (hasModifier(modifiers, J.Modifier.Type.Protected)) { + return "protected"; + } else if (hasModifier(modifiers, J.Modifier.Type.Public)) { + return "public"; + } + return "package-private"; + } + + private String getModifiers(List modifiers) { + List mods = new ArrayList<>(); + if (hasModifier(modifiers, J.Modifier.Type.Static)) { + mods.add("static"); + } + if (hasModifier(modifiers, J.Modifier.Type.Final)) { + mods.add("final"); + } + return String.join(" ", mods); + } + + private boolean hasModifier(List modifiers, J.Modifier.Type type) { + return modifiers.stream().anyMatch(m -> m.getType() == type); + } + }); + } + + protected abstract boolean shouldMarkThreadLocal(ThreadLocalInfo info); + protected abstract String getMessage(ThreadLocalInfo info); + protected abstract String getMutationType(ThreadLocalInfo info); + + protected static boolean isThreadLocalType(JavaType type) { + return TypeUtils.isOfClassType(type, THREAD_LOCAL_FQN) || + TypeUtils.isOfClassType(type, INHERITED_THREAD_LOCAL_FQN); + } +} \ No newline at end of file diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java b/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java deleted file mode 100644 index 2adbfcfed5..0000000000 --- a/src/main/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariables.java +++ /dev/null @@ -1,344 +0,0 @@ -/* - * Copyright 2024 the original author or authors. - *

- * Licensed under the Moderne Source Available License (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - *

- * https://docs.moderne.io/licensing/moderne-source-available-license - *

- * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.openrewrite.java.migrate.search; - -import lombok.EqualsAndHashCode; -import lombok.Value; -import org.openrewrite.ExecutionContext; -import org.openrewrite.ScanningRecipe; -import org.openrewrite.SourceFile; -import org.openrewrite.TreeVisitor; -import org.openrewrite.java.JavaIsoVisitor; -import org.openrewrite.java.MethodMatcher; -import org.openrewrite.java.search.UsesJavaVersion; -import org.openrewrite.java.search.UsesType; -import org.openrewrite.java.tree.Expression; -import org.openrewrite.java.tree.J; -import org.openrewrite.java.tree.JavaType; -import org.openrewrite.java.tree.TypeUtils; -import org.openrewrite.marker.SearchResult; - -import java.nio.file.Path; -import java.util.ArrayList; -import java.util.HashMap; -import java.util.List; -import java.util.Map; - -import static org.openrewrite.Preconditions.*; - -@EqualsAndHashCode(callSuper = false) -public class FindImmutableThreadLocalVariables extends ScanningRecipe { - - private static final String THREAD_LOCAL_FQN = "java.lang.ThreadLocal"; - private static final String INHERITED_THREAD_LOCAL_FQN = "java.lang.InheritableThreadLocal"; - private static final MethodMatcher THREAD_LOCAL_SET = new MethodMatcher(THREAD_LOCAL_FQN + " set(..)"); - private static final MethodMatcher THREAD_LOCAL_REMOVE = new MethodMatcher(THREAD_LOCAL_FQN + " remove()"); - private static final MethodMatcher INHERITABLE_THREAD_LOCAL_SET = new MethodMatcher(INHERITED_THREAD_LOCAL_FQN + " set(..)"); - private static final MethodMatcher INHERITABLE_THREAD_LOCAL_REMOVE = new MethodMatcher(INHERITED_THREAD_LOCAL_FQN + " remove()"); - - @Override - public String getDisplayName() { - return "Find immutable ThreadLocal variables"; - } - - @Override - public String getDescription() { - return "Find `ThreadLocal` variables that are never mutated and could be candidates for migration to `ScopedValue` in Java 25+. " + "This recipe identifies `ThreadLocal` variables that are only initialized but never reassigned or modified through `set()` or `remove()` methods. " + "The recipe analyzes mutations across all source files in the project to provide accurate results."; - } - - @Value - static class ThreadLocalUsageAccumulator { - // Map of ThreadLocal declarations: fullyQualifiedName -> declaration info - Map declarations = new HashMap<>(); - // Map of ThreadLocal mutations: fullyQualifiedName -> list of mutation locations - Map> mutations = new HashMap<>(); - // Map of ThreadLocal accesses: fullyQualifiedName -> list of access locations - Map> accesses = new HashMap<>(); - - void addDeclaration(String fullyQualifiedName, ThreadLocalDeclaration declaration) { - declarations.put(fullyQualifiedName, declaration); - } - - void addMutation(String fullyQualifiedName, MutationLocation mutation) { - mutations.computeIfAbsent(fullyQualifiedName, k -> new ArrayList<>()).add(mutation); - } - - void addAccess(String fullyQualifiedName, AccessLocation access) { - accesses.computeIfAbsent(fullyQualifiedName, k -> new ArrayList<>()).add(access); - } - - boolean hasExternalMutations(String fullyQualifiedName) { - List mutationList = mutations.get(fullyQualifiedName); - if (mutationList == null || mutationList.isEmpty()) { - return false; - } - - ThreadLocalDeclaration declaration = declarations.get(fullyQualifiedName); - if (declaration == null) { - return true; // Conservative: if we can't find the declaration, assume it can be mutated - } - - // Check if any mutation is from a different file than the declaration - for (MutationLocation mutation : mutationList) { - if (!mutation.getSourcePath().equals(declaration.getSourcePath())) { - return true; - } - } - return false; - } - - boolean hasMutations(String fullyQualifiedName) { - return mutations.containsKey(fullyQualifiedName) && !mutations.get(fullyQualifiedName).isEmpty(); - } - } - - @Value - static class ThreadLocalDeclaration { - String fullyQualifiedName; // e.g., "com.example.MyClass.MY_THREAD_LOCAL" - String className; - String fieldName; - boolean isPrivate; - boolean isStatic; - boolean isFinal; - Path sourcePath; - } - - @Value - static class MutationLocation { - Path sourcePath; - String className; - String methodName; - MutationType type; - - enum MutationType { - REASSIGNMENT, SET_CALL, REMOVE_CALL, OTHER - } - } - - @Value - static class AccessLocation { - Path sourcePath; - String className; - String methodName; - AccessType type; - - enum AccessType { - GET_CALL, FIELD_READ, OTHER - } - } - - @Override - public ThreadLocalUsageAccumulator getInitialValue(ExecutionContext ctx) { - return new ThreadLocalUsageAccumulator(); - } - - @Override - public TreeVisitor getScanner(ThreadLocalUsageAccumulator acc) { - return check( - and(new UsesJavaVersion<>(25), - or(new UsesType<>(THREAD_LOCAL_FQN, true), new UsesType<>(INHERITED_THREAD_LOCAL_FQN, true))), - new JavaIsoVisitor() { - @Override - public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionContext ctx) { - // Store the current compilation unit's path for tracking locations - getCursor().putMessage("currentSourcePath", cu.getSourcePath()); - return super.visitCompilationUnit(cu, ctx); - } - - @Override - public J.VariableDeclarations.NamedVariable visitVariable(J.VariableDeclarations.NamedVariable variable, ExecutionContext ctx) { - variable = super.visitVariable(variable, ctx); - - if (isThreadLocalType(variable.getType())) { - // Check if this is a field (not a local variable) - // A field is declared directly in a class, not inside a method - J.MethodDeclaration enclosingMethod = getCursor().firstEnclosing(J.MethodDeclaration.class); - J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - - // It's a field if there's no enclosing method, or if the variable is declared - // directly in the class body (not inside a method body) - boolean isField = classDecl != null && (enclosingMethod == null || getCursor().getPathAsStream().filter(J.Block.class::isInstance).map(J.Block.class::cast).noneMatch(block -> block == enclosingMethod.getBody())); - - if (isField) { - J.VariableDeclarations variableDecls = getCursor().firstEnclosing(J.VariableDeclarations.class); - - if (classDecl != null && variableDecls != null) { - String className = classDecl.getType() != null ? classDecl.getType().getFullyQualifiedName() : "UnknownClass"; - String fieldName = variable.getName().getSimpleName(); - String fullyQualifiedName = className + "." + fieldName; - - boolean isPrivate = variableDecls.getModifiers().stream().anyMatch(mod -> mod.getType() == J.Modifier.Type.Private); - boolean isStatic = variableDecls.getModifiers().stream().anyMatch(mod -> mod.getType() == J.Modifier.Type.Static); - boolean isFinal = variableDecls.getModifiers().stream().anyMatch(mod -> mod.getType() == J.Modifier.Type.Final); - - Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); - - ThreadLocalDeclaration declaration = new ThreadLocalDeclaration(fullyQualifiedName, className, fieldName, isPrivate, isStatic, isFinal, sourcePath); - - acc.addDeclaration(fullyQualifiedName, declaration); - } - } - } - return variable; - } - - @Override - public J.Assignment visitAssignment(J.Assignment assignment, ExecutionContext ctx) { - assignment = super.visitAssignment(assignment, ctx); - - // Check if we're reassigning a ThreadLocal field - if (isThreadLocalFieldAccess(assignment.getVariable())) { - String fullyQualifiedName = getFieldFullyQualifiedName(assignment.getVariable()); - if (fullyQualifiedName != null) { - Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); - J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - J.MethodDeclaration methodDecl = getCursor().firstEnclosing(J.MethodDeclaration.class); - - String className = classDecl != null && classDecl.getType() != null ? classDecl.getType().getFullyQualifiedName() : "UnknownClass"; - String methodName = methodDecl != null ? methodDecl.getSimpleName() : "UnknownMethod"; - - MutationLocation mutation = new MutationLocation(sourcePath, className, methodName, MutationLocation.MutationType.REASSIGNMENT); - acc.addMutation(fullyQualifiedName, mutation); - } - } - return assignment; - } - - @Override - public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { - method = super.visitMethodInvocation(method, ctx); - - // Check for ThreadLocal.set() or ThreadLocal.remove() calls - if (THREAD_LOCAL_SET.matches(method) || THREAD_LOCAL_REMOVE.matches(method) || INHERITABLE_THREAD_LOCAL_SET.matches(method) || INHERITABLE_THREAD_LOCAL_REMOVE.matches(method)) { - - String fullyQualifiedName = getFieldFullyQualifiedName(method.getSelect()); - if (fullyQualifiedName != null) { - Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); - J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - J.MethodDeclaration methodDecl = getCursor().firstEnclosing(J.MethodDeclaration.class); - - String className = classDecl != null && classDecl.getType() != null ? classDecl.getType().getFullyQualifiedName() : "UnknownClass"; - String methodName = methodDecl != null ? methodDecl.getSimpleName() : "UnknownMethod"; - - MutationLocation.MutationType mutationType = (THREAD_LOCAL_SET.matches(method) || INHERITABLE_THREAD_LOCAL_SET.matches(method)) ? MutationLocation.MutationType.SET_CALL : MutationLocation.MutationType.REMOVE_CALL; - - MutationLocation mutation = new MutationLocation(sourcePath, className, methodName, mutationType); - acc.addMutation(fullyQualifiedName, mutation); - } - } else if (method.getSimpleName().equals("get") && isThreadLocalType(method.getType())) { - // Track get() calls for completeness - String fullyQualifiedName = getFieldFullyQualifiedName(method.getSelect()); - if (fullyQualifiedName != null) { - Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); - J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - J.MethodDeclaration methodDecl = getCursor().firstEnclosing(J.MethodDeclaration.class); - - String className = classDecl != null && classDecl.getType() != null ? classDecl.getType().getFullyQualifiedName() : "UnknownClass"; - String methodName = methodDecl != null ? methodDecl.getSimpleName() : "UnknownMethod"; - - AccessLocation access = new AccessLocation(sourcePath, className, methodName, AccessLocation.AccessType.GET_CALL); - acc.addAccess(fullyQualifiedName, access); - } - } - return method; - } - - private boolean isThreadLocalFieldAccess(Expression expression) { - if (expression instanceof J.Identifier) { - J.Identifier id = (J.Identifier) expression; - return isThreadLocalType(id.getType()); - } else if (expression instanceof J.FieldAccess) { - J.FieldAccess fieldAccess = (J.FieldAccess) expression; - return isThreadLocalType(fieldAccess.getType()); - } - return false; - } - - private String getFieldFullyQualifiedName(Expression expression) { - if (expression instanceof J.Identifier) { - J.Identifier id = (J.Identifier) expression; - JavaType.Variable varType = id.getFieldType(); - if (varType != null && varType.getOwner() instanceof JavaType.FullyQualified) { - JavaType.FullyQualified owner = (JavaType.FullyQualified) varType.getOwner(); - return owner.getFullyQualifiedName() + "." + varType.getName(); - } - } else if (expression instanceof J.FieldAccess) { - J.FieldAccess fieldAccess = (J.FieldAccess) expression; - JavaType.Variable varType = fieldAccess.getName().getFieldType(); - if (varType != null && varType.getOwner() instanceof JavaType.FullyQualified) { - JavaType.FullyQualified owner = (JavaType.FullyQualified) varType.getOwner(); - return owner.getFullyQualifiedName() + "." + varType.getName(); - } - } - return null; - } - }); - } - - @Override - public TreeVisitor getVisitor(ThreadLocalUsageAccumulator acc) { - return check(!acc.declarations.isEmpty(), check( - and(new UsesJavaVersion<>(25), - or(new UsesType<>(THREAD_LOCAL_FQN, true), new UsesType<>(INHERITED_THREAD_LOCAL_FQN, true))), - new JavaIsoVisitor() { - @Override - public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { - multiVariable = super.visitVariableDeclarations(multiVariable, ctx); - - // Check if any of the variables in this declaration are immutable ThreadLocals - J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - - for (J.VariableDeclarations.NamedVariable variable : multiVariable.getVariables()) { - if (isThreadLocalType(variable.getType()) && classDecl != null) { - String className = classDecl.getType() != null ? classDecl.getType().getFullyQualifiedName() : "UnknownClass"; - String fieldName = variable.getName().getSimpleName(); - String fullyQualifiedName = className + "." + fieldName; - - ThreadLocalDeclaration declaration = acc.declarations.get(fullyQualifiedName); - if (declaration != null) { - boolean hasMutations = acc.hasMutations(fullyQualifiedName); - boolean hasExternalMutations = acc.hasExternalMutations(fullyQualifiedName); - - if (!hasMutations) { - // No mutations at all - String message = declaration.isPrivate() ? "ThreadLocal candidate for ScopedValue migration - never mutated after initialization" : "ThreadLocal candidate for ScopedValue migration - never mutated in project (but accessible from outside due to non-private access)"; - return SearchResult.found(multiVariable, message); - } else if (!hasExternalMutations && declaration.isPrivate()) { - // Has mutations, but only in the declaring file and it's private - // This is still safe since no external access is possible - // However, we should check if mutations are only in the initializer - List mutations = acc.mutations.get(fullyQualifiedName); - boolean onlyLocalMutations = mutations.stream().allMatch(m -> m.getSourcePath().equals(declaration.getSourcePath())); - - if (onlyLocalMutations) { - // Check if it's only mutated in the same class - // For now, we'll be conservative and not mark it - continue; - } - } - } - } - } - - return multiVariable; - } - })); - } - - private static boolean isThreadLocalType(JavaType type) { - return TypeUtils.isOfClassType(type, THREAD_LOCAL_FQN) || TypeUtils.isOfClassType(type, INHERITED_THREAD_LOCAL_FQN); - } -} diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java b/src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java new file mode 100644 index 0000000000..414e4caa84 --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java @@ -0,0 +1,50 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.search; + +import lombok.EqualsAndHashCode; + +@EqualsAndHashCode(callSuper = false) +public class FindNeverMutatedThreadLocals extends AbstractFindThreadLocals { + + @Override + public String getDisplayName() { + return "Find ThreadLocal variables that are never mutated"; + } + + @Override + public String getDescription() { + return "Find `ThreadLocal` variables that are never mutated after initialization. " + + "These are prime candidates for migration to `ScopedValue` in Java 25+ as they are effectively immutable. " + + "The recipe identifies `ThreadLocal` variables that are only initialized but never reassigned or modified through `set()` or `remove()` methods."; + } + + @Override + protected boolean shouldMarkThreadLocal(ThreadLocalInfo info) { + // Mark ThreadLocals that have no mutations at all + return !info.hasAnyMutation(); + } + + @Override + protected String getMessage(ThreadLocalInfo info) { + return "ThreadLocal is never mutated and could be replaced with ScopedValue"; + } + + @Override + protected String getMutationType(ThreadLocalInfo info) { + return "Never mutated"; + } +} \ No newline at end of file diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutside.java b/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutside.java new file mode 100644 index 0000000000..b91cb97784 --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutside.java @@ -0,0 +1,62 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.search; + +import lombok.EqualsAndHashCode; + +@EqualsAndHashCode(callSuper = false) +public class FindThreadLocalsMutatableFromOutside extends AbstractFindThreadLocals { + + @Override + public String getDisplayName() { + return "Find ThreadLocal variables mutatable from outside their defining class"; + } + + @Override + public String getDescription() { + return "Find `ThreadLocal` variables that can be mutated from outside their defining class. " + + "These ThreadLocals have the highest risk as they can be modified by any code with access to them. " + + "This includes non-private ThreadLocals or those mutated from other classes in the codebase."; + } + + @Override + protected boolean shouldMarkThreadLocal(ThreadLocalInfo info) { + // Mark ThreadLocals that are either: + // 1. Actually mutated from outside their defining class + // 2. Non-private (and thus potentially mutable from outside) + return info.hasExternalMutations() || !info.isPrivate(); + } + + @Override + protected String getMessage(ThreadLocalInfo info) { + if (info.hasExternalMutations()) { + return "ThreadLocal is mutated from outside its defining class"; + } else { + // Non-private but not currently mutated externally + String access = info.isStatic() ? "static " : ""; + return "ThreadLocal is " + access + "non-private and can potentially be mutated from outside"; + } + } + + @Override + protected String getMutationType(ThreadLocalInfo info) { + if (info.hasExternalMutations()) { + return "Mutated externally"; + } else { + return "Potentially mutable"; + } + } +} \ No newline at end of file diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java b/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java new file mode 100644 index 0000000000..9934d6993e --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java @@ -0,0 +1,60 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.search; + +import lombok.EqualsAndHashCode; + +@EqualsAndHashCode(callSuper = false) +public class FindThreadLocalsMutatedOnlyInDefiningScope extends AbstractFindThreadLocals { + + @Override + public String getDisplayName() { + return "Find ThreadLocal variables mutated only in their defining scope"; + } + + @Override + public String getDescription() { + return "Find `ThreadLocal` variables that are only mutated within their defining class or initialization context (constructor/static initializer). " + + "These may be candidates for refactoring as they have limited mutation scope. " + + "The recipe identifies `ThreadLocal` variables that are only modified during initialization or within their declaring class."; + } + + @Override + protected boolean shouldMarkThreadLocal(ThreadLocalInfo info) { + // Mark private ThreadLocals that ARE mutated, but only locally + return info.hasAnyMutation() && + info.isPrivate() && + info.isOnlyLocallyMutated(); + } + + @Override + protected String getMessage(ThreadLocalInfo info) { + if (info.hasOnlyInitMutations()) { + return "ThreadLocal is only mutated during initialization (constructor/static initializer)"; + } else { + return "ThreadLocal is only mutated within its defining class"; + } + } + + @Override + protected String getMutationType(ThreadLocalInfo info) { + if (info.hasOnlyInitMutations()) { + return "Mutated only in initialization"; + } else { + return "Mutated in defining class"; + } + } +} \ No newline at end of file diff --git a/src/main/java/org/openrewrite/java/migrate/search/ThreadLocalTable.java b/src/main/java/org/openrewrite/java/migrate/search/ThreadLocalTable.java new file mode 100644 index 0000000000..f968f84b5a --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/search/ThreadLocalTable.java @@ -0,0 +1,61 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.search; + +import lombok.Value; +import org.openrewrite.Column; +import org.openrewrite.DataTable; +import org.openrewrite.Recipe; + +public class ThreadLocalTable extends DataTable { + + public ThreadLocalTable(Recipe recipe) { + super(recipe, + "ThreadLocal usage", + "ThreadLocal variables and their mutation patterns."); + } + + @Value + public static class Row { + @Column(displayName = "Source file", + description = "The source file containing the ThreadLocal declaration.") + String sourceFile; + + @Column(displayName = "Class name", + description = "The fully qualified class name where the ThreadLocal is declared.") + String className; + + @Column(displayName = "Field name", + description = "The name of the ThreadLocal field.") + String fieldName; + + @Column(displayName = "Access modifier", + description = "The access modifier of the ThreadLocal field (private, protected, public, package-private).") + String accessModifier; + + @Column(displayName = "Field modifiers", + description = "Additional modifiers like static, final.") + String modifiers; + + @Column(displayName = "Mutation type", + description = "Type of mutation detected (Never mutated, Mutated only in initialization, Mutated in defining class, Mutated externally, Potentially mutable).") + String mutationType; + + @Column(displayName = "Message", + description = "Detailed message about the ThreadLocal's usage pattern.") + String message; + } +} \ No newline at end of file diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesCrossFileTest.java b/src/test/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocalsCrossFileTest.java similarity index 92% rename from src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesCrossFileTest.java rename to src/test/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocalsCrossFileTest.java index 52f7465778..1984076117 100644 --- a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesCrossFileTest.java +++ b/src/test/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocalsCrossFileTest.java @@ -22,11 +22,11 @@ import static org.openrewrite.java.Assertions.java; -class FindImmutableThreadLocalVariablesCrossFileTest implements RewriteTest { +class FindNeverMutatedThreadLocalsCrossFileTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { - spec.recipe(new FindImmutableThreadLocalVariables()); + spec.recipe(new FindNeverMutatedThreadLocals()); } @DocumentExample @@ -86,7 +86,7 @@ public int getCount() { package com.example; public class ReadOnlyHolder { - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated in project (but accessible from outside due to non-private access))~~>*/public static final ThreadLocal COUNTER = ThreadLocal.withInitial(() -> 0); + /*~~(ThreadLocal is never mutated and could be replaced with ScopedValue)~~>*/public static final ThreadLocal COUNTER = ThreadLocal.withInitial(() -> 0); public int getCount() { return COUNTER.get(); @@ -180,7 +180,7 @@ public void tryToAccess() { package com.example; public class PrivateHolder { - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal PRIVATE_TL = new ThreadLocal<>(); + /*~~(ThreadLocal is never mutated and could be replaced with ScopedValue)~~>*/private static final ThreadLocal PRIVATE_TL = new ThreadLocal<>(); public String getValue() { return PRIVATE_TL.get(); @@ -291,9 +291,9 @@ public void readAll() { package com.example; public class MultipleThreadLocals { - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal PRIVATE_IMMUTABLE = new ThreadLocal<>(); + /*~~(ThreadLocal is never mutated and could be replaced with ScopedValue)~~>*/private static final ThreadLocal PRIVATE_IMMUTABLE = new ThreadLocal<>(); static final ThreadLocal PACKAGE_MUTATED = new ThreadLocal<>(); - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated in project (but accessible from outside due to non-private access))~~>*/public static final ThreadLocal PUBLIC_READ_ONLY = new ThreadLocal<>(); + /*~~(ThreadLocal is never mutated and could be replaced with ScopedValue)~~>*/public static final ThreadLocal PUBLIC_READ_ONLY = new ThreadLocal<>(); protected static final ThreadLocal PROTECTED_MUTATED = new ThreadLocal<>(); public void readAll() { diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java b/src/test/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocalsTest.java similarity index 82% rename from src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java rename to src/test/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocalsTest.java index 821ce41a6e..a33f49f7bb 100644 --- a/src/test/java/org/openrewrite/java/migrate/search/FindImmutableThreadLocalVariablesTest.java +++ b/src/test/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocalsTest.java @@ -22,11 +22,11 @@ import static org.openrewrite.java.Assertions.java; -class FindImmutableThreadLocalVariablesTest implements RewriteTest { +class FindNeverMutatedThreadLocalsTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { - spec.recipe(new FindImmutableThreadLocalVariables()); + spec.recipe(new FindNeverMutatedThreadLocals()); } @DocumentExample @@ -45,7 +45,7 @@ public String getValue() { """, """ class Example { - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal TL = new ThreadLocal<>(); + /*~~(ThreadLocal is never mutated and could be replaced with ScopedValue)~~>*/private static final ThreadLocal TL = new ThreadLocal<>(); public String getValue() { return TL.get(); @@ -71,7 +71,7 @@ public int getCount() { """, """ class Example { - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal COUNTER = ThreadLocal.withInitial(() -> 0); + /*~~(ThreadLocal is never mutated and could be replaced with ScopedValue)~~>*/private static final ThreadLocal COUNTER = ThreadLocal.withInitial(() -> 0); public int getCount() { return COUNTER.get(); @@ -170,9 +170,9 @@ public Boolean getAnother() { """, """ class Example { - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal IMMUTABLE_TL = new ThreadLocal<>(); + /*~~(ThreadLocal is never mutated and could be replaced with ScopedValue)~~>*/private static final ThreadLocal IMMUTABLE_TL = new ThreadLocal<>(); private static final ThreadLocal MUTABLE_TL = new ThreadLocal<>(); - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal ANOTHER_IMMUTABLE = ThreadLocal.withInitial(() -> false); + /*~~(ThreadLocal is never mutated and could be replaced with ScopedValue)~~>*/private static final ThreadLocal ANOTHER_IMMUTABLE = ThreadLocal.withInitial(() -> false); public void updateMutable(int value) { MUTABLE_TL.set(value); @@ -206,7 +206,7 @@ public String getValue() { """, """ class Example { - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private final ThreadLocal instanceTL = new ThreadLocal<>(); + /*~~(ThreadLocal is never mutated and could be replaced with ScopedValue)~~>*/private final ThreadLocal instanceTL = new ThreadLocal<>(); public String getValue() { return instanceTL.get(); @@ -237,7 +237,7 @@ public String formatDate(java.util.Date date) { import java.text.SimpleDateFormat; class Example { - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final ThreadLocal DATE_FORMAT = + /*~~(ThreadLocal is never mutated and could be replaced with ScopedValue)~~>*/private static final ThreadLocal DATE_FORMAT = ThreadLocal.withInitial(() -> new SimpleDateFormat("yyyy-MM-dd")); public String formatDate(java.util.Date date) { @@ -269,6 +269,9 @@ public void method() { @Test void warnAboutPackagePrivateThreadLocal() { + // Package-private ThreadLocals without mutations are still flagged by FindNeverMutatedThreadLocals + // but should NOT be flagged by FindThreadLocalsMutatableFromOutside if they have no mutations + // For this test, we'll test that it's flagged as never mutated rewriteRun( java( """ @@ -282,7 +285,7 @@ public String getValue() { """, """ class Example { - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated in project (but accessible from outside due to non-private access))~~>*/static final ThreadLocal PACKAGE_TL = new ThreadLocal<>(); + /*~~(ThreadLocal is never mutated and could be replaced with ScopedValue)~~>*/static final ThreadLocal PACKAGE_TL = new ThreadLocal<>(); public String getValue() { return PACKAGE_TL.get(); @@ -308,7 +311,7 @@ public String getValue() { """, """ class Example { - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated in project (but accessible from outside due to non-private access))~~>*/protected static final ThreadLocal PROTECTED_TL = new ThreadLocal<>(); + /*~~(ThreadLocal is never mutated and could be replaced with ScopedValue)~~>*/protected static final ThreadLocal PROTECTED_TL = new ThreadLocal<>(); public String getValue() { return PROTECTED_TL.get(); @@ -334,7 +337,7 @@ public String getValue() { """, """ class Example { - /*~~(ThreadLocal candidate for ScopedValue migration - never mutated after initialization)~~>*/private static final InheritableThreadLocal ITL = new InheritableThreadLocal<>(); + /*~~(ThreadLocal is never mutated and could be replaced with ScopedValue)~~>*/private static final InheritableThreadLocal ITL = new InheritableThreadLocal<>(); public String getValue() { return ITL.get(); diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutsideTest.java b/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutsideTest.java new file mode 100644 index 0000000000..5f02aaa4cc --- /dev/null +++ b/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutsideTest.java @@ -0,0 +1,254 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.search; + +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; + +class FindThreadLocalsMutatableFromOutsideTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new FindThreadLocalsMutatableFromOutside()); + } + + @DocumentExample + @Test + void identifyNonPrivateThreadLocal() { + rewriteRun( + java( + """ + class Example { + public static final ThreadLocal PUBLIC_TL = new ThreadLocal<>(); + + public String getValue() { + return PUBLIC_TL.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal is static non-private and can potentially be mutated from outside)~~>*/public static final ThreadLocal PUBLIC_TL = new ThreadLocal<>(); + + public String getValue() { + return PUBLIC_TL.get(); + } + } + """ + ) + ); + } + + @Test + void identifyPackagePrivateThreadLocal() { + rewriteRun( + java( + """ + class Example { + static final ThreadLocal PACKAGE_TL = new ThreadLocal<>(); + + public String getValue() { + return PACKAGE_TL.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal is static non-private and can potentially be mutated from outside)~~>*/static final ThreadLocal PACKAGE_TL = new ThreadLocal<>(); + + public String getValue() { + return PACKAGE_TL.get(); + } + } + """ + ) + ); + } + + @Test + void identifyProtectedThreadLocal() { + rewriteRun( + java( + """ + class Example { + protected static final ThreadLocal PROTECTED_TL = new ThreadLocal<>(); + + public String getValue() { + return PROTECTED_TL.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal is static non-private and can potentially be mutated from outside)~~>*/protected static final ThreadLocal PROTECTED_TL = new ThreadLocal<>(); + + public String getValue() { + return PROTECTED_TL.get(); + } + } + """ + ) + ); + } + + @Test + void doNotMarkPrivateThreadLocal() { + rewriteRun( + java( + """ + class Example { + private static final ThreadLocal PRIVATE_TL = new ThreadLocal<>(); + + public String getValue() { + return PRIVATE_TL.get(); + } + } + """ + ) + ); + } + + @Test + void identifyThreadLocalMutatedFromOutside() { + rewriteRun( + java( + """ + package com.example; + + public class ThreadLocalHolder { + public static final ThreadLocal SHARED_TL = new ThreadLocal<>(); + + public String getValue() { + return SHARED_TL.get(); + } + } + """, + """ + package com.example; + + public class ThreadLocalHolder { + /*~~(ThreadLocal is mutated from outside its defining class)~~>*/public static final ThreadLocal SHARED_TL = new ThreadLocal<>(); + + public String getValue() { + return SHARED_TL.get(); + } + } + """ + ), + java( + """ + package com.example; + + class ThreadLocalMutator { + public void mutate() { + ThreadLocalHolder.SHARED_TL.set("mutated"); + } + } + """ + ) + ); + } + + @Test + void identifyInstanceThreadLocalNonPrivate() { + rewriteRun( + java( + """ + class Example { + public final ThreadLocal instanceTL = new ThreadLocal<>(); + + public String getValue() { + return instanceTL.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal is non-private and can potentially be mutated from outside)~~>*/public final ThreadLocal instanceTL = new ThreadLocal<>(); + + public String getValue() { + return instanceTL.get(); + } + } + """ + ) + ); + } + + @Test + void doNotMarkPrivateInstanceThreadLocal() { + rewriteRun( + java( + """ + class Example { + private final ThreadLocal instanceTL = new ThreadLocal<>(); + + public String getValue() { + return instanceTL.get(); + } + } + """ + ) + ); + } + + @Test + void identifyProtectedMutatedFromSubclass() { + rewriteRun( + java( + """ + package com.example; + + public class BaseClass { + protected static final ThreadLocal PROTECTED_TL = new ThreadLocal<>(); + + public String getValue() { + return PROTECTED_TL.get(); + } + } + """, + """ + package com.example; + + public class BaseClass { + /*~~(ThreadLocal is mutated from outside its defining class)~~>*/protected static final ThreadLocal PROTECTED_TL = new ThreadLocal<>(); + + public String getValue() { + return PROTECTED_TL.get(); + } + } + """ + ), + java( + """ + package com.example.sub; + + import com.example.BaseClass; + + public class SubClass extends BaseClass { + public void modifyThreadLocal() { + PROTECTED_TL.set("modified by subclass"); + } + } + """ + ) + ); + } +} \ No newline at end of file diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java b/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java new file mode 100644 index 0000000000..095737c460 --- /dev/null +++ b/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java @@ -0,0 +1,254 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.openrewrite.java.migrate.search; + +import org.junit.jupiter.api.Test; +import org.openrewrite.DocumentExample; +import org.openrewrite.test.RecipeSpec; +import org.openrewrite.test.RewriteTest; + +import static org.openrewrite.java.Assertions.java; + +class FindThreadLocalsMutatedOnlyInDefiningScopeTest implements RewriteTest { + + @Override + public void defaults(RecipeSpec spec) { + spec.recipe(new FindThreadLocalsMutatedOnlyInDefiningScope()); + } + + @DocumentExample + @Test + void identifyThreadLocalMutatedOnlyInConstructor() { + rewriteRun( + java( + """ + class Example { + private final ThreadLocal TL = new ThreadLocal<>(); + + public Example() { + TL.set("initial value"); + } + + public String getValue() { + return TL.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal is only mutated during initialization (constructor/static initializer))~~>*/private final ThreadLocal TL = new ThreadLocal<>(); + + public Example() { + TL.set("initial value"); + } + + public String getValue() { + return TL.get(); + } + } + """ + ) + ); + } + + @Test + void identifyThreadLocalMutatedOnlyInStaticInitializer() { + rewriteRun( + java( + """ + class Example { + private static final ThreadLocal TL = new ThreadLocal<>(); + + static { + TL.set("static initial value"); + } + + public String getValue() { + return TL.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal is only mutated during initialization (constructor/static initializer))~~>*/private static final ThreadLocal TL = new ThreadLocal<>(); + + static { + TL.set("static initial value"); + } + + public String getValue() { + return TL.get(); + } + } + """ + ) + ); + } + + @Test + void identifyThreadLocalMutatedInDefiningClass() { + rewriteRun( + java( + """ + class Example { + private static final ThreadLocal TL = new ThreadLocal<>(); + + public void setValue(String value) { + TL.set(value); + } + + public String getValue() { + return TL.get(); + } + + public void cleanup() { + TL.remove(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal is only mutated within its defining class)~~>*/private static final ThreadLocal TL = new ThreadLocal<>(); + + public void setValue(String value) { + TL.set(value); + } + + public String getValue() { + return TL.get(); + } + + public void cleanup() { + TL.remove(); + } + } + """ + ) + ); + } + + @Test + void doNotMarkThreadLocalNeverMutated() { + // This should not be marked by this recipe - it's for FindNeverMutatedThreadLocals + rewriteRun( + java( + """ + class Example { + private static final ThreadLocal TL = new ThreadLocal<>(); + + public String getValue() { + return TL.get(); + } + } + """ + ) + ); + } + + @Test + void doNotMarkNonPrivateThreadLocal() { + // Non-private ThreadLocals shouldn't be marked by this recipe + rewriteRun( + java( + """ + class Example { + static final ThreadLocal TL = new ThreadLocal<>(); + + public void setValue(String value) { + TL.set(value); + } + } + """ + ) + ); + } + + @Test + void identifyInstanceThreadLocalMutatedInConstructor() { + rewriteRun( + java( + """ + class Example { + private final ThreadLocal counter = new ThreadLocal<>(); + + public Example(int initial) { + counter.set(initial); + } + + public Integer getCount() { + return counter.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal is only mutated during initialization (constructor/static initializer))~~>*/private final ThreadLocal counter = new ThreadLocal<>(); + + public Example(int initial) { + counter.set(initial); + } + + public Integer getCount() { + return counter.get(); + } + } + """ + ) + ); + } + + @Test + void identifyThreadLocalWithMixedInitAndClassMutations() { + rewriteRun( + java( + """ + class Example { + private static final ThreadLocal TL = new ThreadLocal<>(); + + static { + TL.set("initial"); + } + + public void update(String value) { + TL.set(value); + } + + public String getValue() { + return TL.get(); + } + } + """, + """ + class Example { + /*~~(ThreadLocal is only mutated within its defining class)~~>*/private static final ThreadLocal TL = new ThreadLocal<>(); + + static { + TL.set("initial"); + } + + public void update(String value) { + TL.set(value); + } + + public String getValue() { + return TL.get(); + } + } + """ + ) + ); + } +} \ No newline at end of file From 01f2e9fc1a216a19bf1c56b9ebe2e6e33f8845da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Fri, 17 Oct 2025 00:09:32 +0200 Subject: [PATCH 07/10] First round of review --- .../search/AbstractFindThreadLocals.java | 147 ++++++++++-------- .../search/FindNeverMutatedThreadLocals.java | 11 ++ ...> FindThreadLocalsMutableFromOutside.java} | 15 +- ...hreadLocalsMutatedOnlyInDefiningScope.java | 41 +++-- ...ndThreadLocalsMutableFromOutsideTest.java} | 4 +- 5 files changed, 137 insertions(+), 81 deletions(-) rename src/main/java/org/openrewrite/java/migrate/search/{FindThreadLocalsMutatableFromOutside.java => FindThreadLocalsMutableFromOutside.java} (83%) rename src/test/java/org/openrewrite/java/migrate/search/{FindThreadLocalsMutatableFromOutsideTest.java => FindThreadLocalsMutableFromOutsideTest.java} (98%) diff --git a/src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java b/src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java index 440d666050..e35acf7675 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java +++ b/src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java @@ -16,11 +16,11 @@ package org.openrewrite.java.migrate.search; import lombok.Value; +import org.jspecify.annotations.Nullable; import org.openrewrite.ExecutionContext; import org.openrewrite.ScanningRecipe; import org.openrewrite.SourceFile; import org.openrewrite.TreeVisitor; -import org.openrewrite.internal.lang.Nullable; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.MethodMatcher; import org.openrewrite.java.search.UsesType; @@ -66,7 +66,7 @@ public void recordMutation(String fqn, Path sourcePath, boolean isInitContext) { } } - public ThreadLocalInfo getInfo(String fqn) { + public @Nullable ThreadLocalInfo getInfo(String fqn) { return threadLocals.get(fqn); } @@ -76,7 +76,7 @@ public boolean hasDeclarations() { } public static class ThreadLocalInfo { - private Path declarationPath; + private @Nullable Path declarationPath; private boolean isPrivate; private boolean isStatic; private boolean isFinal; @@ -161,28 +161,40 @@ public J.VariableDeclarations.NamedVariable visitVariable( J.VariableDeclarations.NamedVariable variable, ExecutionContext ctx) { variable = super.visitVariable(variable, ctx); - if (isThreadLocalType(variable.getType())) { - // Only track fields, not local variables - J.MethodDeclaration enclosingMethod = getCursor().firstEnclosing(J.MethodDeclaration.class); - J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); - - if (classDecl != null && enclosingMethod == null) { - // It's a field - J.VariableDeclarations variableDecls = getCursor().firstEnclosing(J.VariableDeclarations.class); - if (variableDecls != null) { - String className = classDecl.getType() != null ? - classDecl.getType().getFullyQualifiedName() : "UnknownClass"; - String fqn = className + "." + variable.getName().getSimpleName(); - - boolean isPrivate = hasModifier(variableDecls.getModifiers(), J.Modifier.Type.Private); - boolean isStatic = hasModifier(variableDecls.getModifiers(), J.Modifier.Type.Static); - boolean isFinal = hasModifier(variableDecls.getModifiers(), J.Modifier.Type.Final); - Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); - - acc.recordDeclaration(fqn, sourcePath, isPrivate, isStatic, isFinal); - } - } + // Early return for non-ThreadLocal types + if (!isThreadLocalType(variable.getType())) { + return variable; + } + + // Early return for local variables (not fields) + J.MethodDeclaration enclosingMethod = getCursor().firstEnclosing(J.MethodDeclaration.class); + if (enclosingMethod != null) { + return variable; + } + + // Early return if not in a class + J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + if (classDecl == null) { + return variable; } + + // Early return if we can't find the variable declarations + J.VariableDeclarations variableDecls = getCursor().firstEnclosing(J.VariableDeclarations.class); + if (variableDecls == null) { + return variable; + } + + // Process ThreadLocal field declaration + JavaType.@Nullable FullyQualified classType = classDecl.getType(); + String className = classType != null ? classType.getFullyQualifiedName() : "UnknownClass"; + String fqn = className + "." + variable.getName().getSimpleName(); + + boolean isPrivate = variableDecls.hasModifier(J.Modifier.Type.Private); + boolean isStatic = variableDecls.hasModifier(J.Modifier.Type.Static); + boolean isFinal = variableDecls.hasModifier(J.Modifier.Type.Final); + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); + + acc.recordDeclaration(fqn, sourcePath, isPrivate, isStatic, isFinal); return variable; } @@ -190,17 +202,20 @@ public J.VariableDeclarations.NamedVariable visitVariable( public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, ExecutionContext ctx) { method = super.visitMethodInvocation(method, ctx); - // Check for ThreadLocal.set() or ThreadLocal.remove() calls - if (THREAD_LOCAL_SET.matches(method) || THREAD_LOCAL_REMOVE.matches(method) || - INHERITABLE_THREAD_LOCAL_SET.matches(method) || INHERITABLE_THREAD_LOCAL_REMOVE.matches(method)) { + // Early return if not a ThreadLocal mutation method + if (!THREAD_LOCAL_SET.matches(method) && !THREAD_LOCAL_REMOVE.matches(method) && + !INHERITABLE_THREAD_LOCAL_SET.matches(method) && !INHERITABLE_THREAD_LOCAL_REMOVE.matches(method)) { + return method; + } - String fqn = getFieldFullyQualifiedName(method.getSelect()); - if (fqn != null) { - Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); - boolean isInitContext = isInInitializationContext(); - acc.recordMutation(fqn, sourcePath, isInitContext); - } + @Nullable String fqn = getFieldFullyQualifiedName(method.getSelect()); + if (fqn == null) { + return method; } + + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); + boolean isInitContext = isInInitializationContext(); + acc.recordMutation(fqn, sourcePath, isInitContext); return method; } @@ -208,15 +223,19 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu public J.Assignment visitAssignment(J.Assignment assignment, ExecutionContext ctx) { assignment = super.visitAssignment(assignment, ctx); - // Check if we're reassigning a ThreadLocal field - if (isThreadLocalFieldAccess(assignment.getVariable())) { - String fqn = getFieldFullyQualifiedName(assignment.getVariable()); - if (fqn != null) { - Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); - boolean isInitContext = isInInitializationContext(); - acc.recordMutation(fqn, sourcePath, isInitContext); - } + // Early return if not a ThreadLocal field access + if (!isThreadLocalFieldAccess(assignment.getVariable())) { + return assignment; } + + @Nullable String fqn = getFieldFullyQualifiedName(assignment.getVariable()); + if (fqn == null) { + return assignment; + } + + Path sourcePath = getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath(); + boolean isInitContext = isInInitializationContext(); + acc.recordMutation(fqn, sourcePath, isInitContext); return assignment; } @@ -244,25 +263,30 @@ private boolean isThreadLocalFieldAccess(Expression expression) { return false; } - private String getFieldFullyQualifiedName(Expression expression) { - JavaType.Variable varType = null; + private @Nullable String getFieldFullyQualifiedName(@Nullable Expression expression) { + if (expression == null) { + return null; + } + JavaType.@Nullable Variable varType = null; if (expression instanceof J.Identifier) { varType = ((J.Identifier) expression).getFieldType(); } else if (expression instanceof J.FieldAccess) { varType = ((J.FieldAccess) expression).getName().getFieldType(); } - if (varType != null && varType.getOwner() instanceof JavaType.FullyQualified) { - JavaType.FullyQualified owner = (JavaType.FullyQualified) varType.getOwner(); - return owner.getFullyQualifiedName() + "." + varType.getName(); + if (varType == null) { + return null; + } + + @Nullable JavaType owner = varType.getOwner(); + if (!(owner instanceof JavaType.FullyQualified)) { + return null; } - return null; - } - private boolean hasModifier(List modifiers, J.Modifier.Type type) { - return modifiers.stream().anyMatch(m -> m.getType() == type); + return ((JavaType.FullyQualified) owner).getFullyQualifiedName() + "." + varType.getName(); } + }); } @@ -302,8 +326,8 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath().toString(), className, fieldName, - getAccessModifier(multiVariable.getModifiers()), - getModifiers(multiVariable.getModifiers()), + getAccessModifier(multiVariable), + getModifiers(multiVariable), mutationType, message )); @@ -317,31 +341,28 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m return multiVariable; } - private String getAccessModifier(List modifiers) { - if (hasModifier(modifiers, J.Modifier.Type.Private)) { + private String getAccessModifier(J.VariableDeclarations variableDecls) { + if (variableDecls.hasModifier(J.Modifier.Type.Private)) { return "private"; - } else if (hasModifier(modifiers, J.Modifier.Type.Protected)) { + } else if (variableDecls.hasModifier(J.Modifier.Type.Protected)) { return "protected"; - } else if (hasModifier(modifiers, J.Modifier.Type.Public)) { + } else if (variableDecls.hasModifier(J.Modifier.Type.Public)) { return "public"; } return "package-private"; } - private String getModifiers(List modifiers) { + private String getModifiers(J.VariableDeclarations variableDecls) { List mods = new ArrayList<>(); - if (hasModifier(modifiers, J.Modifier.Type.Static)) { + if (variableDecls.hasModifier(J.Modifier.Type.Static)) { mods.add("static"); } - if (hasModifier(modifiers, J.Modifier.Type.Final)) { + if (variableDecls.hasModifier(J.Modifier.Type.Final)) { mods.add("final"); } return String.join(" ", mods); } - private boolean hasModifier(List modifiers, J.Modifier.Type type) { - return modifiers.stream().anyMatch(m -> m.getType() == type); - } }); } @@ -349,7 +370,7 @@ private boolean hasModifier(List modifiers, J.Modifier.Type type) { protected abstract String getMessage(ThreadLocalInfo info); protected abstract String getMutationType(ThreadLocalInfo info); - protected static boolean isThreadLocalType(JavaType type) { + protected static boolean isThreadLocalType(@Nullable JavaType type) { return TypeUtils.isOfClassType(type, THREAD_LOCAL_FQN) || TypeUtils.isOfClassType(type, INHERITED_THREAD_LOCAL_FQN); } diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java b/src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java index 414e4caa84..6fe60b1ff9 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java +++ b/src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java @@ -16,7 +16,13 @@ package org.openrewrite.java.migrate.search; import lombok.EqualsAndHashCode; +import lombok.Value; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; + +@Value @EqualsAndHashCode(callSuper = false) public class FindNeverMutatedThreadLocals extends AbstractFindThreadLocals { @@ -32,6 +38,11 @@ public String getDescription() { "The recipe identifies `ThreadLocal` variables that are only initialized but never reassigned or modified through `set()` or `remove()` methods."; } + @Override + public Set getTags() { + return new HashSet<>(Arrays.asList("java25", "threadlocal", "scopedvalue", "migration")); + } + @Override protected boolean shouldMarkThreadLocal(ThreadLocalInfo info) { // Mark ThreadLocals that have no mutations at all diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutside.java b/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutside.java similarity index 83% rename from src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutside.java rename to src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutside.java index b91cb97784..a3a60141cd 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutside.java +++ b/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutside.java @@ -16,13 +16,19 @@ package org.openrewrite.java.migrate.search; import lombok.EqualsAndHashCode; +import lombok.Value; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; + +@Value @EqualsAndHashCode(callSuper = false) -public class FindThreadLocalsMutatableFromOutside extends AbstractFindThreadLocals { +public class FindThreadLocalsMutableFromOutside extends AbstractFindThreadLocals { @Override public String getDisplayName() { - return "Find ThreadLocal variables mutatable from outside their defining class"; + return "Find ThreadLocal variables mutable from outside their defining class"; } @Override @@ -32,6 +38,11 @@ public String getDescription() { "This includes non-private ThreadLocals or those mutated from other classes in the codebase."; } + @Override + public Set getTags() { + return new HashSet<>(Arrays.asList("java25", "threadlocal", "scopedvalue", "migration", "security")); + } + @Override protected boolean shouldMarkThreadLocal(ThreadLocalInfo info) { // Mark ThreadLocals that are either: diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java b/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java index 9934d6993e..6f27a5b034 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java +++ b/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java @@ -16,7 +16,13 @@ package org.openrewrite.java.migrate.search; import lombok.EqualsAndHashCode; +import lombok.Value; +import java.util.Arrays; +import java.util.HashSet; +import java.util.Set; + +@Value @EqualsAndHashCode(callSuper = false) public class FindThreadLocalsMutatedOnlyInDefiningScope extends AbstractFindThreadLocals { @@ -32,29 +38,36 @@ public String getDescription() { "The recipe identifies `ThreadLocal` variables that are only modified during initialization or within their declaring class."; } + @Override + public Set getTags() { + return new HashSet<>(Arrays.asList("java25", "threadlocal", "scopedvalue", "migration")); + } + @Override protected boolean shouldMarkThreadLocal(ThreadLocalInfo info) { - // Mark private ThreadLocals that ARE mutated, but only locally - return info.hasAnyMutation() && - info.isPrivate() && - info.isOnlyLocallyMutated(); + // Early return for ThreadLocals without mutations + if (!info.hasAnyMutation()) { + return false; + } + // Early return for non-private ThreadLocals + if (!info.isPrivate()) { + return false; + } + // Mark if only locally mutated + return info.isOnlyLocallyMutated(); } @Override protected String getMessage(ThreadLocalInfo info) { - if (info.hasOnlyInitMutations()) { - return "ThreadLocal is only mutated during initialization (constructor/static initializer)"; - } else { - return "ThreadLocal is only mutated within its defining class"; - } + return info.hasOnlyInitMutations() + ? "ThreadLocal is only mutated during initialization (constructor/static initializer)" + : "ThreadLocal is only mutated within its defining class"; } @Override protected String getMutationType(ThreadLocalInfo info) { - if (info.hasOnlyInitMutations()) { - return "Mutated only in initialization"; - } else { - return "Mutated in defining class"; - } + return info.hasOnlyInitMutations() + ? "Mutated only in initialization" + : "Mutated in defining class"; } } \ No newline at end of file diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutsideTest.java b/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutsideTest.java similarity index 98% rename from src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutsideTest.java rename to src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutsideTest.java index 5f02aaa4cc..19fe6e4200 100644 --- a/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatableFromOutsideTest.java +++ b/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutsideTest.java @@ -22,11 +22,11 @@ import static org.openrewrite.java.Assertions.java; -class FindThreadLocalsMutatableFromOutsideTest implements RewriteTest { +class FindThreadLocalsMutableFromOutsideTest implements RewriteTest { @Override public void defaults(RecipeSpec spec) { - spec.recipe(new FindThreadLocalsMutatableFromOutside()); + spec.recipe(new FindThreadLocalsMutableFromOutside()); } @DocumentExample From 7563cb6abe18393eb4eb552196a67db46e79af9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Fri, 17 Oct 2025 00:10:35 +0200 Subject: [PATCH 08/10] apply best practices --- .../java/migrate/search/AbstractFindThreadLocals.java | 2 +- .../java/migrate/search/FindNeverMutatedThreadLocals.java | 2 +- .../java/migrate/search/FindThreadLocalsMutableFromOutside.java | 2 +- .../search/FindThreadLocalsMutatedOnlyInDefiningScope.java | 2 +- .../org/openrewrite/java/migrate/search/ThreadLocalTable.java | 2 +- .../migrate/search/FindThreadLocalsMutableFromOutsideTest.java | 2 +- .../search/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java b/src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java index e35acf7675..050df7dd2a 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java +++ b/src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java @@ -374,4 +374,4 @@ protected static boolean isThreadLocalType(@Nullable JavaType type) { return TypeUtils.isOfClassType(type, THREAD_LOCAL_FQN) || TypeUtils.isOfClassType(type, INHERITED_THREAD_LOCAL_FQN); } -} \ No newline at end of file +} diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java b/src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java index 6fe60b1ff9..eccdc18d69 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java +++ b/src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java @@ -58,4 +58,4 @@ protected String getMessage(ThreadLocalInfo info) { protected String getMutationType(ThreadLocalInfo info) { return "Never mutated"; } -} \ No newline at end of file +} diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutside.java b/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutside.java index a3a60141cd..372bdd9bac 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutside.java +++ b/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutside.java @@ -70,4 +70,4 @@ protected String getMutationType(ThreadLocalInfo info) { return "Potentially mutable"; } } -} \ No newline at end of file +} diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java b/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java index 6f27a5b034..a25b15e6f8 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java +++ b/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java @@ -70,4 +70,4 @@ protected String getMutationType(ThreadLocalInfo info) { ? "Mutated only in initialization" : "Mutated in defining class"; } -} \ No newline at end of file +} diff --git a/src/main/java/org/openrewrite/java/migrate/search/ThreadLocalTable.java b/src/main/java/org/openrewrite/java/migrate/search/ThreadLocalTable.java index f968f84b5a..13051ccaf5 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/ThreadLocalTable.java +++ b/src/main/java/org/openrewrite/java/migrate/search/ThreadLocalTable.java @@ -58,4 +58,4 @@ public static class Row { description = "Detailed message about the ThreadLocal's usage pattern.") String message; } -} \ No newline at end of file +} diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutsideTest.java b/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutsideTest.java index 19fe6e4200..3ef34db3f1 100644 --- a/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutsideTest.java +++ b/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutsideTest.java @@ -251,4 +251,4 @@ public void modifyThreadLocal() { ) ); } -} \ No newline at end of file +} diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java b/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java index 095737c460..ce7693ccb3 100644 --- a/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java +++ b/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java @@ -251,4 +251,4 @@ public String getValue() { ) ); } -} \ No newline at end of file +} From 88d6365b1b06a6dd4318eb8bd1223c443e707b70 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Fri, 17 Oct 2025 00:15:25 +0200 Subject: [PATCH 09/10] remove old artifacts --- .../ConvertImmutableClassToRecord.java | 289 ------ .../ConvertImmutableClassToRecordTest.java | 821 ------------------ 2 files changed, 1110 deletions(-) delete mode 100644 src/main/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecord.java delete mode 100644 src/test/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecordTest.java diff --git a/src/main/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecord.java b/src/main/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecord.java deleted file mode 100644 index aa7c2496ed..0000000000 --- a/src/main/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecord.java +++ /dev/null @@ -1,289 +0,0 @@ -/* - * Copyright 2024 the original author or authors. - *

- * Licensed under the Moderne Source Available License (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - *

- * https://docs.moderne.io/licensing/moderne-source-available-license - *

- * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.openrewrite.java.migrate; - -import lombok.EqualsAndHashCode; -import lombok.Value; -import org.jspecify.annotations.Nullable; -import org.openrewrite.ExecutionContext; -import org.openrewrite.Option; -import org.openrewrite.Recipe; -import org.openrewrite.TreeVisitor; -import org.openrewrite.java.ChangeMethodName; -import org.openrewrite.java.JavaTemplate; -import org.openrewrite.java.JavaVisitor; -import org.openrewrite.java.tree.J; - -import java.util.*; -import java.util.stream.Collectors; - -@Value -@EqualsAndHashCode(callSuper = false) -public class ConvertImmutableClassToRecord extends Recipe { - - @Option(displayName = "Package pattern", - description = "A glob pattern to match packages where classes should be converted to records. " + - "If not specified, the recipe will be applied to all packages.", - example = "com.example.**", - required = false) - @Nullable - String packagePattern; - - @Override - public String getDisplayName() { - return "Convert immutable class to record"; - } - - @Override - public String getDescription() { - //language=Markdown - return "Converts immutable classes to Java records. This is a composite recipe that:\n" + - " 1. Identifies classes that meet record conversion criteria\n" + - " 2. Converts eligible classes to records\n" + - " 3. Updates all getter method calls to use record accessor syntax\n" + - "\n" + - " A class is eligible for conversion if it:\n" + - " - Has only private fields\n" + - " - Has corresponding getter methods for all fields\n" + - " - Has no setter methods\n" + - " - Does not extend another class\n" + - " - Is effectively immutable"; - } - - @Override - public TreeVisitor getVisitor() { - return new ConvertImmutableClassToRecordVisitor(); - } - - private class ConvertImmutableClassToRecordVisitor extends JavaVisitor { - - @Override - public J visitClassDeclaration(J.ClassDeclaration classDecl, ExecutionContext ctx) { - if (!isEligibleForRecordConversion(classDecl) || !matchesPackagePattern(classDecl)) { - return classDecl; - } - - // Get all getter methods that will need to be updated - List getterMethods = getGetterMethods(classDecl); - String classTypeFqn = getFullyQualifiedName(classDecl); - - // Schedule method name changes for all getters after this transformation - for (J.MethodDeclaration getter : getterMethods) { - String fieldName = getFieldNameFromGetter(getter); - if (fieldName != null) { - String oldMethodPattern = classTypeFqn + " " + getter.getSimpleName() + "()"; - doAfterVisit(new ChangeMethodName(oldMethodPattern, fieldName, true, null).getVisitor()); - } - } - - // Transform the class to a record - return transformToRecord(classDecl); - } - - private J.ClassDeclaration transformToRecord(J.ClassDeclaration classDecl) { - List fields = getPrivateFields(classDecl); - List utilityMethods = extractUtilityMethods(classDecl); - - // Create record components string - StringBuilder recordComponents = new StringBuilder(); - for (int i = 0; i < fields.size(); i++) { - J.VariableDeclarations field = fields.get(i); - for (J.VariableDeclarations.NamedVariable var : field.getVariables()) { - if (recordComponents.length() > 0) { - recordComponents.append(", "); - } - - // Include field annotations - StringBuilder fieldComponent = new StringBuilder(); - if (!field.getLeadingAnnotations().isEmpty()) { - for (J.Annotation annotation : field.getLeadingAnnotations()) { - fieldComponent.append("@").append(annotation.getAnnotationType().toString()).append(" "); - } - } - fieldComponent.append(field.getTypeExpression()).append(" ").append(var.getSimpleName()); - recordComponents.append(fieldComponent); - } - } - - // Build record template - StringBuilder recordTemplate = new StringBuilder(); - - // Add class-level annotations and modifiers - if (!classDecl.getLeadingAnnotations().isEmpty() || !classDecl.getModifiers().isEmpty()) { - for (J.Annotation annotation : classDecl.getLeadingAnnotations()) { - recordTemplate.append("@").append(annotation.getAnnotationType().toString()).append(" "); - } - for (J.Modifier modifier : classDecl.getModifiers()) { - if (modifier.getType() != J.Modifier.Type.Final) { // Records are implicitly final - recordTemplate.append(modifier.getType().toString().toLowerCase()).append(" "); - } - } - } - - recordTemplate.append("record ").append(classDecl.getSimpleName()).append("(") - .append(recordComponents).append(")"); - - if (!classDecl.getImplements().isEmpty()) { - recordTemplate.append(" implements "); - for (int i = 0; i < classDecl.getImplements().size(); i++) { - if (i > 0) { - recordTemplate.append( ", " ); - } - recordTemplate.append("#{any()}"); - } - } - - recordTemplate.append(" { "); - for (int i = 0; i < utilityMethods.size(); i++) { - recordTemplate.append("#{any()} "); - } - recordTemplate.append("}"); - - // Prepare template parameters - List templateParams = new ArrayList<>(); - if (!classDecl.getImplements().isEmpty()) { - for (J j : classDecl.getImplements()) { - templateParams.add(j); - } - } - for (J.MethodDeclaration method : utilityMethods) { - templateParams.add(method); - } - - JavaTemplate template = JavaTemplate.builder(recordTemplate.toString()).build(); - return template.apply(updateCursor(classDecl), classDecl.getCoordinates().replace(), templateParams.toArray()); - } - - private boolean isEligibleForRecordConversion(J.ClassDeclaration classDecl) { - // Skip if class extends another class (records can't extend classes) - if (classDecl.getExtends() != null) { - return false; - } - - // Skip if class is not a regular class - if (classDecl.getKind() != J.ClassDeclaration.Kind.Type.Class) { - return false; - } - - List fields = getPrivateFields(classDecl); - if (fields.isEmpty()) { - return false; - } - - // Check if all fields have corresponding getters - Set fieldNames = fields.stream() - .flatMap(field -> field.getVariables().stream()) - .map(J.VariableDeclarations.NamedVariable::getSimpleName) - .collect(Collectors.toSet()); - - Set getterFields = getGetterMethods(classDecl).stream() - .map(this::getFieldNameFromGetter) - .filter(Objects::nonNull) - .collect(Collectors.toSet()); - - return fieldNames.equals(getterFields) && !hasSetterMethods(classDecl); - } - - private List extractUtilityMethods(J.ClassDeclaration classDecl) { - return classDecl.getBody().getStatements().stream() - .filter(J.MethodDeclaration.class::isInstance) - .map(J.MethodDeclaration.class::cast) - .filter(method -> !isConstructor(method) && !isGetterMethod(method) && !isSetterMethod(method)) - .collect(Collectors.toList()); - } - - private List getPrivateFields(J.ClassDeclaration classDecl) { - return classDecl.getBody().getStatements().stream() - .filter(J.VariableDeclarations.class::isInstance) - .map(J.VariableDeclarations.class::cast) - .filter(field -> field.hasModifier(J.Modifier.Type.Private)) - .collect(Collectors.toList()); - } - - private List getGetterMethods(J.ClassDeclaration classDecl) { - return classDecl.getBody().getStatements().stream() - .filter(J.MethodDeclaration.class::isInstance) - .map(J.MethodDeclaration.class::cast) - .filter(this::isGetterMethod) - .collect(Collectors.toList()); - } - - private boolean hasSetterMethods(J.ClassDeclaration classDecl) { - return classDecl.getBody().getStatements().stream() - .filter(J.MethodDeclaration.class::isInstance) - .map(J.MethodDeclaration.class::cast) - .anyMatch(this::isSetterMethod); - } - - private boolean isConstructor(J.MethodDeclaration method) { - return method.isConstructor(); - } - - private boolean isGetterMethod(J.MethodDeclaration method) { - String methodName = method.getSimpleName(); - return (methodName.startsWith("get") || methodName.startsWith("is")) - && method.getParameters().isEmpty() - && method.getReturnTypeExpression() != null; - } - - private boolean isSetterMethod(J.MethodDeclaration method) { - String methodName = method.getSimpleName(); - return methodName.startsWith("set") - && method.getParameters().size() == 1; - } - - private @Nullable String getFieldNameFromGetter(J.MethodDeclaration getter) { - String methodName = getter.getSimpleName(); - if (methodName.startsWith("get") && methodName.length() > 3) { - return Character.toLowerCase(methodName.charAt(3)) + methodName.substring(4); - } else if (methodName.startsWith("is") && methodName.length() > 2) { - return Character.toLowerCase(methodName.charAt(2)) + methodName.substring(3); - } - return null; - } - - private boolean matchesPackagePattern(J.ClassDeclaration classDecl) { - if (packagePattern == null || packagePattern.trim().isEmpty()) { - return true; // No pattern specified, match all - } - - String packageName = getPackageName(classDecl); - if (packageName == null) { - return packagePattern.equals("*"); // Default package - } - - // Simple glob pattern matching - String pattern = packagePattern.replace("**", ".*").replace("*", "[^.]*"); - return packageName.matches(pattern); - } - - private String getPackageName(J.ClassDeclaration classDecl) { - J.CompilationUnit cu = getCursor().firstEnclosing(J.CompilationUnit.class); - if (cu != null && cu.getPackageDeclaration() != null) { - return cu.getPackageDeclaration().getExpression().toString(); - } - return null; - } - - private String getFullyQualifiedName(J.ClassDeclaration classDecl) { - String packageName = getPackageName(classDecl); - if (packageName != null) { - return packageName + "." + classDecl.getSimpleName(); - } - return classDecl.getSimpleName(); - } - } -} diff --git a/src/test/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecordTest.java b/src/test/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecordTest.java deleted file mode 100644 index 169bc5bc4e..0000000000 --- a/src/test/java/org/openrewrite/java/migrate/ConvertImmutableClassToRecordTest.java +++ /dev/null @@ -1,821 +0,0 @@ -/* - * Copyright 2024 the original author or authors. - *

- * Licensed under the Moderne Source Available License (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - *

- * https://docs.moderne.io/licensing/moderne-source-available-license - *

- * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - */ -package org.openrewrite.java.migrate; - -import org.junit.jupiter.api.Test; -import org.openrewrite.DocumentExample; -import org.openrewrite.test.RecipeSpec; -import org.openrewrite.test.RewriteTest; - -import static org.openrewrite.java.Assertions.java; - -class ConvertImmutableClassToRecordTest implements RewriteTest { - - @Override - public void defaults(RecipeSpec spec) { - spec.recipe(new ConvertImmutableClassToRecord(null)); - } - - @DocumentExample - @Test - void simpleImmutableClass() { - rewriteRun( - //language=java - java( - """ - class Person { - private final String name; - private final int age; - - public Person(String name, int age) { - this.name = name; - this.age = age; - } - - public String getName() { - return name; - } - - public int getAge() { - return age; - } - } - """, - """ - record Person(String name, int age) { - } - """ - ) - ); - } - - @Test - void completeClassToRecordTransformationWithMethodCallUpdates() { - rewriteRun( - //language=java - java( - """ - class Person { - private final String name; - private final int age; - - public Person(String name, int age) { - this.name = name; - this.age = age; - } - - public String getName() { - return name; - } - - public int getAge() { - return age; - } - - public String getDisplayName() { - return name + " (" + age + ")"; - } - } - """, - """ - record Person(String name, int age) { - public String getDisplayName() { - return name + " (" + age + ")"; - } - } - """ - ), - //language=java - java( - """ - class PersonService { - public void processPerson(Person person) { - String name = person.getName(); - int age = person.getAge(); - String display = person.getDisplayName(); - System.out.println("Processing: " + display); - } - } - """, - """ - class PersonService { - public void processPerson(Person person) { - String name = person.name(); - int age = person.age(); - String display = person.getDisplayName(); - System.out.println("Processing: " + display); - } - } - """ - ) - ); - } - - @Test - void packagePatternRestriction() { - rewriteRun( - spec -> spec.recipe(new ConvertImmutableClassToRecord("com.example.model.**")), - //language=java - java( - """ - package com.example.model; - - class Person { - private final String name; - - public Person(String name) { - this.name = name; - } - - public String getName() { - return name; - } - } - """, - """ - package com.example.model; - - record Person(String name) { - } - """ - ), - //language=java - java( - """ - package com.example.service; - - class User { - private final String username; - - public User(String username) { - this.username = username; - } - - public String getUsername() { - return username; - } - } - """ - // Should not be transformed due to package pattern restriction - ) - ); - } - - @Test - void multipleClassesTransformation() { - rewriteRun( - //language=java - java( - """ - class Address { - private final String street; - private final String city; - - public Address(String street, String city) { - this.street = street; - this.city = city; - } - - public String getStreet() { - return street; - } - - public String getCity() { - return city; - } - } - - class Person { - private final String name; - private final Address address; - - public Person(String name, Address address) { - this.name = name; - this.address = address; - } - - public String getName() { - return name; - } - - public Address getAddress() { - return address; - } - } - """, - """ - record Address(String street, String city) { - } - - record Person(String name, Address address) { - } - """ - ), - //language=java - java( - """ - class AddressService { - public void printFullAddress(Person person) { - Address addr = person.getAddress(); - String street = addr.getStreet(); - String city = addr.getCity(); - System.out.println(street + ", " + city); - } - } - """, - """ - class AddressService { - public void printFullAddress(Person person) { - Address addr = person.address(); - String street = addr.street(); - String city = addr.city(); - System.out.println(street + ", " + city); - } - } - """ - ) - ); - } - - @Test - void complexBusinessLogicPreservation() { - rewriteRun( - //language=java - java( - """ - import java.util.Objects; - - class Product { - private final String id; - private final String name; - private final double price; - - public Product(String id, String name, double price) { - this.id = id; - this.name = name; - this.price = price; - } - - public String getId() { - return id; - } - - public String getName() { - return name; - } - - public double getPrice() { - return price; - } - - public String getFormattedPrice() { - return String.format("$%.2f", price); - } - - public boolean isExpensive() { - return price > 100.0; - } - - public Product withDiscount(double discountPercent) { - double newPrice = price * (1 - discountPercent / 100); - return new Product(id, name, newPrice); - } - } - """, - """ - import java.util.Objects; - - record Product(String id, String name, double price) { - public String getFormattedPrice() { - return String.format("$%.2f", price); - } - - public boolean isExpensive() { - return price > 100.0; - } - - public Product withDiscount(double discountPercent) { - double newPrice = price * (1 - discountPercent / 100); - return new Product(id, name, newPrice); - } - } - """ - ) - ); - } - - @Test - void nestedClassesHandling() { - rewriteRun( - //language=java - java( - """ - class OuterClass { - private String value; - - static class InnerData { - private final String data; - private final int count; - - public InnerData(String data, int count) { - this.data = data; - this.count = count; - } - - public String getData() { - return data; - } - - public int getCount() { - return count; - } - } - - public void process(InnerData inner) { - String data = inner.getData(); - int count = inner.getCount(); - System.out.println(data + ": " + count); - } - } - """, - """ - class OuterClass { - private String value; - - static record InnerData(String data, int count) { - } - - public void process(InnerData inner) { - String data = inner.data(); - int count = inner.count(); - System.out.println(data + ": " + count); - } - } - """ - ) - ); - } - - @Test - void genericTypesSupport() { - rewriteRun( - //language=java - java( - """ - import java.util.List; - import java.util.Optional; - - class Container { - private final T value; - private final List items; - - public Container(T value, List items) { - this.value = value; - this.items = items; - } - - public T getValue() { - return value; - } - - public List getItems() { - return items; - } - - public Optional findFirst() { - return items.isEmpty() ? Optional.empty() : Optional.of(items.get(0)); - } - } - """, - """ - import java.util.List; - import java.util.Optional; - - record Container(T value, List items) { - public Optional findFirst() { - return items.isEmpty() ? Optional.empty() : Optional.of(items.get(0)); - } - } - """ - ) - ); - } - - @Test - void integrationWithStreamOperations() { - rewriteRun( - //language=java - java( - """ - import java.util.List; - import java.util.stream.Collectors; - - class Employee { - private final String name; - private final String department; - private final double salary; - - public Employee(String name, String department, double salary) { - this.name = name; - this.department = department; - this.salary = salary; - } - - public String getName() { - return name; - } - - public String getDepartment() { - return department; - } - - public double getSalary() { - return salary; - } - } - - class EmployeeService { - public List getNamesByDepartment(List employees, String dept) { - return employees.stream() - .filter(emp -> emp.getDepartment().equals(dept)) - .map(Employee::getName) - .collect(Collectors.toList()); - } - - public double getTotalSalary(List employees) { - return employees.stream() - .mapToDouble(Employee::getSalary) - .sum(); - } - } - """, - """ - import java.util.List; - import java.util.stream.Collectors; - - record Employee(String name, String department, double salary) { - } - - class EmployeeService { - public List getNamesByDepartment(List employees, String dept) { - return employees.stream() - .filter(emp -> emp.department().equals(dept)) - .map(Employee::name) - .collect(Collectors.toList()); - } - - public double getTotalSalary(List employees) { - return employees.stream() - .mapToDouble(Employee::salary) - .sum(); - } - } - """ - ) - ); - } - - @Test - void immutableClassWithAnnotations() { - rewriteRun( - //language=java - java( - """ - import javax.validation.constraints.NotNull; - import javax.validation.constraints.Min; - - @Entity - public class Person { - @NotNull - private final String name; - - @Min(0) - private final int age; - - public Person(String name, int age) { - this.name = name; - this.age = age; - } - - public String getName() { - return name; - } - - public int getAge() { - return age; - } - } - """, - """ - import javax.validation.constraints.NotNull; - import javax.validation.constraints.Min; - - @Entity - public record Person(@NotNull String name, @Min(0) int age) { - } - """ - ) - ); - } - - @Test - void booleanGetterWithIsPrefix() { - rewriteRun( - //language=java - java( - """ - class User { - private final String name; - private final boolean active; - - public User(String name, boolean active) { - this.name = name; - this.active = active; - } - - public String getName() { - return name; - } - - public boolean isActive() { - return active; - } - } - """, - """ - record User(String name, boolean active) { - } - """ - ) - ); - } - - @Test - void classWithCollectionFields() { - rewriteRun( - //language=java - java( - """ - import java.util.List; - import java.util.Set; - - class Student { - private final String name; - private final List courses; - private final Set skills; - - public Student(String name, List courses, Set skills) { - this.name = name; - this.courses = courses; - this.skills = skills; - } - - public String getName() { - return name; - } - - public List getCourses() { - return courses; - } - - public Set getSkills() { - return skills; - } - } - """, - """ - import java.util.List; - import java.util.Set; - - record Student(String name, List courses, Set skills) { - } - """ - ) - ); - } - - @Test - void classWithInterfaceImplementation() { - rewriteRun( - //language=java - java( - """ - interface Identifiable { - String getId(); - } - - class Entity implements Identifiable { - private final String id; - private final String name; - - public Entity(String id, String name) { - this.id = id; - this.name = name; - } - - public String getId() { - return id; - } - - public String getName() { - return name; - } - } - """, - """ - interface Identifiable { - String getId(); - } - - record Entity(String id, String name) implements Identifiable { - } - """ - ) - ); - } - - @Test - void shouldNotConvertClassWithSetterMethods() { - rewriteRun( - //language=java - java( - """ - class MutablePerson { - private String name; - private int age; - - public String getName() { - return name; - } - - public void setName(String name) { - this.name = name; - } - - public int getAge() { - return age; - } - - public void setAge(int age) { - this.age = age; - } - } - """ - ) - ); - } - - @Test - void shouldNotConvertClassThatExtendsAnotherClass() { - rewriteRun( - //language=java - java( - """ - class BaseEntity { - private final String id; - - public BaseEntity(String id) { - this.id = id; - } - - public String getId() { - return id; - } - } - - class Person extends BaseEntity { - private final String name; - - public Person(String id, String name) { - super(id); - this.name = name; - } - - public String getName() { - return name; - } - } - """ - ) - ); - } - - @Test - void shouldNotConvertClassWithoutGetters() { - rewriteRun( - //language=java - java( - """ - class DataHolder { - private final String data; - - public DataHolder(String data) { - this.data = data; - } - - public void processData() { - System.out.println(data); - } - } - """ - ) - ); - } - - @Test - void shouldNotConvertClassWithMissingGetters() { - rewriteRun( - //language=java - java( - """ - class PartialPerson { - private final String name; - private final int age; - - public PartialPerson(String name, int age) { - this.name = name; - this.age = age; - } - - public String getName() { - return name; - } - - // Missing getAge() method - } - """ - ) - ); - } - - @Test - void shouldNotConvertEnum() { - rewriteRun( - //language=java - java( - """ - enum Status { - ACTIVE, - INACTIVE; - - public boolean isActive() { - return this == ACTIVE; - } - } - """ - ) - ); - } - - @Test - void shouldNotConvertInterface() { - rewriteRun( - //language=java - java( - """ - interface Repository { - String getName(); - int getVersion(); - } - """ - ) - ); - } - - @Test - void shouldNotConvertAbstractClass() { - rewriteRun( - //language=java - java( - """ - abstract class AbstractEntity { - private final String id; - - public AbstractEntity(String id) { - this.id = id; - } - - public String getId() { - return id; - } - - public abstract void process(); - } - """ - ) - ); - } -} From 9d2a7982a3c1c8be4e0b4636aa2ae19a16b46b86 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Merlin=20B=C3=B6gershausen?= Date: Fri, 17 Oct 2025 09:46:33 +0200 Subject: [PATCH 10/10] apply code review --- .../AbstractFindThreadLocals.java | 133 +++++++++++------- .../FindNeverMutatedThreadLocals.java | 4 +- .../FindThreadLocalsMutableFromOutside.java | 15 +- ...hreadLocalsMutatedOnlyInDefiningScope.java | 24 ++-- .../search/threadlocal/package-info.java | 21 +++ ...NeverMutatedThreadLocalsCrossFileTest.java | 2 +- .../FindNeverMutatedThreadLocalsTest.java | 2 +- ...indThreadLocalsMutableFromOutsideTest.java | 2 +- ...dLocalsMutatedOnlyInDefiningScopeTest.java | 2 +- 9 files changed, 130 insertions(+), 75 deletions(-) rename src/main/java/org/openrewrite/java/migrate/search/{ => threadlocal}/AbstractFindThreadLocals.java (78%) rename src/main/java/org/openrewrite/java/migrate/search/{ => threadlocal}/FindNeverMutatedThreadLocals.java (95%) rename src/main/java/org/openrewrite/java/migrate/search/{ => threadlocal}/FindThreadLocalsMutableFromOutside.java (86%) rename src/main/java/org/openrewrite/java/migrate/search/{ => threadlocal}/FindThreadLocalsMutatedOnlyInDefiningScope.java (78%) create mode 100644 src/main/java/org/openrewrite/java/migrate/search/threadlocal/package-info.java rename src/test/java/org/openrewrite/java/migrate/search/{ => threadlocal}/FindNeverMutatedThreadLocalsCrossFileTest.java (99%) rename src/test/java/org/openrewrite/java/migrate/search/{ => threadlocal}/FindNeverMutatedThreadLocalsTest.java (99%) rename src/test/java/org/openrewrite/java/migrate/search/{ => threadlocal}/FindThreadLocalsMutableFromOutsideTest.java (99%) rename src/test/java/org/openrewrite/java/migrate/search/{ => threadlocal}/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java (99%) diff --git a/src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java b/src/main/java/org/openrewrite/java/migrate/search/threadlocal/AbstractFindThreadLocals.java similarity index 78% rename from src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java rename to src/main/java/org/openrewrite/java/migrate/search/threadlocal/AbstractFindThreadLocals.java index 050df7dd2a..1b32173630 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java +++ b/src/main/java/org/openrewrite/java/migrate/search/threadlocal/AbstractFindThreadLocals.java @@ -13,8 +13,9 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.openrewrite.java.migrate.search; +package org.openrewrite.java.migrate.search.threadlocal; +import lombok.Getter; import lombok.Value; import org.jspecify.annotations.Nullable; import org.openrewrite.ExecutionContext; @@ -23,6 +24,7 @@ import org.openrewrite.TreeVisitor; import org.openrewrite.java.JavaIsoVisitor; import org.openrewrite.java.MethodMatcher; +import org.openrewrite.java.migrate.search.ThreadLocalTable; import org.openrewrite.java.search.UsesType; import org.openrewrite.java.tree.Expression; import org.openrewrite.java.tree.J; @@ -33,7 +35,9 @@ import java.nio.file.Path; import java.util.*; -import static org.openrewrite.Preconditions.*; +import static org.openrewrite.Preconditions.check; +import static org.openrewrite.Preconditions.or; + public abstract class AbstractFindThreadLocals extends ScanningRecipe { @@ -45,8 +49,7 @@ public abstract class AbstractFindThreadLocals extends ScanningRecipe initMutationPaths = new HashSet<>(); private final Set regularMutationPaths = new HashSet<>(); @@ -92,38 +99,37 @@ void setDeclaration(Path path, boolean priv, boolean stat, boolean fin) { this.declared = true; } + /** + * Records a mutation from an initialization context (constructor/static initializer). + */ void addInitMutation(Path path) { initMutationPaths.add(path); } + /** + * Records a regular (non-initialization) mutation. + */ void addRegularMutation(Path path) { regularMutationPaths.add(path); } - public boolean isPrivate() { - return isPrivate; - } - - public boolean isStatic() { - return isStatic; - } - - public boolean isFinal() { - return isFinal; - } - - public boolean isDeclared() { - return declared; - } - - public boolean hasAnyMutation() { - return !initMutationPaths.isEmpty() || !regularMutationPaths.isEmpty(); + /** + * Checks if there are no mutations (both init and regular). + */ + public boolean hasNoMutation() { + return initMutationPaths.isEmpty() && regularMutationPaths.isEmpty(); } + /** + * Checks if there are only mutations from initialization contexts (constructors/static initializers). + */ public boolean hasOnlyInitMutations() { return !initMutationPaths.isEmpty() && regularMutationPaths.isEmpty(); } + /** + * Checks if there are any mutations (both init and regular) from files other than the declaration file. + */ public boolean hasExternalMutations() { if (!declared || declarationPath == null) { return true; // Conservative @@ -134,6 +140,9 @@ public boolean hasExternalMutations() { regularMutationPaths.stream().anyMatch(p -> !p.equals(declarationPath)); } + /** + * Checks if all mutations (both init and regular) are from the same file as the declaration. + */ public boolean isOnlyLocallyMutated() { if (!declared || declarationPath == null) { return false; @@ -208,7 +217,7 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu return method; } - @Nullable String fqn = getFieldFullyQualifiedName(method.getSelect()); + String fqn = getFieldFullyQualifiedName(method.getSelect()); if (fqn == null) { return method; } @@ -228,7 +237,7 @@ public J.Assignment visitAssignment(J.Assignment assignment, ExecutionContext ct return assignment; } - @Nullable String fqn = getFieldFullyQualifiedName(assignment.getVariable()); + String fqn = getFieldFullyQualifiedName(assignment.getVariable()); if (fqn == null) { return assignment; } @@ -256,9 +265,10 @@ private boolean isInInitializationContext() { private boolean isThreadLocalFieldAccess(Expression expression) { if (expression instanceof J.Identifier) { - return isThreadLocalType(((J.Identifier) expression).getType()); - } else if (expression instanceof J.FieldAccess) { - return isThreadLocalType(((J.FieldAccess) expression).getType()); + return isThreadLocalType(expression.getType()); + } + if (expression instanceof J.FieldAccess) { + return isThreadLocalType(expression.getType()); } return false; } @@ -279,7 +289,7 @@ private boolean isThreadLocalFieldAccess(Expression expression) { return null; } - @Nullable JavaType owner = varType.getOwner(); + JavaType owner = varType.getOwner(); if (!(owner instanceof JavaType.FullyQualified)) { return null; } @@ -294,22 +304,19 @@ private boolean isThreadLocalFieldAccess(Expression expression) { public TreeVisitor getVisitor(ThreadLocalAccumulator acc) { return check(acc.hasDeclarations(), new JavaIsoVisitor() { - @Override - public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionContext ctx) { - if (dataTable == null) { - dataTable = new ThreadLocalTable(AbstractFindThreadLocals.this); - } - return super.visitCompilationUnit(cu, ctx); - } + @Override public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) { multiVariable = super.visitVariableDeclarations(multiVariable, ctx); J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class); + if(classDecl == null) { + return multiVariable; + } for (J.VariableDeclarations.NamedVariable variable : multiVariable.getVariables()) { - if (isThreadLocalType(variable.getType()) && classDecl != null) { + if (isThreadLocalType(variable.getType())) { String className = classDecl.getType() != null ? classDecl.getType().getFullyQualifiedName() : "UnknownClass"; String fieldName = variable.getName().getSimpleName(); @@ -320,18 +327,15 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m String message = getMessage(info); String mutationType = getMutationType(info); - // Add to data table - if (dataTable != null) { - dataTable.insertRow(ctx, new ThreadLocalTable.Row( - getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath().toString(), - className, - fieldName, - getAccessModifier(multiVariable), - getModifiers(multiVariable), - mutationType, - message - )); - } + dataTable.insertRow(ctx, new ThreadLocalTable.Row( + getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath().toString(), + className, + fieldName, + getAccessModifier(multiVariable), + getFieldModifiers(multiVariable), + mutationType, + message + )); return SearchResult.found(multiVariable, message); } @@ -344,15 +348,17 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m private String getAccessModifier(J.VariableDeclarations variableDecls) { if (variableDecls.hasModifier(J.Modifier.Type.Private)) { return "private"; - } else if (variableDecls.hasModifier(J.Modifier.Type.Protected)) { + } + if (variableDecls.hasModifier(J.Modifier.Type.Protected)) { return "protected"; - } else if (variableDecls.hasModifier(J.Modifier.Type.Public)) { + } + if (variableDecls.hasModifier(J.Modifier.Type.Public)) { return "public"; } return "package-private"; } - private String getModifiers(J.VariableDeclarations variableDecls) { + private String getFieldModifiers(J.VariableDeclarations variableDecls) { List mods = new ArrayList<>(); if (variableDecls.hasModifier(J.Modifier.Type.Static)) { mods.add("static"); @@ -366,8 +372,33 @@ private String getModifiers(J.VariableDeclarations variableDecls) { }); } + /** + * Determines whether a ThreadLocal should be marked based on its usage info. + * Implementations should define the criteria for marking. + * It is used to decide if a ThreadLocal variable should be highlighted in the results. + * If an expected ThreadLocal instance is missing from the results, consider adjusting this method. + * + * @param info The ThreadLocalInfo containing usage details. + * @return true if the ThreadLocal should be marked, false otherwise. + */ protected abstract boolean shouldMarkThreadLocal(ThreadLocalInfo info); + /** + * Generates a descriptive message about the ThreadLocal's usage pattern. + * Implementations should provide context-specific messages. + * It is used to receive the Markers message and the Data Tables detailed message. + * + * @param info The ThreadLocalInfo containing usage details. + * @return A string message describing the ThreadLocal's usage. + */ protected abstract String getMessage(ThreadLocalInfo info); + /** + * Determines the mutation type of the ThreadLocal based on its usage info. + * Implementations should define the mutation categories. + * It is used to populate the Data Tables human-readable mutation type column. + * + * @param info The ThreadLocalInfo containing usage details. + * @return A string representing the mutation type. + */ protected abstract String getMutationType(ThreadLocalInfo info); protected static boolean isThreadLocalType(@Nullable JavaType type) { diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java b/src/main/java/org/openrewrite/java/migrate/search/threadlocal/FindNeverMutatedThreadLocals.java similarity index 95% rename from src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java rename to src/main/java/org/openrewrite/java/migrate/search/threadlocal/FindNeverMutatedThreadLocals.java index eccdc18d69..728040176b 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java +++ b/src/main/java/org/openrewrite/java/migrate/search/threadlocal/FindNeverMutatedThreadLocals.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.openrewrite.java.migrate.search; +package org.openrewrite.java.migrate.search.threadlocal; import lombok.EqualsAndHashCode; import lombok.Value; @@ -46,7 +46,7 @@ public Set getTags() { @Override protected boolean shouldMarkThreadLocal(ThreadLocalInfo info) { // Mark ThreadLocals that have no mutations at all - return !info.hasAnyMutation(); + return info.hasNoMutation(); } @Override diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutside.java b/src/main/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutableFromOutside.java similarity index 86% rename from src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutside.java rename to src/main/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutableFromOutside.java index 372bdd9bac..b6c0909640 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutside.java +++ b/src/main/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutableFromOutside.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.openrewrite.java.migrate.search; +package org.openrewrite.java.migrate.search.threadlocal; import lombok.EqualsAndHashCode; import lombok.Value; @@ -33,6 +33,7 @@ public String getDisplayName() { @Override public String getDescription() { + //language=markdown return "Find `ThreadLocal` variables that can be mutated from outside their defining class. " + "These ThreadLocals have the highest risk as they can be modified by any code with access to them. " + "This includes non-private ThreadLocals or those mutated from other classes in the codebase."; @@ -55,19 +56,19 @@ protected boolean shouldMarkThreadLocal(ThreadLocalInfo info) { protected String getMessage(ThreadLocalInfo info) { if (info.hasExternalMutations()) { return "ThreadLocal is mutated from outside its defining class"; - } else { - // Non-private but not currently mutated externally - String access = info.isStatic() ? "static " : ""; - return "ThreadLocal is " + access + "non-private and can potentially be mutated from outside"; } + + // Non-private but not currently mutated externally + String access = info.isStatic() ? "static " : ""; + return "ThreadLocal is " + access + "non-private and can potentially be mutated from outside"; } @Override protected String getMutationType(ThreadLocalInfo info) { if (info.hasExternalMutations()) { return "Mutated externally"; - } else { - return "Potentially mutable"; } + + return "Potentially mutable"; } } diff --git a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java b/src/main/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutatedOnlyInDefiningScope.java similarity index 78% rename from src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java rename to src/main/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutatedOnlyInDefiningScope.java index a25b15e6f8..844b5c88da 100644 --- a/src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java +++ b/src/main/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutatedOnlyInDefiningScope.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.openrewrite.java.migrate.search; +package org.openrewrite.java.migrate.search.threadlocal; import lombok.EqualsAndHashCode; import lombok.Value; @@ -33,6 +33,7 @@ public String getDisplayName() { @Override public String getDescription() { + //language=markdown return "Find `ThreadLocal` variables that are only mutated within their defining class or initialization context (constructor/static initializer). " + "These may be candidates for refactoring as they have limited mutation scope. " + "The recipe identifies `ThreadLocal` variables that are only modified during initialization or within their declaring class."; @@ -45,29 +46,30 @@ public Set getTags() { @Override protected boolean shouldMarkThreadLocal(ThreadLocalInfo info) { - // Early return for ThreadLocals without mutations - if (!info.hasAnyMutation()) { + if (info.hasNoMutation()) { return false; } - // Early return for non-private ThreadLocals if (!info.isPrivate()) { return false; } - // Mark if only locally mutated return info.isOnlyLocallyMutated(); } @Override protected String getMessage(ThreadLocalInfo info) { - return info.hasOnlyInitMutations() - ? "ThreadLocal is only mutated during initialization (constructor/static initializer)" - : "ThreadLocal is only mutated within its defining class"; + if (info.hasOnlyInitMutations()) { + return "ThreadLocal is only mutated during initialization (constructor/static initializer)"; + } + + return "ThreadLocal is only mutated within its defining class"; } @Override protected String getMutationType(ThreadLocalInfo info) { - return info.hasOnlyInitMutations() - ? "Mutated only in initialization" - : "Mutated in defining class"; + if (info.hasOnlyInitMutations()) { + return "Mutated only in initialization"; + } + + return "Mutated in defining class"; } } diff --git a/src/main/java/org/openrewrite/java/migrate/search/threadlocal/package-info.java b/src/main/java/org/openrewrite/java/migrate/search/threadlocal/package-info.java new file mode 100644 index 0000000000..34bcf91372 --- /dev/null +++ b/src/main/java/org/openrewrite/java/migrate/search/threadlocal/package-info.java @@ -0,0 +1,21 @@ +/* + * Copyright 2024 the original author or authors. + *

+ * Licensed under the Moderne Source Available License (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + *

+ * https://docs.moderne.io/licensing/moderne-source-available-license + *

+ * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +@NullMarked +@NonNullFields +package org.openrewrite.java.migrate.search.threadlocal; + +import org.jspecify.annotations.NullMarked; +import org.openrewrite.internal.lang.NonNullFields; diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocalsCrossFileTest.java b/src/test/java/org/openrewrite/java/migrate/search/threadlocal/FindNeverMutatedThreadLocalsCrossFileTest.java similarity index 99% rename from src/test/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocalsCrossFileTest.java rename to src/test/java/org/openrewrite/java/migrate/search/threadlocal/FindNeverMutatedThreadLocalsCrossFileTest.java index 1984076117..d19906a99d 100644 --- a/src/test/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocalsCrossFileTest.java +++ b/src/test/java/org/openrewrite/java/migrate/search/threadlocal/FindNeverMutatedThreadLocalsCrossFileTest.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.openrewrite.java.migrate.search; +package org.openrewrite.java.migrate.search.threadlocal; import org.junit.jupiter.api.Test; import org.openrewrite.DocumentExample; diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocalsTest.java b/src/test/java/org/openrewrite/java/migrate/search/threadlocal/FindNeverMutatedThreadLocalsTest.java similarity index 99% rename from src/test/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocalsTest.java rename to src/test/java/org/openrewrite/java/migrate/search/threadlocal/FindNeverMutatedThreadLocalsTest.java index a33f49f7bb..03f2add0a1 100644 --- a/src/test/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocalsTest.java +++ b/src/test/java/org/openrewrite/java/migrate/search/threadlocal/FindNeverMutatedThreadLocalsTest.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.openrewrite.java.migrate.search; +package org.openrewrite.java.migrate.search.threadlocal; import org.junit.jupiter.api.Test; import org.openrewrite.DocumentExample; diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutsideTest.java b/src/test/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutableFromOutsideTest.java similarity index 99% rename from src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutsideTest.java rename to src/test/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutableFromOutsideTest.java index 3ef34db3f1..56308de926 100644 --- a/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutsideTest.java +++ b/src/test/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutableFromOutsideTest.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.openrewrite.java.migrate.search; +package org.openrewrite.java.migrate.search.threadlocal; import org.junit.jupiter.api.Test; import org.openrewrite.DocumentExample; diff --git a/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java b/src/test/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java similarity index 99% rename from src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java rename to src/test/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java index ce7693ccb3..d496183db8 100644 --- a/src/test/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java +++ b/src/test/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutatedOnlyInDefiningScopeTest.java @@ -13,7 +13,7 @@ * See the License for the specific language governing permissions and * limitations under the License. */ -package org.openrewrite.java.migrate.search; +package org.openrewrite.java.migrate.search.threadlocal; import org.junit.jupiter.api.Test; import org.openrewrite.DocumentExample;