Skip to content

Commit 287cdd4

Browse files
aurelien-coet-sonarsourceaurelien-coet-sonarsource
authored
SONARJAVA-6122 Change the implementation of S3078 to cover more operations (#5509)
1 parent b9ee2c1 commit 287cdd4

File tree

2 files changed

+143
-50
lines changed

2 files changed

+143
-50
lines changed

‎java-checks-test-sources/default/src/main/java/checks/VolatileVariablesOperationsCheck.java‎

Lines changed: 45 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,13 +16,21 @@ class VolatileVariablesOperationsCheck {
1616

1717
public void incrementCounts() {
1818
count1++; // Noncompliant {{Use an "AtomicInteger" for this field; its operations are atomic.}}
19+
// ^^^^^^
1920
++this.count1; // Noncompliant {{Use an "AtomicInteger" for this field; its operations are atomic.}}
21+
// ^^^^^^
2022
(count2)++; // Noncompliant {{Use an "AtomicLong" for this field; its operations are atomic.}}
23+
// ^^^^^^
2124
count2 = (++count2); // Noncompliant {{Use an "AtomicLong" for this field; its operations are atomic.}}
25+
// ^^^^^^
2226
count3++; // Noncompliant {{Use an "AtomicInteger" for this field; its operations are atomic.}}
27+
// ^^^^^^
2328
++count3; // Noncompliant
29+
// ^^^^^^
2430
count4++; // Noncompliant {{Use an "AtomicLong" for this field; its operations are atomic.}}
31+
// ^^^^^^
2532
++count4; // Noncompliant
33+
// ^^^^^^
2634
nonVolatileCount1++;
2735
++nonVolatileCount1;
2836
nonVolatileCount2++;
@@ -33,13 +41,21 @@ public void incrementCounts() {
3341

3442
public void decrementCounts() {
3543
count1--; // Noncompliant {{Use an "AtomicInteger" for this field; its operations are atomic.}}
44+
// ^^^^^^
3645
--count1; // Noncompliant
46+
// ^^^^^^
3747
(count2)--; // Noncompliant {{Use an "AtomicLong" for this field; its operations are atomic.}}
48+
// ^^^^^^
3849
count2 = (--count2); // Noncompliant
50+
// ^^^^^^
3951
count3--; // Noncompliant
52+
// ^^^^^^
4053
--count3; // Noncompliant
54+
// ^^^^^^
4155
count4--; // Noncompliant
56+
// ^^^^^^
4257
--count4; // Noncompliant
58+
// ^^^^^^
4359
nonVolatileCount1--;
4460
--nonVolatileCount1;
4561
nonVolatileCount2--;
@@ -50,9 +66,13 @@ public void decrementCounts() {
5066

5167
public boolean toggleBooleans(){
5268
boo1 = !boo1; // Noncompliant {{Use an "AtomicBoolean" for this field; its operations are atomic.}}
69+
// ^^^^
5370
boo1 = (!boo1); // Noncompliant
71+
// ^^^^
5472
boo1 = !(boo1); // Noncompliant
73+
// ^^^^
5574
this.boo1 = (!this.boo1); // Noncompliant
75+
// ^^^^
5676
boo2 = !boo2;
5777
boo2 = !boo1;
5878
this.boo2 = (!this.boo1);
@@ -62,17 +82,38 @@ public boolean toggleBooleans(){
6282
return !boo1;
6383
}
6484

65-
void binaryOperations() {
85+
void assignments() {
86+
// 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...
87+
count1 = 3 * count1 + 2; // Noncompliant {{Use an "AtomicInteger" for this field; its operations are atomic.}}
88+
// ^^^^^^
6689
count1 *= 1; // Noncompliant
90+
// ^^^^^^
6791
count1 /= 1; // Noncompliant
92+
// ^^^^^^
6893
count1 %= 1; // Noncompliant
94+
// ^^^^^^
6995
count1 += 1; // Noncompliant
96+
// ^^^^^^
7097
count1 -= 1; // Noncompliant
98+
// ^^^^^^
7199
count1 <<= 1; // Noncompliant
100+
// ^^^^^^
72101
count1 >>= 1; // Noncompliant
102+
// ^^^^^^
73103
count1 >>>= 1; // Noncompliant
104+
// ^^^^^^
74105
count1 ^= 1; // Noncompliant
106+
// ^^^^^^
75107
count1 |= 1; // Noncompliant
108+
// ^^^^^^
109+
boo1 = true && !(boo1 || boo2); // Noncompliant
110+
// ^^^^
111+
boo1 &= true; // Noncompliant
112+
// ^^^^
113+
boo1 |= true; // Noncompliant
114+
// ^^^^
115+
boo1 ^= true; // Noncompliant
116+
// ^^^^
76117
}
77118

78119
synchronized void synchronizedMethod() {
@@ -107,6 +148,7 @@ enum anEnum {
107148

108149
void method() {
109150
value++; // Noncompliant
151+
// ^^^^^
110152
}
111153
}
112154

@@ -116,6 +158,7 @@ record Vinyl(String singer, String title, int year) {
116158

117159
void method() {
118160
counter++; // Noncompliant
161+
// ^^^^^^^
119162
nonVolatile++;
120163
}
121164
}
@@ -128,6 +171,7 @@ class Container {
128171
interface MyInterface {
129172
default void method() {
130173
Container.counter++; // Noncompliant
174+
// ^^^^^^^
131175
Container.nonVolatile++;
132176
}
133177
}

‎java-checks/src/main/java/org/sonar/java/checks/VolatileVariablesOperationsCheck.java‎

Lines changed: 98 additions & 49 deletions
Original file line numberDiff line numberDiff line change
@@ -16,8 +16,10 @@
1616
*/
1717
package org.sonar.java.checks;
1818

19-
import java.util.Arrays;
19+
import java.util.HashSet;
2020
import java.util.List;
21+
import java.util.Optional;
22+
import java.util.Set;
2123
import javax.annotation.Nullable;
2224
import org.sonar.check.Rule;
2325
import org.sonar.java.model.ExpressionUtils;
@@ -26,6 +28,7 @@
2628
import org.sonar.plugins.java.api.semantic.Symbol;
2729
import org.sonar.plugins.java.api.semantic.Type;
2830
import org.sonar.plugins.java.api.tree.AssignmentExpressionTree;
31+
import org.sonar.plugins.java.api.tree.BaseTreeVisitor;
2932
import org.sonar.plugins.java.api.tree.ClassTree;
3033
import org.sonar.plugins.java.api.tree.ExpressionTree;
3134
import org.sonar.plugins.java.api.tree.IdentifierTree;
@@ -38,44 +41,76 @@
3841
@Rule(key = "S3078")
3942
public class VolatileVariablesOperationsCheck extends IssuableSubscriptionVisitor {
4043

44+
private final Set<Tree> visitedUnaryExpressions = new HashSet<>();
45+
4146
@Override
4247
public List<Tree.Kind> nodesToVisit() {
43-
return Arrays.asList(
44-
Tree.Kind.PREFIX_INCREMENT,
45-
Tree.Kind.POSTFIX_INCREMENT,
48+
return List.of(
4649
Tree.Kind.PREFIX_DECREMENT,
50+
Tree.Kind.PREFIX_INCREMENT,
4751
Tree.Kind.POSTFIX_DECREMENT,
48-
Tree.Kind.LOGICAL_COMPLEMENT,
52+
Tree.Kind.POSTFIX_INCREMENT,
53+
Tree.Kind.ASSIGNMENT,
54+
Tree.Kind.PLUS_ASSIGNMENT,
55+
Tree.Kind.MINUS_ASSIGNMENT,
4956
Tree.Kind.MULTIPLY_ASSIGNMENT,
5057
Tree.Kind.DIVIDE_ASSIGNMENT,
5158
Tree.Kind.REMAINDER_ASSIGNMENT,
52-
Tree.Kind.PLUS_ASSIGNMENT,
53-
Tree.Kind.MINUS_ASSIGNMENT,
5459
Tree.Kind.LEFT_SHIFT_ASSIGNMENT,
5560
Tree.Kind.RIGHT_SHIFT_ASSIGNMENT,
5661
Tree.Kind.UNSIGNED_RIGHT_SHIFT_ASSIGNMENT,
62+
Tree.Kind.AND_ASSIGNMENT,
5763
Tree.Kind.XOR_ASSIGNMENT,
58-
Tree.Kind.OR_ASSIGNMENT);
64+
Tree.Kind.OR_ASSIGNMENT
65+
);
5966
}
6067

6168
@Override
6269
public void visitNode(Tree tree) {
63-
ExpressionTree expression;
64-
if (tree instanceof UnaryExpressionTree unaryExpressionTree) {
65-
expression = ExpressionUtils.skipParentheses(unaryExpressionTree.expression());
70+
if (tree instanceof UnaryExpressionTree unaryExpression) {
71+
checkUnaryExpression(unaryExpression);
6672
} else {
67-
expression = ((AssignmentExpressionTree) tree).variable();
73+
checkAssignment((AssignmentExpressionTree) tree);
74+
}
75+
}
76+
77+
@Override
78+
public void leaveNode(Tree tree) {
79+
visitedUnaryExpressions.clear();
80+
}
81+
82+
private void checkUnaryExpression(UnaryExpressionTree unaryExpression) {
83+
if (visitedUnaryExpressions.contains(unaryExpression)) {
84+
return;
6885
}
69-
IdentifierTree identifier = getVariableIdentifier(expression);
86+
IdentifierTree identifier = getVariableIdentifier(ExpressionUtils.skipParentheses(unaryExpression.expression()));
7087
if (identifier == null || !identifier.symbol().isVolatile()) {
7188
return;
7289
}
90+
reportIssueIfNotInExcludedContext(identifier);
91+
}
7392

74-
if (tree.is(Tree.Kind.LOGICAL_COMPLEMENT)) {
75-
checkBooleanToggling(tree, identifier.symbol());
76-
} else {
77-
checkIncrementDecrement(tree, identifier);
93+
private void checkAssignment(AssignmentExpressionTree assignment) {
94+
IdentifierTree assignee = getVariableIdentifier(assignment.variable());
95+
if (assignee == null) {
96+
return;
97+
}
98+
99+
Symbol assigneeSymbol = assignee.symbol();
100+
if (!assigneeSymbol.isVolatile()) {
101+
return;
102+
}
103+
104+
if (assignment.is(Tree.Kind.ASSIGNMENT)) {
105+
SymbolCollector symbolCollector = new SymbolCollector();
106+
assignment.expression().accept(symbolCollector);
107+
Set<Symbol> symbols = symbolCollector.symbols;
108+
if (!symbols.contains(assigneeSymbol)) {
109+
return;
110+
}
78111
}
112+
113+
reportIssueIfNotInExcludedContext(assignee);
79114
}
80115

81116
@Nullable
@@ -89,57 +124,71 @@ private static IdentifierTree getVariableIdentifier(ExpressionTree expressionTre
89124
return null;
90125
}
91126

92-
private void checkBooleanToggling(Tree tree, Symbol identifierSymbol) {
93-
Tree parent = tree.parent();
94-
if (parent.is(Tree.Kind.PARENTHESIZED_EXPRESSION)) {
95-
checkBooleanToggling(parent, identifierSymbol);
96-
} else if (parent.is(Tree.Kind.ASSIGNMENT)) {
97-
IdentifierTree variableIdentifier = getVariableIdentifier(((AssignmentExpressionTree) parent).variable());
98-
if (variableIdentifier != null && identifierSymbol.equals(variableIdentifier.symbol())) {
99-
reportIssueIfNotInExcludedContext(tree, "AtomicBoolean");
100-
}
101-
}
102-
}
103-
104-
private void checkIncrementDecrement(Tree tree, IdentifierTree identifier) {
105-
Type symbolType = identifier.symbol().type();
106-
if (symbolType.is("int") || symbolType.is("java.lang.Integer")) {
107-
reportIssueIfNotInExcludedContext(tree, "AtomicInteger");
108-
} else if (symbolType.is("long") || symbolType.is("java.lang.Long")) {
109-
reportIssueIfNotInExcludedContext(tree, "AtomicLong");
127+
private void reportIssueIfNotInExcludedContext(IdentifierTree identifier) {
128+
Optional<String> recommendedType = recommendedType(identifier);
129+
if (recommendedType.isEmpty()) {
130+
return;
110131
}
111-
}
112132

113-
private void reportIssueIfNotInExcludedContext(Tree tree, String recommendedType) {
114-
Tree current = tree.parent();
133+
Tree current = identifier.parent();
115134
boolean foundClass = false;
116135
while (!foundClass) {
117136
switch (current.kind()) {
118-
case LAMBDA_EXPRESSION,
119-
SYNCHRONIZED_STATEMENT,
120-
COMPILATION_UNIT:
137+
case LAMBDA_EXPRESSION, SYNCHRONIZED_STATEMENT, COMPILATION_UNIT:
121138
return;
122139
case METHOD:
123140
if (ModifiersUtils.hasModifier(((MethodTree) current).modifiers(), Modifier.SYNCHRONIZED)) {
124141
return;
125142
}
126143
break;
127-
case ENUM,
128-
CLASS,
129-
INTERFACE,
130-
RECORD,
131-
IMPLICIT_CLASS:
144+
case ENUM, CLASS, INTERFACE, RECORD, IMPLICIT_CLASS:
132145
if (!current.is(Tree.Kind.IMPLICIT_CLASS) && ((ClassTree) current).simpleName() == null) {
133146
return;
134147
}
135-
// we got to a non-anonymous class or implicit class in compact source file, we can safely raise an issue
148+
// we got to a non-anonymous class or implicit class in a compact source file, we can safely raise an issue
136149
foundClass = true;
137150
break;
138-
139151
}
140152
current = current.parent();
141153
}
142-
reportIssue(tree, String.format("Use an \"%s\" for this field; its operations are atomic.", recommendedType));
154+
155+
reportIssue(identifier, String.format("Use an \"%s\" for this field; its operations are atomic.", recommendedType.get()));
156+
}
157+
158+
private static Optional<String> recommendedType(IdentifierTree identifier) {
159+
Type type = identifier.symbol().type();
160+
if (type.is("boolean") || type.is("java.lang.Boolean")) {
161+
return Optional.of("AtomicBoolean");
162+
} else if (type.is("int") || type.is("java.lang.Integer")) {
163+
return Optional.of("AtomicInteger");
164+
} else if (type.is("long") || type.is("java.lang.Long")) {
165+
return Optional.of("AtomicLong");
166+
} else {
167+
return Optional.empty();
168+
}
169+
}
170+
171+
private class SymbolCollector extends BaseTreeVisitor {
172+
173+
final Set<Symbol> symbols = new HashSet<>();
174+
175+
@Override
176+
public void visitUnaryExpression(UnaryExpressionTree tree) {
177+
visitedUnaryExpressions.add(tree);
178+
super.visitUnaryExpression(tree);
179+
}
180+
181+
@Override
182+
public void visitIdentifier(IdentifierTree tree) {
183+
symbols.add(tree.symbol());
184+
}
185+
186+
@Override
187+
public void visitMemberSelectExpression(MemberSelectExpressionTree tree) {
188+
symbols.add(tree.identifier().symbol());
189+
super.visitMemberSelectExpression(tree);
190+
}
191+
143192
}
144193

145194
}

0 commit comments

Comments
 (0)