diff --git a/java-checks-test-sources/default/src/main/java/checks/VolatileVariablesOperationsCheck.java b/java-checks-test-sources/default/src/main/java/checks/VolatileVariablesOperationsCheck.java index c337cae61d..8cf34da6ac 100644 --- a/java-checks-test-sources/default/src/main/java/checks/VolatileVariablesOperationsCheck.java +++ b/java-checks-test-sources/default/src/main/java/checks/VolatileVariablesOperationsCheck.java @@ -16,13 +16,21 @@ class VolatileVariablesOperationsCheck { public void incrementCounts() { count1++; // Noncompliant {{Use an "AtomicInteger" for this field; its operations are atomic.}} +// ^^^^^^ ++this.count1; // Noncompliant {{Use an "AtomicInteger" for this field; its operations are atomic.}} +// ^^^^^^ (count2)++; // Noncompliant {{Use an "AtomicLong" for this field; its operations are atomic.}} +// ^^^^^^ count2 = (++count2); // Noncompliant {{Use an "AtomicLong" for this field; its operations are atomic.}} +// ^^^^^^ count3++; // Noncompliant {{Use an "AtomicInteger" for this field; its operations are atomic.}} +// ^^^^^^ ++count3; // Noncompliant +// ^^^^^^ count4++; // Noncompliant {{Use an "AtomicLong" for this field; its operations are atomic.}} +// ^^^^^^ ++count4; // Noncompliant +// ^^^^^^ nonVolatileCount1++; ++nonVolatileCount1; nonVolatileCount2++; @@ -33,13 +41,21 @@ public void incrementCounts() { public void decrementCounts() { count1--; // Noncompliant {{Use an "AtomicInteger" for this field; its operations are atomic.}} +// ^^^^^^ --count1; // Noncompliant +// ^^^^^^ (count2)--; // Noncompliant {{Use an "AtomicLong" for this field; its operations are atomic.}} +// ^^^^^^ count2 = (--count2); // Noncompliant +// ^^^^^^ count3--; // Noncompliant +// ^^^^^^ --count3; // Noncompliant +// ^^^^^^ count4--; // Noncompliant +// ^^^^^^ --count4; // Noncompliant +// ^^^^^^ nonVolatileCount1--; --nonVolatileCount1; nonVolatileCount2--; @@ -50,9 +66,13 @@ public void decrementCounts() { public boolean toggleBooleans(){ boo1 = !boo1; // Noncompliant {{Use an "AtomicBoolean" for this field; its operations are atomic.}} +// ^^^^ boo1 = (!boo1); // Noncompliant +// ^^^^ boo1 = !(boo1); // Noncompliant +// ^^^^ this.boo1 = (!this.boo1); // Noncompliant +// ^^^^ boo2 = !boo2; boo2 = !boo1; this.boo2 = (!this.boo1); @@ -62,17 +82,38 @@ public boolean toggleBooleans(){ return !boo1; } - void binaryOperations() { + void assignments() { + // An example such as the one below definitely shouldn't be applied on a volatile field, but it's probably also poor practice to apply it on an atomic field... + count1 = 3 * count1 + 2; // Noncompliant {{Use an "AtomicInteger" for this field; its operations are atomic.}} +// ^^^^^^ count1 *= 1; // Noncompliant +// ^^^^^^ count1 /= 1; // Noncompliant +// ^^^^^^ count1 %= 1; // Noncompliant +// ^^^^^^ count1 += 1; // Noncompliant +// ^^^^^^ count1 -= 1; // Noncompliant +// ^^^^^^ count1 <<= 1; // Noncompliant +// ^^^^^^ count1 >>= 1; // Noncompliant +// ^^^^^^ count1 >>>= 1; // Noncompliant +// ^^^^^^ count1 ^= 1; // Noncompliant +// ^^^^^^ count1 |= 1; // Noncompliant +// ^^^^^^ + boo1 = true && !(boo1 || boo2); // Noncompliant +// ^^^^ + boo1 &= true; // Noncompliant +// ^^^^ + boo1 |= true; // Noncompliant +// ^^^^ + boo1 ^= true; // Noncompliant +// ^^^^ } synchronized void synchronizedMethod() { @@ -107,6 +148,7 @@ enum anEnum { void method() { value++; // Noncompliant +// ^^^^^ } } @@ -116,6 +158,7 @@ record Vinyl(String singer, String title, int year) { void method() { counter++; // Noncompliant +// ^^^^^^^ nonVolatile++; } } @@ -128,6 +171,7 @@ class Container { interface MyInterface { default void method() { Container.counter++; // Noncompliant +// ^^^^^^^ Container.nonVolatile++; } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/VolatileVariablesOperationsCheck.java b/java-checks/src/main/java/org/sonar/java/checks/VolatileVariablesOperationsCheck.java index 5b34f0c27b..e3f74bfd07 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/VolatileVariablesOperationsCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/VolatileVariablesOperationsCheck.java @@ -16,8 +16,10 @@ */ package org.sonar.java.checks; -import java.util.Arrays; +import java.util.HashSet; import java.util.List; +import java.util.Optional; +import java.util.Set; import javax.annotation.Nullable; import org.sonar.check.Rule; import org.sonar.java.model.ExpressionUtils; @@ -26,6 +28,7 @@ import org.sonar.plugins.java.api.semantic.Symbol; import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.AssignmentExpressionTree; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; import org.sonar.plugins.java.api.tree.ClassTree; import org.sonar.plugins.java.api.tree.ExpressionTree; import org.sonar.plugins.java.api.tree.IdentifierTree; @@ -38,44 +41,76 @@ @Rule(key = "S3078") public class VolatileVariablesOperationsCheck extends IssuableSubscriptionVisitor { + private final Set visitedUnaryExpressions = new HashSet<>(); + @Override public List nodesToVisit() { - return Arrays.asList( - Tree.Kind.PREFIX_INCREMENT, - Tree.Kind.POSTFIX_INCREMENT, + return List.of( Tree.Kind.PREFIX_DECREMENT, + Tree.Kind.PREFIX_INCREMENT, Tree.Kind.POSTFIX_DECREMENT, - Tree.Kind.LOGICAL_COMPLEMENT, + Tree.Kind.POSTFIX_INCREMENT, + Tree.Kind.ASSIGNMENT, + Tree.Kind.PLUS_ASSIGNMENT, + Tree.Kind.MINUS_ASSIGNMENT, Tree.Kind.MULTIPLY_ASSIGNMENT, Tree.Kind.DIVIDE_ASSIGNMENT, Tree.Kind.REMAINDER_ASSIGNMENT, - Tree.Kind.PLUS_ASSIGNMENT, - Tree.Kind.MINUS_ASSIGNMENT, Tree.Kind.LEFT_SHIFT_ASSIGNMENT, Tree.Kind.RIGHT_SHIFT_ASSIGNMENT, Tree.Kind.UNSIGNED_RIGHT_SHIFT_ASSIGNMENT, + Tree.Kind.AND_ASSIGNMENT, Tree.Kind.XOR_ASSIGNMENT, - Tree.Kind.OR_ASSIGNMENT); + Tree.Kind.OR_ASSIGNMENT + ); } @Override public void visitNode(Tree tree) { - ExpressionTree expression; - if (tree instanceof UnaryExpressionTree unaryExpressionTree) { - expression = ExpressionUtils.skipParentheses(unaryExpressionTree.expression()); + if (tree instanceof UnaryExpressionTree unaryExpression) { + checkUnaryExpression(unaryExpression); } else { - expression = ((AssignmentExpressionTree) tree).variable(); + checkAssignment((AssignmentExpressionTree) tree); + } + } + + @Override + public void leaveNode(Tree tree) { + visitedUnaryExpressions.clear(); + } + + private void checkUnaryExpression(UnaryExpressionTree unaryExpression) { + if (visitedUnaryExpressions.contains(unaryExpression)) { + return; } - IdentifierTree identifier = getVariableIdentifier(expression); + IdentifierTree identifier = getVariableIdentifier(ExpressionUtils.skipParentheses(unaryExpression.expression())); if (identifier == null || !identifier.symbol().isVolatile()) { return; } + reportIssueIfNotInExcludedContext(identifier); + } - if (tree.is(Tree.Kind.LOGICAL_COMPLEMENT)) { - checkBooleanToggling(tree, identifier.symbol()); - } else { - checkIncrementDecrement(tree, identifier); + private void checkAssignment(AssignmentExpressionTree assignment) { + IdentifierTree assignee = getVariableIdentifier(assignment.variable()); + if (assignee == null) { + return; + } + + Symbol assigneeSymbol = assignee.symbol(); + if (!assigneeSymbol.isVolatile()) { + return; + } + + if (assignment.is(Tree.Kind.ASSIGNMENT)) { + SymbolCollector symbolCollector = new SymbolCollector(); + assignment.expression().accept(symbolCollector); + Set symbols = symbolCollector.symbols; + if (!symbols.contains(assigneeSymbol)) { + return; + } } + + reportIssueIfNotInExcludedContext(assignee); } @Nullable @@ -89,57 +124,71 @@ private static IdentifierTree getVariableIdentifier(ExpressionTree expressionTre return null; } - private void checkBooleanToggling(Tree tree, Symbol identifierSymbol) { - Tree parent = tree.parent(); - if (parent.is(Tree.Kind.PARENTHESIZED_EXPRESSION)) { - checkBooleanToggling(parent, identifierSymbol); - } else if (parent.is(Tree.Kind.ASSIGNMENT)) { - IdentifierTree variableIdentifier = getVariableIdentifier(((AssignmentExpressionTree) parent).variable()); - if (variableIdentifier != null && identifierSymbol.equals(variableIdentifier.symbol())) { - reportIssueIfNotInExcludedContext(tree, "AtomicBoolean"); - } - } - } - - private void checkIncrementDecrement(Tree tree, IdentifierTree identifier) { - Type symbolType = identifier.symbol().type(); - if (symbolType.is("int") || symbolType.is("java.lang.Integer")) { - reportIssueIfNotInExcludedContext(tree, "AtomicInteger"); - } else if (symbolType.is("long") || symbolType.is("java.lang.Long")) { - reportIssueIfNotInExcludedContext(tree, "AtomicLong"); + private void reportIssueIfNotInExcludedContext(IdentifierTree identifier) { + Optional recommendedType = recommendedType(identifier); + if (recommendedType.isEmpty()) { + return; } - } - private void reportIssueIfNotInExcludedContext(Tree tree, String recommendedType) { - Tree current = tree.parent(); + Tree current = identifier.parent(); boolean foundClass = false; while (!foundClass) { switch (current.kind()) { - case LAMBDA_EXPRESSION, - SYNCHRONIZED_STATEMENT, - COMPILATION_UNIT: + case LAMBDA_EXPRESSION, SYNCHRONIZED_STATEMENT, COMPILATION_UNIT: return; case METHOD: if (ModifiersUtils.hasModifier(((MethodTree) current).modifiers(), Modifier.SYNCHRONIZED)) { return; } break; - case ENUM, - CLASS, - INTERFACE, - RECORD, - IMPLICIT_CLASS: + case ENUM, CLASS, INTERFACE, RECORD, IMPLICIT_CLASS: if (!current.is(Tree.Kind.IMPLICIT_CLASS) && ((ClassTree) current).simpleName() == null) { return; } - // we got to a non-anonymous class or implicit class in compact source file, we can safely raise an issue + // we got to a non-anonymous class or implicit class in a compact source file, we can safely raise an issue foundClass = true; break; - } current = current.parent(); } - reportIssue(tree, String.format("Use an \"%s\" for this field; its operations are atomic.", recommendedType)); + + reportIssue(identifier, String.format("Use an \"%s\" for this field; its operations are atomic.", recommendedType.get())); + } + + private static Optional recommendedType(IdentifierTree identifier) { + Type type = identifier.symbol().type(); + if (type.is("boolean") || type.is("java.lang.Boolean")) { + return Optional.of("AtomicBoolean"); + } else if (type.is("int") || type.is("java.lang.Integer")) { + return Optional.of("AtomicInteger"); + } else if (type.is("long") || type.is("java.lang.Long")) { + return Optional.of("AtomicLong"); + } else { + return Optional.empty(); + } + } + + private class SymbolCollector extends BaseTreeVisitor { + + final Set symbols = new HashSet<>(); + + @Override + public void visitUnaryExpression(UnaryExpressionTree tree) { + visitedUnaryExpressions.add(tree); + super.visitUnaryExpression(tree); + } + + @Override + public void visitIdentifier(IdentifierTree tree) { + symbols.add(tree.symbol()); + } + + @Override + public void visitMemberSelectExpression(MemberSelectExpressionTree tree) { + symbols.add(tree.identifier().symbol()); + super.visitMemberSelectExpression(tree); + } + } }