Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add processor fixing sonar rule S4065_ThreadLocalWithInitial #706

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion src/main/java/sorald/Processors.java
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,14 @@ private Processors() {}
put("S3032", GetClassLoaderProcessor.class);
put("S3067", SynchronizationOnGetClassProcessor.class);
put("S3984", UnusedThrowableProcessor.class);
put("S4065", ThreadLocalWithInitial.class);
put("S4973", CompareStringsBoxedTypesWithEqualsProcessor.class);
}
};

// GENERATED FIELD
public static final String RULE_DESCRIPTIONS =
"S1068: Unused \"private\" fields should be removed\nS1118: Utility classes should not have public constructors\n\t(incomplete: Only handles implicit public constructor)\nS1132: Strings literals should be placed on the left side when checking for equality\nS1155: Collection.isEmpty() should be used to test for emptiness\nS1217: \"Thread.run()\" should not be called directly\nS1444: \"public static\" fields should be constant\n\t(incomplete: does not fix variable naming)\nS1481: Unused local variables should be removed\nS1596: \"Collections.EMPTY_LIST\", \"EMPTY_MAP\", and \"EMPTY_SET\" should not be used\nS1656: Variables should not be self-assigned\nS1854: Unused assignments should be removed\nS1860: Synchronization should not be based on Strings or boxed primitives\nS1948: Fields in a \"Serializable\" class should either be transient or serializable\nS2057: Every class implementing Serializable should declare a static final serialVersionUID.\n\t(incomplete: This processor does not address the case where the class already has a serialVersionUID with a non long type.)\nS2095: Resources should be closed\nS2097: \"equals(Object obj)\" should test argument type\nS2111: \"BigDecimal(double)\" should not be used\nS2116: \"hashCode\" and \"toString\" should not be called on array instances\nS2142: \"InterruptedException\" should not be ignored\nS2164: Math should not be performed on floats\nS2167: \"compareTo\" should not return \"Integer.MIN_VALUE\"\nS2184: Math operands should be cast before assignment\nS2204: \".equals()\" should not be used to test the values of \"Atomic\" classes\nS2225: \"toString()\" and \"clone()\" methods should not return null\n\t(incomplete: does not fix null returning clone())\nS2272: \"Iterator.next()\" methods should throw \"NoSuchElementException\"\nS2755: XML parsers should not be vulnerable to XXE attacks\n\t(incomplete: This processor is a WIP and currently supports a subset of rule 2755. See Sorald\'s documentation for details.)\nS3032: JEE applications should not \"getClassLoader\"\nS3067: \"getClass\" should not be used for synchronization\nS3984: Exception should not be created without being thrown\nS4973: Strings and Boxed types should be compared using \"equals()\"";
"S1068: Unused \"private\" fields should be removed\nS1118: Utility classes should not have public constructors\n\t(incomplete: Only handles implicit public constructor)\nS1132: Strings literals should be placed on the left side when checking for equality\nS1155: Collection.isEmpty() should be used to test for emptiness\nS1217: \"Thread.run()\" should not be called directly\nS1444: \"public static\" fields should be constant\n\t(incomplete: does not fix variable naming)\nS1481: Unused local variables should be removed\nS1596: \"Collections.EMPTY_LIST\", \"EMPTY_MAP\", and \"EMPTY_SET\" should not be used\nS1656: Variables should not be self-assigned\nS1854: Unused assignments should be removed\nS1860: Synchronization should not be based on Strings or boxed primitives\nS1948: Fields in a \"Serializable\" class should either be transient or serializable\nS2057: Every class implementing Serializable should declare a static final serialVersionUID.\n\t(incomplete: This processor does not address the case where the class already has a serialVersionUID with a non long type.)\nS2095: Resources should be closed\nS2097: \"equals(Object obj)\" should test argument type\nS2111: \"BigDecimal(double)\" should not be used\nS2116: \"hashCode\" and \"toString\" should not be called on array instances\nS2142: \"InterruptedException\" should not be ignored\nS2164: Math should not be performed on floats\nS2167: \"compareTo\" should not return \"Integer.MIN_VALUE\"\nS2184: Math operands should be cast before assignment\nS2204: \".equals()\" should not be used to test the values of \"Atomic\" classes\nS2225: \"toString()\" and \"clone()\" methods should not return null\n\t(incomplete: does not fix null returning clone())\nS2272: \"Iterator.next()\" methods should throw \"NoSuchElementException\"\nS2755: XML parsers should not be vulnerable to XXE attacks\n\t(incomplete: This processor is a WIP and currently supports a subset of rule 2755. See Sorald\'s documentation for details.)\nS3032: JEE applications should not \"getClassLoader\"\nS3067: \"getClass\" should not be used for synchronization\nS3984: Exception should not be created without being thrown\nS4065: \"ThreadLocal.withInitial\" should be preferred\nS4973: Strings and Boxed types should be compared using \"equals()\"";

