From 6d9a93d648d811083bf38a879af2def7ec464ad1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Coet?= Date: Wed, 4 Mar 2026 09:06:21 +0100 Subject: [PATCH 1/5] Change the implementation of S3078 to cover more operations --- .../VolatileVariablesOperationsCheck.java | 45 +++++- .../VolatileVariablesOperationsCheck.java | 131 +++++++++++------- 2 files changed, 126 insertions(+), 50 deletions(-) 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 c337cae61d8..3f33e271a84 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,37 @@ public boolean toggleBooleans(){ return !boo1; } - void binaryOperations() { + void assignments() { + count1 = 3 * count1 + 2; // 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 +// ^^^^^^ count1 |= 1; // Noncompliant +// ^^^^^^ + boo1 = true && !(boo1 || boo2); // Noncompliant +// ^^^^ + boo1 &= true; // Noncompliant +// ^^^^ + boo1 |= true; // Noncompliant +// ^^^^ + boo1 ^= true; // Noncompliant +// ^^^^ } synchronized void synchronizedMethod() { @@ -107,6 +147,7 @@ enum anEnum { void method() { value++; // Noncompliant +// ^^^^^ } } @@ -116,6 +157,7 @@ record Vinyl(String singer, String title, int year) { void method() { counter++; // Noncompliant +// ^^^^^^^ nonVolatile++; } } @@ -128,6 +170,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 5b34f0c27bd..80b2f835833 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,9 @@ */ package org.sonar.java.checks; -import java.util.Arrays; +import java.util.HashSet; import java.util.List; +import java.util.Set; import javax.annotation.Nullable; import org.sonar.check.Rule; import org.sonar.java.model.ExpressionUtils; @@ -26,6 +27,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 +40,67 @@ @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); + } + } + + 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()); + 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 +114,65 @@ 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) { + Type type = identifier.symbol().type(); + String recommendedType; + if (type.is("boolean") || type.is("java.lang.Boolean")) { + recommendedType = "AtomicBoolean"; + } else if (type.is("int") || type.is("java.lang.Integer")) { + recommendedType = "AtomicInteger"; + } else if (type.is("long") || type.is("java.lang.Long")) { + recommendedType = "AtomicLong"; + } else { + 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)); + } + + 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); + } + } } From 6ece6c7cb6efea08fc1947fcd4429ffcae017c9c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Coet?= Date: Wed, 4 Mar 2026 10:19:25 +0100 Subject: [PATCH 2/5] Add missing null check and fix cognitive complexity --- .../VolatileVariablesOperationsCheck.java | 31 +++++++++++++------ 1 file changed, 21 insertions(+), 10 deletions(-) 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 80b2f835833..6fc33f3f985 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 @@ -18,6 +18,7 @@ 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; @@ -86,6 +87,10 @@ private void checkUnaryExpression(UnaryExpressionTree unaryExpression) { private void checkAssignment(AssignmentExpressionTree assignment) { IdentifierTree assignee = getVariableIdentifier(assignment.variable()); + if (assignee == null) { + return; + } + Symbol assigneeSymbol = assignee.symbol(); if (!assigneeSymbol.isVolatile()) { return; @@ -115,15 +120,8 @@ private static IdentifierTree getVariableIdentifier(ExpressionTree expressionTre } private void reportIssueIfNotInExcludedContext(IdentifierTree identifier) { - Type type = identifier.symbol().type(); - String recommendedType; - if (type.is("boolean") || type.is("java.lang.Boolean")) { - recommendedType = "AtomicBoolean"; - } else if (type.is("int") || type.is("java.lang.Integer")) { - recommendedType = "AtomicInteger"; - } else if (type.is("long") || type.is("java.lang.Long")) { - recommendedType = "AtomicLong"; - } else { + Optional recommendedType = recommendedType(identifier); + if (recommendedType.isEmpty()) { return; } @@ -149,7 +147,20 @@ private void reportIssueIfNotInExcludedContext(IdentifierTree identifier) { current = current.parent(); } - reportIssue(identifier, 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 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 { From be63b0ed60039983fe9435284c911566ac86fe25 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Coet?= Date: Wed, 4 Mar 2026 10:36:04 +0100 Subject: [PATCH 3/5] Fix SQ issue with non-static method --- .../org/sonar/java/checks/VolatileVariablesOperationsCheck.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 6fc33f3f985..bdae30bb52a 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 @@ -150,7 +150,7 @@ private void reportIssueIfNotInExcludedContext(IdentifierTree identifier) { reportIssue(identifier, String.format("Use an \"%s\" for this field; its operations are atomic.", recommendedType.get())); } - private Optional recommendedType(IdentifierTree identifier) { + private static Optional recommendedType(IdentifierTree identifier) { Type type = identifier.symbol().type(); if (type.is("boolean") || type.is("java.lang.Boolean")) { return Optional.of("AtomicBoolean"); From 591760b18ecc81e0bda185353dee909eeb2adf16 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Coet?= Date: Thu, 5 Mar 2026 11:26:27 +0100 Subject: [PATCH 4/5] Apply fixes following review --- .../src/main/java/checks/VolatileVariablesOperationsCheck.java | 3 ++- .../sonar/java/checks/VolatileVariablesOperationsCheck.java | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) 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 3f33e271a84..8cf34da6ac1 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 @@ -83,7 +83,8 @@ public boolean toggleBooleans(){ } void assignments() { - count1 = 3 * count1 + 2; // Noncompliant + // 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 // ^^^^^^ 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 bdae30bb52a..e1601fef3dc 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 @@ -182,6 +182,7 @@ public void visitIdentifier(IdentifierTree tree) { public void visitMemberSelectExpression(MemberSelectExpressionTree tree) { symbols.add(tree.identifier().symbol()); super.visitMemberSelectExpression(tree); + visitedUnaryExpressions.clear(); } } From 9e8a49e89ed2404b2f970854e5120e2aa52b6ea1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Aur=C3=A9lien=20Coet?= Date: Thu, 5 Mar 2026 11:55:46 +0100 Subject: [PATCH 5/5] Clear the set of visited unary expressions in the leaveNode method --- .../sonar/java/checks/VolatileVariablesOperationsCheck.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 e1601fef3dc..e3f74bfd074 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 @@ -74,6 +74,11 @@ public void visitNode(Tree tree) { } } + @Override + public void leaveNode(Tree tree) { + visitedUnaryExpressions.clear(); + } + private void checkUnaryExpression(UnaryExpressionTree unaryExpression) { if (visitedUnaryExpressions.contains(unaryExpression)) { return; @@ -182,7 +187,6 @@ public void visitIdentifier(IdentifierTree tree) { public void visitMemberSelectExpression(MemberSelectExpressionTree tree) { symbols.add(tree.identifier().symbol()); super.visitMemberSelectExpression(tree); - visitedUnaryExpressions.clear(); } }