Skip to content

Commit 9d2a798

Browse files
MBoegersMBoegers
committed
apply code review
1 parent 88d6365 commit 9d2a798

File tree

9 files changed

+130
-75
lines changed

9 files changed

+130
-75
lines changed

src/main/java/org/openrewrite/java/migrate/search/AbstractFindThreadLocals.java renamed to src/main/java/org/openrewrite/java/migrate/search/threadlocal/AbstractFindThreadLocals.java

Lines changed: 82 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,9 @@
1313
* See the License for the specific language governing permissions and
1414
* limitations under the License.
1515
*/
16-
package org.openrewrite.java.migrate.search;
16+
package org.openrewrite.java.migrate.search.threadlocal;
1717

18+
import lombok.Getter;
1819
import lombok.Value;
1920
import org.jspecify.annotations.Nullable;
2021
import org.openrewrite.ExecutionContext;
@@ -23,6 +24,7 @@
2324
import org.openrewrite.TreeVisitor;
2425
import org.openrewrite.java.JavaIsoVisitor;
2526
import org.openrewrite.java.MethodMatcher;
27+
import org.openrewrite.java.migrate.search.ThreadLocalTable;
2628
import org.openrewrite.java.search.UsesType;
2729
import org.openrewrite.java.tree.Expression;
2830
import org.openrewrite.java.tree.J;
@@ -33,7 +35,9 @@
3335
import java.nio.file.Path;
3436
import java.util.*;
3537

36-
import static org.openrewrite.Preconditions.*;
38+
import static org.openrewrite.Preconditions.check;
39+
import static org.openrewrite.Preconditions.or;
40+
3741