public static Class<? extends SoraldAbstractProcessor<?>> getProcessor(String key) {
return RULE_KEY_TO_PROCESSOR.get(key);
Expand Down
95 changes: 95 additions & 0 deletions src/main/java/sorald/processor/ThreadLocalWithInitial.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,95 @@
package sorald.processor;

import java.util.List;
import java.util.Optional;
import java.util.function.Supplier;
import sorald.annotations.ProcessorAnnotation;
import spoon.reflect.code.CtExpression;
import spoon.reflect.code.CtInvocation;
import spoon.reflect.code.CtLambda;
import spoon.reflect.code.CtNewClass;
import spoon.reflect.code.CtReturn;
import spoon.reflect.code.CtStatement;
import spoon.reflect.declaration.CtClass;
import spoon.reflect.reference.CtExecutableReference;
import spoon.reflect.reference.CtTypeReference;

@ProcessorAnnotation(key = "S4065", description = "\"ThreadLocal.withInitial\" should be preferred")
public class ThreadLocalWithInitial extends SoraldAbstractProcessor<CtNewClass<?>> {

private static final String THREADLOCAL_FQN = "java.lang.ThreadLocal";

@Override
protected void repairInternal(CtNewClass<?> newClass) {
if (isThreadLocalType(newClass)) {
CtClass<?> innerClass = newClass.getAnonymousClass();
if (hasNoFields(innerClass) && hasOnlyConstructorAndSingleMethod(innerClass)) {
Optional<CtExecutableReference<?>> initalValueMethod =
findInitalValueMethod(innerClass);
if (initalValueMethod.isPresent()) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove the if-conditions you have here. It is not necessary to check that the type of newClass is ThreadLocal because if it is not, SonarJava would never report a violation and we don't even need to visit this processor in that case.

Following the same logic as above, initialValueMethod must be present for the violation to be reported.

I am not sure what is the behaviour of SonarJava when there are more than one type members. If they report a violation, they have not illustrated how to fix it. So I am unsure if we need to invoke hasNoFields and hasOnlyConstructorAndSingleMethod.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would leave some defensive programming here because it does not harm.

CtLambda<?> lambda = createSupplier(initalValueMethod.get());
CtInvocation<?> invocation = createInitalMethod(newClass, lambda);
invocation.setArguments(List.of(lambda));
newClass.replace(invocation);
}
}
}
}

private boolean isThreadLocalType(CtNewClass<?> newClass) {
return newClass.getType() != null
&& newClass.getType().getQualifiedName().equals(THREADLOCAL_FQN);
}

private CtInvocation<?> createInitalMethod(CtNewClass<?> threadLocal, CtLambda<?> lambda) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private CtInvocation<?> createInitalMethod(CtNewClass<?> threadLocal, CtLambda<?> lambda) {
private CtInvocation<?> createInitialMethod(CtNewClass<?> threadLocal, CtLambda<?> lambda) {

There is a typographical error in the spelling of initial. This is just one instance. Please correct them everywhere.

return getFactory()
.createInvocation(
getFactory().createTypeAccess(createThreadLocalRef()),
getFactory()
.Executable()
.createReference(
threadLocal.getType(),
true,
threadLocal.getType(),
"withInitial",
List.of(lambda.getType())));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.createInvocation(
getFactory().createTypeAccess(createThreadLocalRef()),
getFactory()
.Executable()
.createReference(
threadLocal.getType(),
true,
threadLocal.getType(),
"withInitial",
List.of(lambda.getType())));
CtTypeAccess<?> target = getFactory().createTypeAccess(createThreadLocalRef());
CtExecutableReference<?> executableReference = getFactory()
.Executable()
.createReference(threadLocal.getType(), true, threadLocal.getType(), "withInitial", List.of(lambda.getType()));
return getFactory().createInvocation(target, executableReference);

Probably a refactoring like this is more readable? You could also create the reference to withInitial in a separate method.

}

private CtTypeReference<Object> createThreadLocalRef() {
return getFactory().createCtTypeReference(ThreadLocal.class);
}

private CtLambda<?> createSupplier(CtExecutableReference<?> initalValueMethod) {
CtLambda<?> lambda = getFactory().createLambda();
if (initalValueMethod.getDeclaration().getBody().getStatements().size() == 1) {
CtStatement statement = initalValueMethod.getDeclaration().getBody().getStatement(0);
if (statement instanceof CtReturn) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need this check. Supplier will have a return statement.

lambda.setExpression(getReturnStatement(statement));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
lambda.setExpression(getReturnStatement(statement));
lambda.setExpression(((CtReturn<T>) statement).getReturnedExpression());

In my opinion, this is better as is. No need to create another method for it, but it is up to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need this method for the type system, otherwise we would need to use raw types. Raw type is worse than an unchecked cast for me.

} else {
lambda.setBody(initalValueMethod.getDeclaration().getBody());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I'm unsure if there is a case where the method initalValue has one statement, and it is not a CtReturn

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it is possible. I think a Supplier needs to return something. At the least, return null should be there.

}
} else {
lambda.setBody(initalValueMethod.getDeclaration().getBody());
}
lambda.setType(getFactory().createCtTypeReference(Supplier.class));
return lambda;
}

private <T> CtExpression<T> getReturnStatement(CtStatement statement) {
return ((CtReturn<T>) statement).getReturnedExpression();
}

private Optional<CtExecutableReference<?>> findInitalValueMethod(CtClass<?> innerClass) {
return innerClass.getDeclaredExecutables().stream()
.filter(v -> v.getSimpleName().equals("initialValue"))
.findFirst();
}

private boolean hasOnlyConstructorAndSingleMethod(CtClass<?> innerClass) {
return innerClass.getDeclaredExecutables().size() == 2;
}

private boolean hasNoFields(CtClass<?> innerClass) {
return innerClass.getDeclaredFields().isEmpty();
}
}
10 changes: 10 additions & 0 deletions src/main/java/sorald/processor/ThreadLocalWithInitial.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Sorald fixes the violations of this rule by replacing an anonymous `ThreadLocal` class overriding `initalValue` with an invocation of `ThreadLocal.withInitial(Supplier)`.
Example:
```diff
- ThreadLocal<String> myThreadLocal = new ThreadLocal<String>() {
- @Override
- protected String initialValue() {
- return "Hello";
- }
+ Threadlocal<String> myThreadLocal = ThreadLocal.withInitial(() -> "Hello");
```
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@

public class ThreadLocalInitial {
Copy link
Member

@algomaster99 algomaster99 Jan 31, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can rename this, I think.

Suggested change
public class ThreadLocalInitial {
public class ThreadLocalInitialWithExactlyOneReturnStatement {


public void bar() {
ThreadLocal<String> threadLocal = new ThreadLocal<String>() { // Noncompliant
@Override
protected String initialValue() {
return "hello";
}
};
threadLocal.set("42");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@

public class ThreadLocalInitial2 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can rename this also.

Suggested change
public class ThreadLocalInitial2 {
public class ThreadLocalInitialWithMultipleStatements {


public void bar() {
ThreadLocal<String> threadLocal = new ThreadLocal<String>() { // Noncompliant
@Override
protected String initialValue() {
String s = "hello";
return s+"42";
}
};
threadLocal.set("42");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@

public class ThreadLocalInitial2 {

public void bar() {
ThreadLocal<String> threadLocal = ThreadLocal.withInitial(() -> {String s = "hello"; return s+"42";});
threadLocal.set("42");
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@

public class ThreadLocalInitial {

public void bar() {
ThreadLocal<String> threadLocal = ThreadLocal.withInitial(() -> "hello");
threadLocal.set("42");
}
}