Skip to content

Commit ef66447

Browse files
ValentinAebi-sonarValentinAebi-sonar
authored
SONARJAVA-4826: S6880 fix FP and semicolon in quickfix (#4730)
1 parent 9cc9a8a commit ef66447

2 files changed

Lines changed: 63 additions & 21 deletions

File tree

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

Lines changed: 31 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -88,7 +88,7 @@ int goodCompute4(Expr expr){
8888
}
8989

9090
// fix@qf1 {{Replace the chain of if/else with a switch expression.}}
91-
// edit@qf1 [[sl=+0;el=+12;sc=5;ec=6]] {{return switch (expr) {\n case Plus plus when plus.lhs.equals(ZERO) && plus.rhs.equals(ZERO) -> 0;\n case Plus plus -> badCompute(plus.lhs) + badCompute(plus.rhs);\n case Minus(var l, Expr r) when r.equals(ZERO) -> badCompute(l);\n case Minus(var l, Expr r) -> badCompute(l) - badCompute(r);\n case Const(var i) -> i;\n default -> throw new AssertionError();\n }}}
91+
// edit@qf1 [[sl=+0;el=+12;sc=5;ec=6]] {{return switch (expr) {\n case Plus plus when plus.lhs.equals(ZERO) && plus.rhs.equals(ZERO) -> 0;\n case Plus plus -> badCompute(plus.lhs) + badCompute(plus.rhs);\n case Minus(var l, Expr r) when r.equals(ZERO) -> badCompute(l);\n case Minus(var l, Expr r) -> badCompute(l) - badCompute(r);\n case Const(var i) -> i;\n default -> throw new AssertionError();\n };}}
9292
int badCompute(Expr expr) {
9393
// Noncompliant@+1 [[sl=+1;el=+1;sc=5;ec=7;quickfixes=qf1]]
9494
if (expr instanceof Plus plus && plus.lhs.equals(ZERO) && plus.rhs.equals(ZERO)) {
@@ -128,7 +128,7 @@ private static Expr mkExpr() {
128128
}
129129

130130
// fix@qf3 {{Replace the chain of if/else with a switch expression.}}
131-
// edit@qf3 [[sl=+0;el=+6;sc=7;ec=8]] {{return switch (x) {\n case 0, 1 -> "binary";\n case -1 -> "negative";\n default -> "I don't know!";\n }}}
131+
// edit@qf3 [[sl=+0;el=+6;sc=7;ec=8]] {{return switch (x) {\n case 0, 1 -> "binary";\n case -1 -> "negative";\n default -> "I don't know!";\n };}}
132132
String badFoo1(int x, boolean b) {
133133
if (b){
134134
// Noncompliant@+1 [[sl=+1;el=+1;sc=7;ec=9;quickfixes=qf3]]
@@ -144,7 +144,7 @@ String badFoo1(int x, boolean b) {
144144
}
145145

146146
// fix@qf2 {{Replace the chain of if/else with a switch expression.}}
147-
// edit@qf2 [[sl=+0;el=+6;sc=5;ec=6]] {{return switch (x) {\n case 0, 1 -> "Hello world";\n case -1 -> "negative";\n default -> "I don't know!";\n }}}
147+
// edit@qf2 [[sl=+0;el=+6;sc=5;ec=6]] {{return switch (x) {\n case 0, 1 -> "Hello world";\n case -1 -> "negative";\n default -> "I don't know!";\n };}}
148148
String badFoo2(int x) {
149149
// Noncompliant@+1 [[sl=+1;el=+1;sc=5;ec=7;quickfixes=qf2]]
150150
if (x == 0 || x == 1) {
@@ -157,7 +157,7 @@ String badFoo2(int x) {
157157
}
158158

159159
// fix@qf5 {{Replace the chain of if/else with a switch expression.}}
160-
// edit@qf5 [[sl=+0;el=+5;sc=7;ec=22]] {{return switch (x) {\n case -1 -> "negative";\n case 0 -> "zero";\n default -> "one";\n }}}
160+
// edit@qf5 [[sl=+0;el=+5;sc=7;ec=22]] {{return switch (x) {\n case -1 -> "negative";\n case 0 -> "zero";\n default -> "one";\n };}}
161161
String badFoo3(int x) {
162162
if (x == 0 || x == 1 || x == -1)
163163
if (x == -1) // Noncompliant [[sl=+0;el=+0;sc=7;ec=9;quickfixes=qf5]]
@@ -171,7 +171,7 @@ else if (x == 0)
171171
}
172172

173173
// fix@qf6 {{Replace the chain of if/else with a switch expression.}}
174-
// edit@qf6 [[sl=+0;el=+5;sc=7;ec=22]] {{return switch (x) {\n case -1 -> "negative";\n case 2, 0 -> "even";\n default -> "one";\n }}}
174+
// edit@qf6 [[sl=+0;el=+5;sc=7;ec=22]] {{return switch (x) {\n case -1 -> "negative";\n case 2, 0 -> "even";\n default -> "one";\n };}}
175175
String badFoo4(int x) {
176176
if (0 == x || x == 1 || -1 == x || x == 2)
177177
if (-1 == x) // Noncompliant [[sl=+0;el=+0;sc=7;ec=9;quickfixes=qf6]]
@@ -184,6 +184,19 @@ else if (x == 2 || 0 == x)
184184
return "Hello world";
185185
}
186186

187+
// fix@qf12 {{Replace the chain of if/else with a switch expression.}}
188+
// edit@qf12 [[sl=+0;el=+6;sc=5;ec=6]] {{return switch (x) {\n case 0, 1 -> "Hello world";\n case -1 -> "negative";\n default -> "I don't know!";\n };}}
189+
String badFoo5(Integer x) {
190+
// Noncompliant@+1 [[sl=+1;el=+1;sc=5;ec=7;quickfixes=qf12]]
191+
if (x == 0 || x == 1) {
192+
return "Hello world";
193+
} else if (x == -1) {
194+
return "negative";
195+
} else {
196+
return "I don't know!";
197+
}
198+
}
199+
187200
String goodFoo1(int x) {
188201
switch (x) {
189202
case 0, 1 -> {
@@ -297,7 +310,7 @@ enum Bar {
297310
}
298311

299312
// fix@qf4 {{Replace the chain of if/else with a switch expression.}}
300-
// edit@qf4 [[sl=+0;el=+6;sc=5;ec=6]] {{return switch (b) {\n case Bar.B1 -> "b1";\n case Bar.B2, B3, Bar.B4 -> "b234";\n default -> "b5";\n }}}
313+
// edit@qf4 [[sl=+0;el=+6;sc=5;ec=6]] {{return switch (b) {\n case Bar.B1 -> "b1";\n case Bar.B2, B3, Bar.B4 -> "b234";\n default -> "b5";\n };}}
301314
static String badBar1(Bar b) {
302315
// Noncompliant@+1 [[sl=+1;el=+1;sc=5;ec=7;quickfixes=qf4]]
303316
if (b == Bar.B1) {
@@ -342,7 +355,7 @@ static int badBar3(Bar b){
342355

343356
// fix@qf10 {{Replace the chain of if/else with a switch expression.}}
344357
// edit@qf10 [[sl=+0;el=+6;sc=5;ec=6]] {{switch (x) {\n case 0 -> {\n return;\n }\n case 1 -> {\n return;\n }\n default -> {\n return;\n }\n }}}
345-
static void doNotLiftVoidReturns(double x){
358+
static void doNotLiftVoidReturns(int x){
346359
// Noncompliant@+1 [[sl=+1;el=+1;sc=5;ec=7;quickfixes=qf10]]
347360
if (x == 0){
348361
return;
@@ -353,6 +366,17 @@ static void doNotLiftVoidReturns(double x){
353366
}
354367
}
355368

369+
static void doNotProposeSwitchOnDouble(double x){
370+
// Compliant: switch scrutinee cannot be a double
371+
if (x == 0){
372+
return;
373+
} else if (x == 1){
374+
return;
375+
} else {
376+
return;
377+
}
378+
}
379+
356380
void coverage(int x) {
357381
if (x == 1 && x*3<10) {
358382
System.out.println("one");

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

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@
2222
import java.util.Deque;
2323
import java.util.LinkedList;
2424
import java.util.List;
25+
import java.util.Set;
2526
import javax.annotation.Nullable;
2627
import org.sonar.check.Rule;
2728
import org.sonar.java.checks.helpers.QuickFixHelper;
@@ -49,6 +50,10 @@ public class PatternMatchUsingIfCheck extends IssuableSubscriptionVisitor implem
4950

5051
private static final String ISSUE_MESSAGE = "Replace the chain of if/else with a switch expression.";
5152
private static final int INDENT = 2;
53+
private static final Set<String> SCRUTINEE_TYPES_FOR_NON_PATTERN_SWITCH = Set.of(
54+
"byte", "short", "char", "int",
55+
"java.lang.Byte", "java.lang.Short", "java.lang.Character", "java.lang.Integer"
56+
);
5257

5358
@Override
5459
public boolean isCompatibleWithJavaVersion(JavaVersion version) {
@@ -74,7 +79,8 @@ public void visitNode(Tree tree) {
7479
}
7580

7681
var cases = extractCasesFromIfSequence(topLevelIfStat);
77-
if (cases == null || !(cases.get(cases.size() - 1) instanceof DefaultCase) || !casesHaveCommonScrutinee(cases)) {
82+
if (cases == null || !(cases.get(cases.size() - 1) instanceof DefaultCase) || !casesHaveCommonScrutinee(cases)
83+
|| (cases.get(0) instanceof EqualityCase && !hasValidScrutineeTypeForNonPatternSwitch(cases.get(0).scrutinee()))) {
7884
return;
7985
}
8086

@@ -86,7 +92,15 @@ public void visitNode(Tree tree) {
8692
}
8793

8894
private static boolean casesHaveCommonScrutinee(List<Case> cases) {
89-
return cases.stream().allMatch(c -> c.scrutinee().equals(cases.get(0).scrutinee()));
95+
return cases.stream().allMatch(c -> c.scrutinee().name().equals(cases.get(0).scrutinee().name()));
96+
}
97+
98+
private static boolean hasValidScrutineeTypeForNonPatternSwitch(IdentifierTree scrutinee) {
99+
if (scrutinee.symbolType().symbol().isEnum()) {
100+
return true;
101+
}
102+
var fullyQualifiedTypeName = scrutinee.symbolType().fullyQualifiedName();
103+
return SCRUTINEE_TYPES_FOR_NON_PATTERN_SWITCH.contains(fullyQualifiedTypeName);
90104
}
91105

92106
private static @Nullable List<Case> extractCasesFromIfSequence(IfStatementTree topLevelIfStat) {
@@ -111,7 +125,7 @@ private static boolean casesHaveCommonScrutinee(List<Case> cases) {
111125
populateGuardsList(condition, guards);
112126
if (leftmost instanceof PatternInstanceOfTree patInstOf && patInstOf.pattern() != null
113127
&& patInstOf.expression() instanceof IdentifierTree idTree) {
114-
return new PatternMatchCase(idTree.name(), patInstOf.pattern(), guards, body);
128+
return new PatternMatchCase(idTree, patInstOf.pattern(), guards, body);
115129
} else if ((leftmost.kind() == Kind.CONDITIONAL_OR || leftmost.kind() == Kind.EQUAL_TO) && guards.isEmpty()) {
116130
return buildEqualityCase(leftmost, body);
117131
} else {
@@ -124,35 +138,35 @@ private static boolean casesHaveCommonScrutinee(List<Case> cases) {
124138
*/
125139
private static @Nullable EqualityCase buildEqualityCase(ExpressionTree expr, StatementTree body) {
126140
var constantsList = new LinkedList<ExpressionTree>();
127-
String scrutinee = null;
141+
IdentifierTree scrutinee = null;
128142
while (expr.kind() == Kind.CONDITIONAL_OR) {
129143
var binary = (BinaryExpressionTree) expr;
130144
var varAndCst = extractVarAndConstFromEqualityCheck(binary.rightOperand());
131145
if (varAndCst == null) {
132146
return null;
133147
} else if (scrutinee == null) {
134148
scrutinee = varAndCst.a;
135-
} else if (!varAndCst.a.equals(scrutinee)) {
149+
} else if (!varAndCst.a.name().equals(scrutinee.name())) {
136150
return null;
137151
}
138152
constantsList.addFirst(varAndCst.b);
139153
expr = binary.leftOperand();
140154
}
141155
var varAndCst = extractVarAndConstFromEqualityCheck(expr);
142-
if (varAndCst == null || (scrutinee != null && !varAndCst.a.equals(scrutinee))) {
156+
if (varAndCst == null || (scrutinee != null && !varAndCst.a.name().equals(scrutinee.name()))) {
143157
return null;
144158
}
145159
constantsList.addFirst(varAndCst.b);
146160
return new EqualityCase(scrutinee == null ? varAndCst.a : scrutinee, constantsList, body);
147161
}
148162

149-
private static @Nullable Pair<String, ExpressionTree> extractVarAndConstFromEqualityCheck(ExpressionTree expr) {
163+
private static @Nullable Pair<IdentifierTree, ExpressionTree> extractVarAndConstFromEqualityCheck(ExpressionTree expr) {
150164
if (expr.kind() == Kind.EQUAL_TO) {
151165
var binary = (BinaryExpressionTree) expr;
152166
if (binary.leftOperand() instanceof IdentifierTree idTree && isPossibleConstantForCase(binary.rightOperand())) {
153-
return new Pair<>(idTree.name(), binary.rightOperand());
167+
return new Pair<>(idTree, binary.rightOperand());
154168
} else if (binary.rightOperand() instanceof IdentifierTree idTree && isPossibleConstantForCase(binary.leftOperand())) {
155-
return new Pair<>(idTree.name(), binary.leftOperand());
169+
return new Pair<>(idTree, binary.leftOperand());
156170
}
157171
}
158172
return null;
@@ -191,13 +205,16 @@ private JavaQuickFix computeQuickFix(List<Case> cases, IfStatementTree topLevelI
191205
if (canLiftReturn) {
192206
sb.append("return ");
193207
}
194-
sb.append("switch (").append(cases.get(0).scrutinee()).append(") {\n");
208+
sb.append("switch (").append(cases.get(0).scrutinee().name()).append(") {\n");
195209
for (Case caze : cases) {
196210
sb.append(" ".repeat(baseIndent + INDENT));
197211
writeCase(caze, sb, baseIndent, canLiftReturn);
198212
sb.append("\n");
199213
}
200214
sb.append(" ".repeat(baseIndent)).append("}");
215+
if (canLiftReturn) {
216+
sb.append(";");
217+
}
201218
var edit = JavaTextEdit.replaceTree(topLevelIfStat, sb.toString());
202219
return JavaQuickFix.newQuickFix(ISSUE_MESSAGE).addTextEdit(edit).build();
203220
}
@@ -267,21 +284,22 @@ private void join(List<? extends Tree> elems, String sep, StringBuilder sb) {
267284
}
268285

269286
private sealed interface Case permits PatternMatchCase, EqualityCase, DefaultCase {
270-
String scrutinee();
287+
IdentifierTree scrutinee();
271288

272289
StatementTree body();
273290
}
274291

275-
private record PatternMatchCase(String scrutinee, PatternTree pattern, List<ExpressionTree> guards, StatementTree body) implements Case {
292+
private record PatternMatchCase(IdentifierTree scrutinee, PatternTree pattern, List<ExpressionTree> guards,
293+
StatementTree body) implements Case {
276294
}
277295

278-
private record EqualityCase(String scrutinee, List<ExpressionTree> constants, StatementTree body) implements Case {
296+
private record EqualityCase(IdentifierTree scrutinee, List<ExpressionTree> constants, StatementTree body) implements Case {
279297
}
280298

281299
/**
282300
* For simplicity the default case should have the same scrutinee as the cases before it
283301
*/
284-
private record DefaultCase(String scrutinee, StatementTree body) implements Case {
302+
private record DefaultCase(IdentifierTree scrutinee, StatementTree body) implements Case {
285303
}
286304

287305
private record Pair<A, B>(A a, B b) {

0 commit comments

Comments
 (0)