Skip to content

Commit

Permalink
SONARJAVA-4770 S2438 FN on arguments whose concrete type is Thread (#…
Browse files Browse the repository at this point in the history 
…4704)
  • Loading branch information
@kaufco
kaufco authored Mar 26, 2024
1 parent 7152801 commit 55359e8
Show file tree
Hide file tree
Showing 3 changed files with 287 additions and 51 deletions.
184 changes: 181 additions & 3 deletions java-checks-test-sources/default/src/main/java/checks/ThreadAsRunnableArgumentCheck.java
View file
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
package checks;

import java.util.function.Supplier;

class ThreadAsRunnableArgumentCheck {

ThreadAsRunnableArgumentCheck (int i) {
Expand All @@ -7,7 +10,7 @@ class ThreadAsRunnableArgumentCheck {
public void foo() {
Thread t = new Thread() {
};
new Thread(t).start(); // Noncompliant [[sc=16;ec=17]] {{Replace "t" of type Thread with an instance of Runnable.}}
new Thread(t).start(); // Noncompliant [[sc=16;ec=17]] {{Replace this Thread instance with an instance of Runnable.}}

new Thread(bar()).start(); // Noncompliant [[sc=16;ec=21]]

Expand All @@ -28,7 +31,7 @@ public void run() {
m = new MyClass(0, new MyThread()); // Noncompliant
// Noncompliant@+1
m = new MyClass(0, myThread, r, new MyThread()); // Noncompliant because of arg1 and arg3
m = new MyClass(0, new Thread[] {myThread, new MyThread()}); // Noncompliant {{Replace "argument 2" of type Thread[] with an instance of Runnable[].}}
m = new MyClass(0, new Thread[] {myThread, new MyThread()}); // Noncompliant [[sc=24;ec=63]] {{Replace this Thread[] instance with an instance of Runnable[].}}
m = new MyClass(0); // Compliant
m = new MyClass(0, new Runnable[] {}); // Compliant
m = new MyClass(0, null, r, null); // Compliant
Expand Down Expand Up @@ -63,6 +66,181 @@ void foo(Runnable r) {
void qix(Runnable... runners) {
}
}
}

private void testCases() {
var l1 = new Thread(() -> {}); // Compliant
Runnable l2 = new Thread(() -> {}); // Noncompliant
Thread l3 = new Thread(() -> {}); // Compliant
Runnable l4;

l1 = new Thread(() -> {}); // Compliant
l2 = new Thread(() -> {}); // Noncompliant
fieldRunnable = new Thread(() -> {}); // Noncompliant
fieldThread = new Thread(() -> {}); // Compliant

foo1(); // Compliant
foo2(new Thread(() -> {})); // Noncompliant
foo3(new Thread(() -> {})); // Compliant
foo4(1,2, new String[0]); // Compliant
foo5(1,2, new Thread(() -> {}), new String[0]); // Compliant
foo6(1,2, new Thread(() -> {}), new String[0]); // Noncompliant

var foo = new Foo(1,2, new Thread(() -> {}), new String[0]); // Noncompliant

Thread[] threads1 = new Thread[]{}; // Compliant
Thread[] threads2 = new Thread[]{l1}; // Compliant
Thread[] threads3 = new Thread[]{l1, fieldThread}; // Compliant

Runnable[] runnables1 = new Thread[]{}; // Noncompliant
Runnable[] runnables2 = new Thread[]{l1}; // Noncompliant
Runnable[] runnables3 = new Thread[]{l1, fieldThread}; // Noncompliant

Runnable[] runnables4 = new Runnable[]{}; // Compliant
Runnable[] runnables5 = new Runnable[]{l1}; // Noncompliant
Runnable[] runnables6 = new Runnable[]{l1, fieldRunnable}; // Noncompliant

Runnable[] runnables7 = new Runnable[]{l2}; // Compliant
Runnable[] runnables8 = new Runnable[]{l2, fieldRunnable}; // Compliant
Runnable[] runnables9 = new Runnable[]{l2, fieldThread}; // Noncompliant

threads2[0] = l1; // Compliant
runnables7[0] = l1; // Noncompliant
runnables7[0] = l2; // Compliant

foo7(threads1); // Compliant
foo8(threads1); // Noncompliant
foo8(runnables1); // Compliant
foo2(threads1[0]); // Noncompliant
}

private Runnable fieldRunnable = new Thread(() -> {}); // Noncompliant

private Thread fieldThread = new Thread(() -> {}); // Compliant

private Runnable fieldRunnableUninitialized; // Compliant

private void foo1() {
}

private void foo2(Runnable r) {
}

private void foo3(Thread t) {
}

private void foo4(int a, int b, String[] c) {
}

private void foo5(int a, int b, Thread t, String[] c) {
}

private void foo6(int a, int b, Runnable r, String[] c) {
}

private void foo7(Thread[] threads) {
}

private void foo8(Runnable[] runnables) {
}

private static class Foo {
public Foo(int a, int b, Runnable r, String[] c) {
}
}
private int return1() {
return 42; // Compliant
}

private Thread return2() {
return new Thread(() -> {}); // Compliant
}

private Runnable return3() {
var a = new Thread(() -> {}); // Compliant
return a; // Noncompliant
}

private Runnable return4() {
Runnable a = new Thread(() -> {}); // Noncompliant
return a; // Compliant
}

private void return5() {
return; // Compliant
}

private void lambda1(Runnable r) {

Supplier<Thread> s1 = () -> {
return new Thread(() -> {}); // Compliant
};

Supplier<Runnable> s2 = () -> {
return new Thread(() -> {}); // Noncompliant
};

Supplier<Runnable> s3 = () -> {
return r; // Compliant
};
}

private void yield1(int code, Thread t, Runnable r) {
Runnable result1 = switch (code) {
case 0 -> t; // Noncompliant
case 1 -> t; // Noncompliant
default -> null;
};

Runnable result2 = switch (code) { // Compliant, switch type is Runnable
case 0 -> r;
case 1 -> r;
default -> null;
};

var result3 = switch (code) { // Compliant, switch type is something else
case 0 -> 42;
case 1 -> 23;
default -> 5;
};

Runnable result4 = switch (code) {
case 0 -> r;
case 1 -> t; // Noncompliant, common supertype is Runnable
default -> null;
};

Runnable result5 = switch (code) {
case 0 -> r;
case 1 -> {
System.out.println();
yield t; // Noncompliant, common supertype is Runnable
}
default -> null;
};

Runnable result6 = switch (code) {
case 0 -> r; // Compliant
case 1 -> {
System.out.println();
yield r; // Compliant
}
default -> null;
};
}

private void arraySubtypeAssign(MyThread[] mts) {
Runnable[] rs = mts; // Noncompliant
}

private void arrayInstantiationWithNulltype() {
String[] words = { "abc" };
}

private void switchWithYieldNulltype(int code) {
switch(code) {
default -> voidFun();
};
}

private void voidFun() {}
}
140 changes: 93 additions & 47 deletions java-checks/src/main/java/org/sonar/java/checks/ThreadAsRunnableArgumentCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,85 +19,131 @@
*/
package org.sonar.java.checks;

import java.text.MessageFormat;
import java.util.List;
import org.sonar.check.Rule;
import org.sonar.java.model.ExpressionUtils;
import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
import org.sonar.plugins.java.api.semantic.Symbol;
import org.sonar.plugins.java.api.semantic.Type;
import org.sonar.plugins.java.api.tree.Arguments;
import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
import org.sonar.plugins.java.api.tree.ExpressionTree;
import org.sonar.plugins.java.api.tree.IdentifierTree;
import org.sonar.plugins.java.api.tree.LambdaExpressionTree;
import org.sonar.plugins.java.api.tree.MethodInvocationTree;
import org.sonar.plugins.java.api.tree.MethodTree;
import org.sonar.plugins.java.api.tree.NewArrayTree;
import org.sonar.plugins.java.api.tree.NewClassTree;
import org.sonar.plugins.java.api.tree.ReturnStatementTree;
import org.sonar.plugins.java.api.tree.Tree;
import java.text.MessageFormat;
import java.util.Arrays;
import java.util.List;
import org.sonar.plugins.java.api.tree.VariableTree;
import org.sonar.plugins.java.api.tree.YieldStatementTree;

@Rule(key = "S2438")
public class ThreadAsRunnableArgumentCheck extends IssuableSubscriptionVisitor {

private static final String RUNNABLE_TYPE = "java.lang.Runnable";
private static final String THREAD_TYPE = "java.lang.Thread";
private static final String RUNNABLE_ARRAY_TYPE = RUNNABLE_TYPE + "[]";
private static final String THREAD_ARRAY_TYPE = THREAD_TYPE + "[]";

@Override
public List<Tree.Kind> nodesToVisit() {
return Arrays.asList(Tree.Kind.NEW_CLASS, Tree.Kind.METHOD_INVOCATION);
return List.of(Tree.Kind.VARIABLE, Tree.Kind.RETURN_STATEMENT, Tree.Kind.YIELD_STATEMENT, Tree.Kind.ASSIGNMENT,
Tree.Kind.METHOD_INVOCATION, Tree.Kind.NEW_CLASS, Tree.Kind.NEW_ARRAY);
}

@Override
public void visitNode(Tree tree) {
List<ExpressionTree> arguments;
Symbol.MethodSymbol methodSymbol;
if (tree.is(Tree.Kind.NEW_CLASS)) {
NewClassTree nct = (NewClassTree) tree;
methodSymbol = nct.methodSymbol();
arguments = nct.arguments();
} else {
MethodInvocationTree mit = (MethodInvocationTree) tree;
methodSymbol = mit.methodSymbol();
arguments = mit.arguments();
switch(tree.kind()) {
case VARIABLE -> visitVariable((VariableTree) tree);
case RETURN_STATEMENT -> visitReturnStatement((ReturnStatementTree) tree);
case YIELD_STATEMENT -> visitYieldStatement((YieldStatementTree) tree);
case NEW_ARRAY -> visitNewArray((NewArrayTree) tree);
case ASSIGNMENT -> {
var assignment = (AssignmentExpressionTree) tree;
checkTypeCoercion(assignment.variable().symbolType(), assignment.expression());
}
case METHOD_INVOCATION -> {
var invocation = (MethodInvocationTree) tree;
visitInvocation(invocation.methodSymbol(), invocation.arguments());
}
case NEW_CLASS -> {
var invocation = (NewClassTree) tree;
visitInvocation(invocation.methodSymbol(), invocation.arguments());
}
}
if (!arguments.isEmpty() && !methodSymbol.isUnknown()) {
checkArgumentsTypes(arguments, methodSymbol);
}

private void visitVariable(VariableTree tree) {
var initializer = tree.initializer();
if (initializer != null) {
checkTypeCoercion(tree.symbol().type(), initializer);
}
}

private void checkArgumentsTypes(List<ExpressionTree> arguments, Symbol.MethodSymbol methodSymbol) {
List<Type> parametersTypes = methodSymbol.parameterTypes();
// FIXME static imports.
// FIXME As arguments are not handled for method resolution using static imports, the provided methodSymbol may not match.
if (!parametersTypes.isEmpty()) {
for (int index = 0; index < arguments.size(); index++) {
ExpressionTree argument = arguments.get(index);
Type providedType = argument.symbolType();
if (!argument.is(Tree.Kind.NULL_LITERAL) && isThreadAsRunnable(providedType, parametersTypes, index, methodSymbol.isVarArgsMethod())) {
reportIssue(argument, getMessage(argument, providedType, index));
}
}
private void visitInvocation(Symbol.MethodSymbol methodSymbol, Arguments rhsValues) {
List<Type> lhsTypes = methodSymbol.parameterTypes();

var nonVarargCount = lhsTypes.size() - (methodSymbol.isVarArgsMethod() ? 1 : 0);
for (int i = 0; i < nonVarargCount; i++) {
checkTypeCoercion(lhsTypes.get(i), rhsValues.get(i));
}
var argumentCount = rhsValues.size();
if (!methodSymbol.isVarArgsMethod() || argumentCount == nonVarargCount) {
return;
}

var arrayType = (Type.ArrayType) lhsTypes.get(nonVarargCount);
var elementType = arrayType.elementType();
checkTypeCoercion(arrayType, rhsValues.get(nonVarargCount));
for (int i = nonVarargCount; i < argumentCount; i++) {
checkTypeCoercion(elementType, rhsValues.get(i));
}
}

private static boolean isThreadAsRunnable(Type providedType, List<Type> parametersTypes, int index, boolean varargs) {
Type expectedType = getExpectedType(providedType, parametersTypes, index, varargs);
return (expectedType.is("java.lang.Runnable") && providedType.isSubtypeOf("java.lang.Thread"))
|| (expectedType.is("java.lang.Runnable[]") && providedType.isSubtypeOf("java.lang.Thread[]"));
private void visitNewArray(NewArrayTree tree) {
var lhsType = tree.type();
if (lhsType != null && lhsType.symbolType().is(RUNNABLE_TYPE)) {
tree.initializers().forEach(rhsValue -> checkTypeCoercion(lhsType.symbolType(), rhsValue));
}
}

private static Type getExpectedType(Type providedType, List<Type> parametersTypes, int index, boolean varargs) {
int lastParameterIndex = parametersTypes.size() - 1;
Type lastParameterType = parametersTypes.get(lastParameterIndex);
Type lastExpectedType = varargs ? ((Type.ArrayType) lastParameterType).elementType() : lastParameterType;
if (index > lastParameterIndex || (index == lastParameterIndex && varargs && !providedType.isArray())) {
return lastExpectedType;
private void visitReturnStatement(ReturnStatementTree tree) {
var expression = tree.expression();
if (expression == null) {
return;
}

Tree enclosing = ExpressionUtils.getEnclosingTree(tree, Tree.Kind.METHOD, Tree.Kind.LAMBDA_EXPRESSION);
if (enclosing != null) {
var lhsType = enclosing instanceof LambdaExpressionTree lambda ?
lambda.symbol().returnType().type() : ((MethodTree) enclosing).returnType().symbolType();
checkTypeCoercion(lhsType, expression);
}
return parametersTypes.get(index);
}

private static String getMessage(ExpressionTree argument, Type providedType, int index) {
String array = providedType.isArray() ? "[]" : "";
return MessageFormat.format("Replace \"{0}\" of type Thread{1} with an instance of Runnable{1}.", getArgName(argument, index), array);
private void visitYieldStatement(YieldStatementTree tree) {
Tree enclosing = ExpressionUtils.getEnclosingTree(tree, Tree.Kind.SWITCH_EXPRESSION, Tree.Kind.SWITCH_STATEMENT);
if (enclosing == null || enclosing.is(Tree.Kind.SWITCH_STATEMENT)) {
// Iside of a SwitchStatementTree, the `yield` is implicit and does not return a value.
// Unlike SwitchExpressionTree, SwitchStatementTree is not an ExpressionTree.
return;
}
var lhsType = ((ExpressionTree) enclosing).symbolType();
checkTypeCoercion(lhsType, tree.expression());
}

private static String getArgName(ExpressionTree tree, int index) {
if (tree.is(Tree.Kind.IDENTIFIER)) {
return ((IdentifierTree) tree).name();
private void checkTypeCoercion(Type lhsType, ExpressionTree rhsValue) {
var rhsType = rhsValue.symbolType();
if ((lhsType.is(RUNNABLE_TYPE) && isNonNullSubtypeOf(rhsType, THREAD_TYPE)) ||
(lhsType.is(RUNNABLE_ARRAY_TYPE) && isNonNullSubtypeOf(rhsType, THREAD_ARRAY_TYPE))) {
var message = MessageFormat.format("Replace this {0} instance with an instance of {1}.", rhsType.name(), lhsType.name());
context.reportIssue(this, rhsValue, message);
}
return "argument " + (index + 1);
}

private static boolean isNonNullSubtypeOf(Type type, String superTypeName) {
return !type.isNullType() && type.isSubtypeOf(superTypeName);
}
}
Loading

0 comments on commit 55359e8

Please sign in to comment.