feat: Add processor fixing sonar rule S4065_ThreadLocalWithInitial #706

MartinWitt · 2022-01-27T17:15:27Z

This code is more or less copied from laughing-train and worked on spoon already, see https://github.com/INRIA/spoon/pull/4540/files. I would be surprised if we find this sonar rule violation anywhere else.

MartinWitt · 2022-01-27T17:22:19Z

src/main/java/sorald/processor/ThreadLocalWithInitial.java

+            if (statement instanceof CtReturn) {
+                lambda.setExpression(getReturnStatement(statement));
+            } else {
+                lambda.setBody(initalValueMethod.getDeclaration().getBody());


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

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

algomaster99 · 2022-01-27T21:24:20Z

This rule does not show up on https://rules.sonarsource.com/ so I thought it was deprecated. However, their JIRA dashboard shows that the rule is still active.

MartinWitt · 2022-01-27T21:27:38Z

https://rules.sonarsource.com/java/RSPEC-4065 here is the link

algomaster99 · 2022-01-27T22:06:03Z

I was prepending the S in the URL, I think. Anyway, thanks for the PR. I can have a look at it soon.

algomaster99

Thanks for the PR, @MartinWitt !

I have posted some general thoughts about this PR as comments. Please have a look.

algomaster99 · 2022-01-28T16:37:57Z

src/main/java/sorald/processor/ThreadLocalWithInitial.java

+        if (isThreadLocalType(newClass)) {
+            CtClass<?> innerClass = newClass.getAnonymousClass();
+            if (hasNoFields(innerClass) && hasOnlyConstructorAndSingleMethod(innerClass)) {
+                Optional<CtExecutableReference<?>> initalValueMethod =
+                        findInitalValueMethod(innerClass);
+                if (initalValueMethod.isPresent()) {


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.

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

algomaster99 · 2022-01-28T17:03:05Z

src/main/java/sorald/processor/ThreadLocalWithInitial.java

+            if (statement instanceof CtReturn) {
+                lambda.setExpression(getReturnStatement(statement));
+            } else {
+                lambda.setBody(initalValueMethod.getDeclaration().getBody());


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

algomaster99

These are my final suggestions. I have commented on defensive programming and why we may opt-out here. Also, have a look at the refactoring comments.

algomaster99 · 2022-01-31T09:35:13Z

src/main/java/sorald/processor/ThreadLocalWithInitial.java

+        CtLambda<?> lambda = getFactory().createLambda();
+        if (initalValueMethod.getDeclaration().getBody().getStatements().size() == 1) {
+            CtStatement statement = initalValueMethod.getDeclaration().getBody().getStatement(0);
+            if (statement instanceof CtReturn) {


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

algomaster99 · 2022-01-31T09:38:12Z

src/main/java/sorald/processor/ThreadLocalWithInitial.java

+        if (hasNoFields(innerClass) && hasOnlyConstructorAndSingleMethod(innerClass)) {
+            Optional<CtExecutableReference<?>> initalValueMethod =
+                    findInitalValueMethod(innerClass);
+            if (initalValueMethod.isPresent()) {


I think we can also remove the .isPresent check because if it would not be present, then it would not be a violation.

Defensive programming is good only when we are unsure about the consequences. For example, we don't know what is supposed to happen when the anonymous class has more than 2 type members. So it is better to add a check for them as you have done above. Moreover, defensive programming reduces coverage as well so we should use it when we really need it.

algomaster99 · 2022-01-31T09:50:02Z

src/main/java/sorald/processor/ThreadLocalWithInitial.java

+        }
+    }
+
+    private CtInvocation<?> createInitalMethod(CtNewClass<?> threadLocal, CtLambda<?> lambda) {


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.

algomaster99 · 2022-01-31T09:54:36Z

src/main/java/sorald/processor/ThreadLocalWithInitial.java

+                .createInvocation(
+                        getFactory().createTypeAccess(createThreadLocalRef()),
+                        getFactory()
+                                .Executable()
+                                .createReference(
+                                        threadLocal.getType(),
+                                        true,
+                                        threadLocal.getType(),
+                                        "withInitial",
+                                        List.of(lambda.getType())));


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.

algomaster99 · 2022-01-31T09:59:05Z

src/main/java/sorald/processor/ThreadLocalWithInitial.java

+        if (initalValueMethod.getDeclaration().getBody().getStatements().size() == 1) {
+            CtStatement statement = initalValueMethod.getDeclaration().getBody().getStatement(0);
+            if (statement instanceof CtReturn) {
+                lambda.setExpression(getReturnStatement(statement));


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.

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.

khesoem · 2022-01-31T16:33:24Z

Thanks @MartinWitt a lot.

Per discussion with @algomaster99, we may not be able to fix some of the violations reported by Sonar. For example, if the anonymous class contains more than one (+ an implicit constructor) type members.

To make sure, we have to see if such violations are reported by Sonar. If they are, we can make this an incomplete processor.

MartinWitt · 2022-01-31T16:42:42Z

To make sure, we have to see if such violations are reported by Sonar.

Using sonar lint inside VS Code reports no violation if there exists more than a initialValue method, see screenshots.
Have you some infrastructure to test this against your sonar version?

No Warning

Warning

khesoem · 2022-01-31T16:44:56Z

Thanks @MartinWitt .

@algomaster99 already tested it, it reports no violation. We're good to continue...

algomaster99

I tested for what @khaes-kth said. It reports no violation.

I also suggested a change in the test files. I am sorry to propose these changes after I said the last ones were my final. 😅

algomaster99 · 2022-01-31T16:45:47Z

src/main/java/sorald/processor/ThreadLocalWithInitial.java

+    @Override
+    protected void repairInternal(CtNewClass<?> newClass) {
+        CtClass<?> innerClass = newClass.getAnonymousClass();
+        if (hasNoFields(innerClass) && hasOnlyConstructorAndSingleMethod(innerClass)) {


Apparently, we don't need these conditions as well. As @khaes-kth suggested, I checked if SonarLint reports a violation if there are more than 2 type members. It does not. For example, the following snippet does not report any sonar violation.

ThreadLocal<List<String>> myThreadLocal = new ThreadLocal<>() { private static final String X = "1"; @Override protected List<String> initialValue() { System.setProperty(X, "abc"); return new ArrayList<>(); } };

algomaster99 · 2022-01-31T16:48:13Z

src/test/resources/processor_test_files/S4065_ThreadLocalWithInitial/ThreadLocalInitial.java

@@ -0,0 +1,13 @@
+
+public class ThreadLocalInitial {


We can rename this, I think.

Suggested change

public class ThreadLocalInitial {

public class ThreadLocalInitialWithExactlyOneReturnStatement {

algomaster99 · 2022-01-31T16:48:40Z

src/test/resources/processor_test_files/S4065_ThreadLocalWithInitial/ThreadLocalInitial2.java

@@ -0,0 +1,14 @@
+
+public class ThreadLocalInitial2 {


We can rename this also.

Suggested change

public class ThreadLocalInitial2 {

public class ThreadLocalInitialWithMultipleStatements {

algomaster99 · 2022-01-31T16:50:36Z

@MartinWitt

Have you some infrastructure to test this against your sonar version?

Thanks for testing it! We just used SonarLint with IntelliJ.

algomaster99

Looks good to me!

algomaster99 · 2022-01-31T17:47:16Z

@khaes-kth you can add more comments or proceed to merge.

khesoem · 2022-01-31T17:50:07Z

Shouldn't we also update the handled rules doc?

algomaster99 · 2022-01-31T17:54:52Z

Shouldn't we also update the handled rules doc?

It will happen automatically once you merge this PR :) GHA will create a PR for that.

khesoem · 2022-01-31T17:57:17Z

src/main/java/sorald/processor/ThreadLocalWithInitial.java

+        newClass.replace(invocation);
+    }
+
+    private CtInvocation<?> createinitialMethod(CtNewClass<?> threadLocal, CtLambda<?> lambda) {


Shouldn't be createInitialMethod? (typo on first character of initial)

@MartinWitt I think I overlooked this while suggesting the typographical error.

khesoem · 2022-01-31T17:58:09Z

Shouldn't we also update the handled rules doc?

It will happen automatically once you merge this PR :) GHA will create a PR for that.

Cool. Didn't remember it.

khesoem

LGTM!

monperrus · 2022-02-01T13:26:36Z

super cool!

Add processor fixing sonar rule S4065_ThreadLocalWithInitial

c3d38b1

MartinWitt changed the title ~~Add processor fixing sonar rule S4065_ThreadLocalWithInitial~~ feat: Add processor fixing sonar rule S4065_ThreadLocalWithInitial Jan 27, 2022

MartinWitt commented Jan 27, 2022

View reviewed changes

algomaster99 requested changes Jan 28, 2022

View reviewed changes

MartinWitt added 2 commits January 30, 2022 00:35

apply requested changes

8bf69d2

fix formatting

63cccee

algomaster99 requested changes Jan 31, 2022

View reviewed changes

apply changes

688eab9

algomaster99 requested changes Jan 31, 2022

View reviewed changes

rename files and remove check

3a15259

algomaster99 approved these changes Jan 31, 2022

View reviewed changes

khesoem reviewed Jan 31, 2022

View reviewed changes

fix method name

79eea1b

khesoem approved these changes Jan 31, 2022

View reviewed changes

khesoem merged commit c11c1bb into ASSERT-KTH:master Jan 31, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add processor fixing sonar rule S4065_ThreadLocalWithInitial #706

feat: Add processor fixing sonar rule S4065_ThreadLocalWithInitial #706

MartinWitt commented Jan 27, 2022

MartinWitt Jan 27, 2022

algomaster99 Jan 28, 2022

algomaster99 commented Jan 27, 2022

MartinWitt commented Jan 27, 2022

algomaster99 commented Jan 27, 2022

algomaster99 left a comment

algomaster99 Jan 28, 2022

MartinWitt Jan 29, 2022

algomaster99 Jan 28, 2022

algomaster99 left a comment

algomaster99 Jan 31, 2022

algomaster99 Jan 31, 2022

algomaster99 Jan 31, 2022

algomaster99 Jan 31, 2022

algomaster99 Jan 31, 2022

MartinWitt Jan 31, 2022

khesoem commented Jan 31, 2022

MartinWitt commented Jan 31, 2022

khesoem commented Jan 31, 2022

algomaster99 left a comment

algomaster99 Jan 31, 2022

algomaster99 Jan 31, 2022 •

edited

Loading

algomaster99 Jan 31, 2022

algomaster99 commented Jan 31, 2022 •

edited

Loading

algomaster99 left a comment

algomaster99 commented Jan 31, 2022

khesoem commented Jan 31, 2022

algomaster99 commented Jan 31, 2022 •

edited

Loading

khesoem Jan 31, 2022

algomaster99 Jan 31, 2022

khesoem commented Jan 31, 2022

khesoem left a comment

monperrus commented Feb 1, 2022

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

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

	public class ThreadLocalInitial {
	public class ThreadLocalInitialWithExactlyOneReturnStatement {

	public class ThreadLocalInitial2 {
	public class ThreadLocalInitialWithMultipleStatements {

feat: Add processor fixing sonar rule S4065_ThreadLocalWithInitial #706

feat: Add processor fixing sonar rule S4065_ThreadLocalWithInitial #706

Conversation

MartinWitt commented Jan 27, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algomaster99 commented Jan 27, 2022

MartinWitt commented Jan 27, 2022

algomaster99 commented Jan 27, 2022

algomaster99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algomaster99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khesoem commented Jan 31, 2022

MartinWitt commented Jan 31, 2022

No Warning

Warning

khesoem commented Jan 31, 2022

algomaster99 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algomaster99 Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

algomaster99 commented Jan 31, 2022 • edited Loading

algomaster99 left a comment

Choose a reason for hiding this comment

algomaster99 commented Jan 31, 2022

khesoem commented Jan 31, 2022

algomaster99 commented Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

khesoem commented Jan 31, 2022

khesoem left a comment

Choose a reason for hiding this comment

monperrus commented Feb 1, 2022

algomaster99 Jan 31, 2022 •

edited

Loading

algomaster99 commented Jan 31, 2022 •

edited

Loading

algomaster99 commented Jan 31, 2022 •

edited

Loading