3842
public abstract class AbstractFindThreadLocals extends ScanningRecipe<AbstractFindThreadLocals.ThreadLocalAccumulator> {
3943

@@ -45,8 +49,7 @@ public abstract class AbstractFindThreadLocals extends ScanningRecipe<AbstractFi
4549
private static final MethodMatcher INHERITABLE_THREAD_LOCAL_SET = new MethodMatcher(INHERITED_THREAD_LOCAL_FQN + " set(..)");
4650
private static final MethodMatcher INHERITABLE_THREAD_LOCAL_REMOVE = new MethodMatcher(INHERITED_THREAD_LOCAL_FQN + " remove()");
4751

48-
@Nullable
49-
protected transient ThreadLocalTable dataTable;
52+
transient ThreadLocalTable dataTable = new ThreadLocalTable(this);
5053

5154
@Value
5255
public static class ThreadLocalAccumulator {
@@ -77,9 +80,13 @@ public boolean hasDeclarations() {
7780

7881
public static class ThreadLocalInfo {
7982
private @Nullable Path declarationPath;
83+
@Getter
8084
private boolean isPrivate;
85+
@Getter
8186
private boolean isStatic;
87+
@Getter
8288
private boolean isFinal;
89+
@Getter
8390
private boolean declared;
8491
private final Set<Path> initMutationPaths = new HashSet<>();
8592
private final Set<Path> regularMutationPaths = new HashSet<>();
@@ -92,38 +99,37 @@ void setDeclaration(Path path, boolean priv, boolean stat, boolean fin) {
9299
this.declared = true;
93100
}
94101

102+
/**
103+
* Records a mutation from an initialization context (constructor/static initializer).
104+
*/
95105
void addInitMutation(Path path) {
96106
initMutationPaths.add(path);
97107
}
98108

109+
/**
110+
* Records a regular (non-initialization) mutation.
111+
*/
99112
void addRegularMutation(Path path) {
100113
regularMutationPaths.add(path);
101114
}
102115

103-
public boolean isPrivate() {
104-
return isPrivate;
105-
}
106-
107-
public boolean isStatic() {
108-
return isStatic;
109-
}
110-
111-
public boolean isFinal() {
112-
return isFinal;
113-
}
114-
115-
public boolean isDeclared() {
116-
return declared;
117-
}
118-
119-
public boolean hasAnyMutation() {
120-
return !initMutationPaths.isEmpty() || !regularMutationPaths.isEmpty();
116+
/**
117+
* Checks if there are no mutations (both init and regular).
118+
*/
119+
public boolean hasNoMutation() {
120+
return initMutationPaths.isEmpty() && regularMutationPaths.isEmpty();
121121
}
122122

123+
/**
124+
* Checks if there are only mutations from initialization contexts (constructors/static initializers).
125+
*/
123126
public boolean hasOnlyInitMutations() {
124127
return !initMutationPaths.isEmpty() && regularMutationPaths.isEmpty();
125128
}
126129

130+
/**
131+
* Checks if there are any mutations (both init and regular) from files other than the declaration file.
132+
*/
127133
public boolean hasExternalMutations() {
128134
if (!declared || declarationPath == null) {
129135
return true; // Conservative
@@ -134,6 +140,9 @@ public boolean hasExternalMutations() {
134140
regularMutationPaths.stream().anyMatch(p -> !p.equals(declarationPath));
135141
}
136142

143+
/**
144+
* Checks if all mutations (both init and regular) are from the same file as the declaration.
145+
*/
137146
public boolean isOnlyLocallyMutated() {
138147
if (!declared || declarationPath == null) {
139148
return false;
@@ -208,7 +217,7 @@ public J.MethodInvocation visitMethodInvocation(J.MethodInvocation method, Execu
208217
return method;
209218
}
210219

211-
@Nullable String fqn = getFieldFullyQualifiedName(method.getSelect());
220+
String fqn = getFieldFullyQualifiedName(method.getSelect());
212221
if (fqn == null) {
213222
return method;
214223
}
@@ -228,7 +237,7 @@ public J.Assignment visitAssignment(J.Assignment assignment, ExecutionContext ct
228237
return assignment;
229238
}
230239

231-
@Nullable String fqn = getFieldFullyQualifiedName(assignment.getVariable());
240+
String fqn = getFieldFullyQualifiedName(assignment.getVariable());
232241
if (fqn == null) {
233242
return assignment;
234243
}
@@ -256,9 +265,10 @@ private boolean isInInitializationContext() {
256265

257266
private boolean isThreadLocalFieldAccess(Expression expression) {
258267
if (expression instanceof J.Identifier) {
259-
return isThreadLocalType(((J.Identifier) expression).getType());
260-
} else if (expression instanceof J.FieldAccess) {
261-
return isThreadLocalType(((J.FieldAccess) expression).getType());
268+
return isThreadLocalType(expression.getType());
269+
}
270+
if (expression instanceof J.FieldAccess) {
271+
return isThreadLocalType(expression.getType());
262272
}
263273
return false;
264274
}
@@ -279,7 +289,7 @@ private boolean isThreadLocalFieldAccess(Expression expression) {
279289
return null;
280290
}
281291

282-
@Nullable JavaType owner = varType.getOwner();
292+
JavaType owner = varType.getOwner();
283293
if (!(owner instanceof JavaType.FullyQualified)) {
284294
return null;
285295
}
@@ -294,22 +304,19 @@ private boolean isThreadLocalFieldAccess(Expression expression) {
294304
public TreeVisitor<?, ExecutionContext> getVisitor(ThreadLocalAccumulator acc) {
295305
return check(acc.hasDeclarations(),
296306
new JavaIsoVisitor<ExecutionContext>() {
297-
@Override
298-
public J.CompilationUnit visitCompilationUnit(J.CompilationUnit cu, ExecutionContext ctx) {
299-
if (dataTable == null) {
300-
dataTable = new ThreadLocalTable(AbstractFindThreadLocals.this);
301-
}
302-
return super.visitCompilationUnit(cu, ctx);
303-
}
307+
304308

305309
@Override
306310
public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations multiVariable, ExecutionContext ctx) {
307311
multiVariable = super.visitVariableDeclarations(multiVariable, ctx);
308312

309313
J.ClassDeclaration classDecl = getCursor().firstEnclosing(J.ClassDeclaration.class);
314+
if(classDecl == null) {
315+
return multiVariable;
316+
}
310317

311318
for (J.VariableDeclarations.NamedVariable variable : multiVariable.getVariables()) {
312-
if (isThreadLocalType(variable.getType()) && classDecl != null) {
319+
if (isThreadLocalType(variable.getType())) {
313320
String className = classDecl.getType() != null ?
314321
classDecl.getType().getFullyQualifiedName() : "UnknownClass";
315322
String fieldName = variable.getName().getSimpleName();
@@ -320,18 +327,15 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m
320327
String message = getMessage(info);
321328
String mutationType = getMutationType(info);
322329

323-
// Add to data table
324-
if (dataTable != null) {
325-
dataTable.insertRow(ctx, new ThreadLocalTable.Row(
326-
getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath().toString(),
327-
className,
328-
fieldName,
329-
getAccessModifier(multiVariable),
330-
getModifiers(multiVariable),
331-
mutationType,
332-
message
333-
));
334-
}
330+
dataTable.insertRow(ctx, new ThreadLocalTable.Row(
331+
getCursor().firstEnclosingOrThrow(SourceFile.class).getSourcePath().toString(),
332+
className,
333+
fieldName,
334+
getAccessModifier(multiVariable),
335+
getFieldModifiers(multiVariable),
336+
mutationType,
337+
message
338+
));
335339

336340
return SearchResult.found(multiVariable, message);
337341
}
@@ -344,15 +348,17 @@ public J.VariableDeclarations visitVariableDeclarations(J.VariableDeclarations m
344348
private String getAccessModifier(J.VariableDeclarations variableDecls) {
345349
if (variableDecls.hasModifier(J.Modifier.Type.Private)) {
346350
return "private";
347-
} else if (variableDecls.hasModifier(J.Modifier.Type.Protected)) {
351+
}
352+
if (variableDecls.hasModifier(J.Modifier.Type.Protected)) {
348353
return "protected";
349-
} else if (variableDecls.hasModifier(J.Modifier.Type.Public)) {
354+
}
355+
if (variableDecls.hasModifier(J.Modifier.Type.Public)) {
350356
return "public";
351357
}
352358
return "package-private";
353359
}
354360

355-
private String getModifiers(J.VariableDeclarations variableDecls) {
361+
private String getFieldModifiers(J.VariableDeclarations variableDecls) {
356362
List<String> mods = new ArrayList<>();
357363
if (variableDecls.hasModifier(J.Modifier.Type.Static)) {
358364
mods.add("static");
@@ -366,8 +372,33 @@ private String getModifiers(J.VariableDeclarations variableDecls) {
366372
});
367373
}
368374

375+
/**
376+
* Determines whether a ThreadLocal should be marked based on its usage info.
377+
* Implementations should define the criteria for marking.
378+
* It is used to decide if a ThreadLocal variable should be highlighted in the results.
379+
* If an expected ThreadLocal instance is missing from the results, consider adjusting this method.
380+
*
381+
* @param info The ThreadLocalInfo containing usage details.
382+
* @return true if the ThreadLocal should be marked, false otherwise.
383+
*/
369384
protected abstract boolean shouldMarkThreadLocal(ThreadLocalInfo info);
385+
/**
386+
* Generates a descriptive message about the ThreadLocal's usage pattern.
387+
* Implementations should provide context-specific messages.
388+
* It is used to receive the Markers message and the Data Tables detailed message.
389+
*
390+
* @param info The ThreadLocalInfo containing usage details.
391+
* @return A string message describing the ThreadLocal's usage.
392+
*/
370393
protected abstract String getMessage(ThreadLocalInfo info);
394+
/**
395+
* Determines the mutation type of the ThreadLocal based on its usage info.
396+
* Implementations should define the mutation categories.
397+
* It is used to populate the Data Tables human-readable mutation type column.
398+
*
399+
* @param info The ThreadLocalInfo containing usage details.
400+
* @return A string representing the mutation type.
401+
*/
371402
protected abstract String getMutationType(ThreadLocalInfo info);
372403

373404
protected static boolean isThreadLocalType(@Nullable JavaType type) {

src/main/java/org/openrewrite/java/migrate/search/FindNeverMutatedThreadLocals.java renamed to src/main/java/org/openrewrite/java/migrate/search/threadlocal/FindNeverMutatedThreadLocals.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
* See the License for the specific language governing permissions and
1414
* limitations under the License.
1515
*/
16-
package org.openrewrite.java.migrate.search;
16+
package org.openrewrite.java.migrate.search.threadlocal;
1717

1818
import lombok.EqualsAndHashCode;
1919
import lombok.Value;
@@ -46,7 +46,7 @@ public Set<String> getTags() {
4646
@Override
4747
protected boolean shouldMarkThreadLocal(ThreadLocalInfo info) {
4848
// Mark ThreadLocals that have no mutations at all
49-
return !info.hasAnyMutation();
49+
return info.hasNoMutation();
5050
}
5151

5252
@Override

src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutableFromOutside.java renamed to src/main/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutableFromOutside.java

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
* See the License for the specific language governing permissions and
1414
* limitations under the License.
1515
*/
16-
package org.openrewrite.java.migrate.search;
16+
package org.openrewrite.java.migrate.search.threadlocal;
1717

1818
import lombok.EqualsAndHashCode;
1919
import lombok.Value;
@@ -33,6 +33,7 @@ public String getDisplayName() {
3333

3434
@Override
3535
public String getDescription() {
36+
//language=markdown
3637
return "Find `ThreadLocal` variables that can be mutated from outside their defining class. " +
3738
"These ThreadLocals have the highest risk as they can be modified by any code with access to them. " +
3839
"This includes non-private ThreadLocals or those mutated from other classes in the codebase.";
@@ -55,19 +56,19 @@ protected boolean shouldMarkThreadLocal(ThreadLocalInfo info) {
5556
protected String getMessage(ThreadLocalInfo info) {
5657
if (info.hasExternalMutations()) {
5758
return "ThreadLocal is mutated from outside its defining class";
58-
} else {
59-
// Non-private but not currently mutated externally
60-
String access = info.isStatic() ? "static " : "";
61-
return "ThreadLocal is " + access + "non-private and can potentially be mutated from outside";
6259
}
60+
61+
// Non-private but not currently mutated externally
62+
String access = info.isStatic() ? "static " : "";
63+
return "ThreadLocal is " + access + "non-private and can potentially be mutated from outside";
6364
}
6465

6566
@Override
6667
protected String getMutationType(ThreadLocalInfo info) {
6768
if (info.hasExternalMutations()) {
6869
return "Mutated externally";
69-
} else {
70-
return "Potentially mutable";
7170
}
71+
72+
return "Potentially mutable";
7273
}
7374
}

src/main/java/org/openrewrite/java/migrate/search/FindThreadLocalsMutatedOnlyInDefiningScope.java renamed to src/main/java/org/openrewrite/java/migrate/search/threadlocal/FindThreadLocalsMutatedOnlyInDefiningScope.java

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
* See the License for the specific language governing permissions and
1414
* limitations under the License.
1515
*/
16-
package org.openrewrite.java.migrate.search;
16+
package org.openrewrite.java.migrate.search.threadlocal;
1717

1818
import lombok.EqualsAndHashCode;
1919
import lombok.Value;
@@ -33,6 +33,7 @@ public String getDisplayName() {
3333

3434
@Override
3535
public String getDescription() {
36+
//language=markdown
3637
return "Find `ThreadLocal` variables that are only mutated within their defining class or initialization context (constructor/static initializer). " +
3738
"These may be candidates for refactoring as they have limited mutation scope. " +
3839
"The recipe identifies `ThreadLocal` variables that are only modified during initialization or within their declaring class.";
@@ -45,29 +46,30 @@ public Set<String> getTags() {
4546

4647
@Override
4748
protected boolean shouldMarkThreadLocal(ThreadLocalInfo info) {
48-
// Early return for ThreadLocals without mutations
49-
if (!info.hasAnyMutation()) {
49+
if (info.hasNoMutation()) {
5050
return false;
5151
}
52-
// Early return for non-private ThreadLocals
5352
if (!info.isPrivate()) {
5453
return false;
5554
}
56-
// Mark if only locally mutated
5755
return info.isOnlyLocallyMutated();
5856
}
5957

6058
@Override
6159
protected String getMessage(ThreadLocalInfo info) {
62-
return info.hasOnlyInitMutations()
63-
? "ThreadLocal is only mutated during initialization (constructor/static initializer)"
64-
: "ThreadLocal is only mutated within its defining class";
60+
if (info.hasOnlyInitMutations()) {
61+
return "ThreadLocal is only mutated during initialization (constructor/static initializer)";
62+
}
63+
64+
return "ThreadLocal is only mutated within its defining class";
6565
}
6666

6767
@Override
6868
protected String getMutationType(ThreadLocalInfo info) {
69-
return info.hasOnlyInitMutations()
70-
? "Mutated only in initialization"
71-
: "Mutated in defining class";
69+
if (info.hasOnlyInitMutations()) {
70+
return "Mutated only in initialization";
71+
}
72+
73+
return "Mutated in defining class";
7274
}
7375
}

0 commit comments

Comments
 (0)