diff --git a/its/ruling/src/test/resources/expected/eclipse-jetty-similar-to-main/java-S1854.json b/its/ruling/src/test/resources/expected/eclipse-jetty-similar-to-main/java-S1854.json index fa56b3bf42b..c1eba36d3df 100644 --- a/its/ruling/src/test/resources/expected/eclipse-jetty-similar-to-main/java-S1854.json +++ b/its/ruling/src/test/resources/expected/eclipse-jetty-similar-to-main/java-S1854.json @@ -9,6 +9,9 @@ "eclipse-jetty:jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/WebSocketUriMappingTest.java": [ 91 ], +"eclipse-jetty:jetty-server/src/main/java/org/eclipse/jetty/server/handler/jmx/AbstractHandlerMBean.java": [ +82 +], "eclipse-jetty:jetty-server/src/test/java/org/eclipse/jetty/server/HttpOutputTest.java": [ 861 ], diff --git a/its/ruling/src/test/resources/expected/eclipse-jetty/java-S1854.json b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S1854.json index 1abca132f7a..a6725dec949 100644 --- a/its/ruling/src/test/resources/expected/eclipse-jetty/java-S1854.json +++ b/its/ruling/src/test/resources/expected/eclipse-jetty/java-S1854.json @@ -9,6 +9,9 @@ "eclipse-jetty:jetty-http/src/test/java/org/eclipse/jetty/http/pathmap/WebSocketUriMappingTest.java": [ 91 ], +"eclipse-jetty:jetty-server/src/main/java/org/eclipse/jetty/server/handler/jmx/AbstractHandlerMBean.java": [ +82 +], "eclipse-jetty:jetty-server/src/test/java/org/eclipse/jetty/server/HttpOutputTest.java": [ 861 ], @@ -80,6 +83,11 @@ 175, 188 ], +"eclipse-jetty:jetty-util-ajax/src/main/java/org/eclipse/jetty/util/ajax/AsyncJSON.java": [ +586, +629, +675 +], "eclipse-jetty:jetty-util/src/main/java/org/eclipse/jetty/util/IO.java": [ 149, 191 diff --git a/its/ruling/src/test/resources/expected/guava/java-S1854.json b/its/ruling/src/test/resources/expected/guava/java-S1854.json index a8ae5bbb88b..09848c96193 100644 --- a/its/ruling/src/test/resources/expected/guava/java-S1854.json +++ b/its/ruling/src/test/resources/expected/guava/java-S1854.json @@ -1,4 +1,7 @@ { +"guava:src/com/google/common/base/CaseFormat.java": [ +149 +], "guava:src/com/google/common/cache/LocalCache.java": [ 2331, 2844, diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/DeadStoreCheckSample.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/DeadStoreCheckSample.java index 7fc7005bd49..a9fc2fc2034 100644 --- a/java-checks-test-sources/default/src/main/files/non-compiling/checks/DeadStoreCheckSample.java +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/DeadStoreCheckSample.java @@ -9,6 +9,8 @@ void lambdas_not_resolved(UnknnownFunction lambda) { int x = 1; // Compliant return y + x; }); + lambdas_not_resolved(y -> ++y); + lambdas_not_resolved(y -> y += 2); } void fpLambdaWithAssignment(boolean cond) { diff --git a/java-checks-test-sources/default/src/main/java/checks/DeadStoreCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/DeadStoreCheckSample.java index 076724fa62f..14673587c45 100644 --- a/java-checks-test-sources/default/src/main/java/checks/DeadStoreCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/DeadStoreCheckSample.java @@ -1,15 +1,47 @@ package checks; import com.google.common.collect.Lists; - -import java.lang.Exception; -import java.io.*; +import java.io.BufferedReader; +import java.io.File; +import java.io.FileInputStream; +import java.io.FileReader; +import java.io.IOException; +import java.util.function.IntConsumer; import java.util.function.IntFunction; +import java.util.function.IntUnaryOperator; +import java.util.function.Supplier; class DeadStoreCheckSample { int var; + class ConstructorDeadStores { + ConstructorDeadStores(int value) { + int x = compute(); // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^^^^^^^^^ + x = 3; + value = compute(); // Noncompliant {{Remove this useless assignment to local variable "value".}} +// ^^^^^^^^^^^ + value = x; + var = value; + } + } + + class ConstructorWithLambdaCapture { + ConstructorWithLambdaCapture(boolean condition) { + int captured = compute(); + if (condition) { + System.out.println("branch"); + } + Runnable runnable = () -> { + int unused = compute(); // Noncompliant {{Remove this useless assignment to local variable "unused".}} +// ^^^^^^^^^^^ + System.out.println(captured); + }; + runnable.run(); + } + } + void singleAssignment(){ int z = 10; // Noncompliant } @@ -33,6 +65,14 @@ int foo(int u) { return 0; } + void dead_store_in_condition(BufferedReader reader) throws IOException { + String line; + if ((line = reader.readLine()) != null) { // Noncompliant {{Remove this useless assignment to local variable "line".}} +// ^^^^^^^^^^^^^^^^^^^ + System.out.println("Line available"); + } + } + void fields() { this.var = 2; // Compliant var = 3; // Compliant - do not check fields @@ -51,6 +91,213 @@ void lambdas_using_local_vars(IntFunction lambda) { lambdas_using_local_vars(y -> a + y); } + void dead_store_in_lambda() { + int captured = compute(); + Runnable r = () -> { + int live = compute(); + int x = compute(); // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^^^^^^^^^ + x = 3; + System.out.println(x + live + captured); + }; + r.run(); + } + + IntFunction dead_store_in_lambda_parameter() { + return x -> { + x = compute(); // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^^^^^^^^^ + x = 3; + return x; + }; + } + + IntFunction assignment_result_used_in_block_lambda() { + return x -> { + int y = x = x + 1; // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^^^^^ + return y; + }; + } + + IntFunction dead_store_in_expression_lambda() { + return x -> x++; // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^ + } + + IntConsumer prefix_increment_lambda = x -> ++x; // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^ + + IntConsumer compound_assignment_lambda = x -> x += 2; // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^^ + + IntConsumer assignment_lambda = x -> x = 2; // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^ + + IntConsumer prefix_increment_block_lambda = x -> { + System.out.println(++x); + ++x; // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^ + }; + + IntConsumer compound_assignment_block_lambda = x -> { + System.out.println(x += 2); + x += 2; // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^^ + }; + + IntUnaryOperator prefix_increment_result_lambda = x -> ++x; // Noncompliant {{Remove this useless assignment to local variable "x".}} + IntUnaryOperator compound_assignment_result_lambda = x -> x += 2; // Noncompliant {{Remove this useless assignment to local variable "x".}} + IntUnaryOperator assignment_result_lambda = x -> x = x + 1; // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^^^^^ + IntUnaryOperator assignment_result_block_lambda = x -> { + return x = x + 1; // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^^^^^ + }; + + Runnable prefix_increment_field_lambda = () -> ++var; + Runnable compound_assignment_field_lambda = () -> var += 2; + Runnable empty_lambda = () -> {}; + + Runnable dead_store_in_nested_lambda(boolean condition) { + return () -> { + int captured = compute(); + if (condition) { + System.out.println("branch"); + } + Runnable nested = () -> { + int x = compute(); // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^^^^^^^^^ + x = captured; + System.out.println(x); + }; + nested.run(); + }; + } + + Supplier expression_lambda_returning_lambda() { + return () -> () -> { + int x = compute(); // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^^^^^^^^^ + x = 3; + System.out.println(x); + }; + } + + Runnable classes_in_lambda_capturing_local_variables(boolean condition) { + return () -> { + int capturedByAnonymousClass = compute(); + int capturedByLocalClass = compute(); + class Local { + void print() { + System.out.println(capturedByLocalClass); + } + } + if (condition) { + System.out.println("branch"); + } + new Runnable() { + @Override + public void run() { + System.out.println(capturedByAnonymousClass); + new Local().print(); + } + }.run(); + }; + } + + Runnable method_reference_in_lambda_using_local_variable(boolean condition) { + return () -> { + Object captured = new Object(); + if (condition) { + System.out.println("branch"); + } + Supplier reference = captured::toString; + System.out.println(reference.get()); + }; + } + + IntFunction lambda_with_conditional_assignment() { + return condition -> { + int x = compute(); + if (condition > 0) { + x = compute(); // Noncompliant {{Remove this useless assignment to local variable "x".}} +// ^^^^^^^^^^^ + x = 3; + } + return x; + }; + } + + Runnable lambda_using_local_variable_in_finally() { + return () -> { + int x = compute(); + try { + x = compute(); + } finally { + System.out.println(x); + } + }; + } + + Runnable lambda_using_local_variable_in_catch() { + return () -> { + int x = compute(); + try { + x = compute(); + System.out.println(x); + } catch (RuntimeException e) { + System.out.println(x); + } + }; + } + + Runnable lambda_with_resource_used_only_by_implicit_close() { + return () -> { + try (FileInputStream stream = new FileInputStream("...")) { + } catch (java.io.IOException e) { + throw new java.io.UncheckedIOException(e); + } + }; + } + + Runnable dead_store_in_lambda_inside_try_finally() { + int x = compute(); + try { + return () -> { + int y = compute(); // Noncompliant {{Remove this useless assignment to local variable "y".}} +// ^^^^^^^^^^^ + y = 3; + System.out.println(y); + }; + } finally { + System.out.println(x); + } + } + + Runnable try_finally_in_nested_lambda_should_not_exclude_enclosing_scopes() { + int unused = compute(); // Noncompliant {{Remove this useless assignment to local variable "unused".}} +// ^^^^^^^^^^^ + int capturedByOuterLambda = compute(); + return () -> { + int unusedInLambda = compute(); // Noncompliant {{Remove this useless assignment to local variable "unusedInLambda".}} +// ^^^^^^^^^^^ + int capturedByInnerLambda = compute(); + Runnable nested = () -> { + try { + System.out.println("try"); + } finally { + System.out.println(capturedByOuterLambda + capturedByInnerLambda); + } + }; + nested.run(); + }; + } + + private int compute() { + return 1; + } + void ignore_try_finally() { int a; a = 12; //false negative excluded by try finally @@ -137,15 +384,15 @@ int increment_operator() { ++b; // Noncompliant // ^^^ int c = 0; - foo(++c); // compliant not last element of block + foo(++c); // Noncompliant {{Remove this useless assignment to local variable "c".}} int j = -1; - while ((j = foo(++j)) != -1) { + while ((j = foo(++j)) != -1) { // Noncompliant {{Remove this useless assignment to local variable "j".}} System.out.println(""); } if(i != 0) { return i++; // Noncompliant } else { - return ++i; + return ++i; // Noncompliant } } @@ -226,7 +473,7 @@ int foo() { n = 2; n += 12; // Noncompliant int order = 0; - return (short) (order &= 12); + return (short) (order &= 12); // Noncompliant } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/DeadStoreCheck.java b/java-checks/src/main/java/org/sonar/java/checks/DeadStoreCheck.java index 50c19590b33..60d1acdf7d3 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/DeadStoreCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/DeadStoreCheck.java @@ -18,7 +18,6 @@ import java.util.ArrayDeque; import java.util.ArrayList; -import java.util.Collections; import java.util.HashSet; import java.util.List; import java.util.Set; @@ -55,25 +54,31 @@ public class DeadStoreCheck extends IssuableSubscriptionVisitor { @Override public List nodesToVisit() { - return Collections.singletonList(Tree.Kind.METHOD); + return List.of(Tree.Kind.METHOD, Tree.Kind.CONSTRUCTOR, Tree.Kind.LAMBDA_EXPRESSION); } @Override public void visitNode(Tree tree) { - MethodTree methodTree = (MethodTree) tree; - if (methodTree.block() == null) { - return; + Tree body; + Symbol.MethodSymbol methodSymbol; + if (tree instanceof MethodTree methodTree) { + body = methodTree.block(); + methodSymbol = methodTree.symbol(); + } else { + LambdaExpressionTree lambda = (LambdaExpressionTree) tree; + body = lambda.body(); + methodSymbol = lambda.symbol(); } // TODO(npe) Exclude try statements with finally as CFG is incorrect for those and lead to false positive - if (hasTryFinallyWithLocalVar(methodTree.block(), methodTree.symbol())) { + if (body == null || hasTryFinallyWithLocalVar(body, methodSymbol)) { return; } - UNRESOLVED_IDENTIFIERS_VISITOR.check(methodTree); + UNRESOLVED_IDENTIFIERS_VISITOR.check(tree); - Symbol.MethodSymbol methodSymbol = methodTree.symbol(); - CFG cfg = (CFG) methodTree.cfg(); + CFG cfg = tree instanceof MethodTree methodTree ? (CFG) methodTree.cfg() : (CFG) ((LambdaExpressionTree) tree).cfg(); + cfg.setMethodSymbol(methodSymbol); LiveVariables liveVariables = LiveVariables.analyze(cfg); // Liveness analysis provides information only for block boundaries, so we should do analysis between elements within blocks for (CFG.Block block : cfg.blocks()) { @@ -153,7 +158,6 @@ private void handleAssignment(Set out, Set assignmentLHS, Assignme Symbol symbol = ((IdentifierTree) lhs).symbol(); if (symbol.isLocalVariable() && !out.contains(symbol) - && (element.is(Tree.Kind.ASSIGNMENT) || isParentExpressionStatement(element)) && !UNRESOLVED_IDENTIFIERS_VISITOR.isUnresolved(symbol.name())) { createIssue(element.operatorToken(), element.expression(), symbol); } @@ -166,10 +170,6 @@ private void handleAssignment(Set out, Set assignmentLHS, Assignme } } - private static boolean isParentExpressionStatement(Tree element) { - return element.parent().is(Tree.Kind.EXPRESSION_STATEMENT); - } - private static void handleIdentifier(Set out, Set assignmentLHS, IdentifierTree element) { Symbol symbol = element.symbol(); if (!assignmentLHS.contains(element) && symbol.isLocalVariable()) { @@ -228,7 +228,7 @@ private static void handleTryStatement(Set out, Symbol.MethodSymbol meth private void handlePrefixExpression(Set out, UnaryExpressionTree element) { // within each block, each produced value is consumed or by following elements or by terminator ExpressionTree expression = element.expression(); - if (isParentExpressionStatement(element) && expression.is(Tree.Kind.IDENTIFIER)) { + if (expression.is(Tree.Kind.IDENTIFIER)) { Symbol symbol = ((IdentifierTree) expression).symbol(); if (symbol.isLocalVariable() && !out.contains(symbol)) { createIssue(element, symbol); @@ -264,9 +264,9 @@ private static Set getUsedLocalVarInSubTree(Tree tree, Symbol.MethodSymb return localVarExtractor.usedVariables(); } - private static boolean hasTryFinallyWithLocalVar(BlockTree block, Symbol.MethodSymbol methodSymbol) { + private static boolean hasTryFinallyWithLocalVar(Tree body, Symbol.MethodSymbol methodSymbol) { TryVisitor tryVisitor = new TryVisitor(methodSymbol); - block.accept(tryVisitor); + body.accept(tryVisitor); return tryVisitor.hasTryFinally; } @@ -292,6 +292,11 @@ public void visitTryStatement(TryStatementTree tree) { public void visitClass(ClassTree tree) { // ignore inner classes } + + @Override + public void visitLambdaExpression(LambdaExpressionTree tree) { + // Lambda bodies have separate CFGs, so their try statements must not exclude the enclosing scope from analysis. + } } private static class AssignedLocalVarVisitor extends BaseTreeVisitor {