Skip to content

Commit

Permalink
SONARJAVA-4826: S6880 fix FP and semicolon in quickfix (#4730)
Browse files Browse the repository at this point in the history
  • Loading branch information
@ValentinAebi-sonar
ValentinAebi-sonar authored Mar 21, 2024
1 parent 9cc9a8a commit ef66447
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 21 deletions.
38 changes: 31 additions & 7 deletions java-checks-test-sources/default/src/main/java/checks/PatternMatchUsingIfCheckSample.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ int goodCompute4(Expr expr){
}

// fix@qf1 {{Replace the chain of if/else with a switch expression.}}
// 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 }}}
// 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 };}}
int badCompute(Expr expr) {
// Noncompliant@+1 [[sl=+1;el=+1;sc=5;ec=7;quickfixes=qf1]]
if (expr instanceof Plus plus && plus.lhs.equals(ZERO) && plus.rhs.equals(ZERO)) {
Expand Down Expand Up @@ -128,7 +128,7 @@ private static Expr mkExpr() {
}

// fix@qf3 {{Replace the chain of if/else with a switch expression.}}
// 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 }}}
// 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 };}}
String badFoo1(int x, boolean b) {
if (b){
// Noncompliant@+1 [[sl=+1;el=+1;sc=7;ec=9;quickfixes=qf3]]
Expand All @@ -144,7 +144,7 @@ String badFoo1(int x, boolean b) {
}

// fix@qf2 {{Replace the chain of if/else with a switch expression.}}
// 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 }}}
// 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 };}}
String badFoo2(int x) {
// Noncompliant@+1 [[sl=+1;el=+1;sc=5;ec=7;quickfixes=qf2]]
if (x == 0 || x == 1) {
Expand All @@ -157,7 +157,7 @@ String badFoo2(int x) {
}

// fix@qf5 {{Replace the chain of if/else with a switch expression.}}
// 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 }}}
// 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 };}}
String badFoo3(int x) {
if (x == 0 || x == 1 || x == -1)
if (x == -1) // Noncompliant [[sl=+0;el=+0;sc=7;ec=9;quickfixes=qf5]]
Expand All @@ -171,7 +171,7 @@ else if (x == 0)
}

// fix@qf6 {{Replace the chain of if/else with a switch expression.}}
// 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 }}}
// 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 };}}
String badFoo4(int x) {
if (0 == x || x == 1 || -1 == x || x == 2)
if (-1 == x) // Noncompliant [[sl=+0;el=+0;sc=7;ec=9;quickfixes=qf6]]
Expand All @@ -184,6 +184,19 @@ else if (x == 2 || 0 == x)
return "Hello world";
}

// fix@qf12 {{Replace the chain of if/else with a switch expression.}}
// 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 };}}
String badFoo5(Integer x) {
// Noncompliant@+1 [[sl=+1;el=+1;sc=5;ec=7;quickfixes=qf12]]
if (x == 0 || x == 1) {
return "Hello world";
} else if (x == -1) {
return "negative";
} else {
return "I don't know!";
}
}

String goodFoo1(int x) {
switch (x) {
case 0, 1 -> {
Expand Down Expand Up @@ -297,7 +310,7 @@ enum Bar {
}

// fix@qf4 {{Replace the chain of if/else with a switch expression.}}
// 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 }}}
// 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 };}}
static String badBar1(Bar b) {
// Noncompliant@+1 [[sl=+1;el=+1;sc=5;ec=7;quickfixes=qf4]]
if (b == Bar.B1) {
Expand Down Expand Up @@ -342,7 +355,7 @@ static int badBar3(Bar b){

// fix@qf10 {{Replace the chain of if/else with a switch expression.}}
// 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 }}}
static void doNotLiftVoidReturns(double x){
static void doNotLiftVoidReturns(int x){
// Noncompliant@+1 [[sl=+1;el=+1;sc=5;ec=7;quickfixes=qf10]]
if (x == 0){
return;
Expand All @@ -353,6 +366,17 @@ static void doNotLiftVoidReturns(double x){
}
}

static void doNotProposeSwitchOnDouble(double x){
// Compliant: switch scrutinee cannot be a double
if (x == 0){
return;
} else if (x == 1){
return;
} else {
return;
}
}

void coverage(int x) {
if (x == 1 && x*3<10) {
System.out.println("one");
Expand Down
46 changes: 32 additions & 14 deletions java-checks/src/main/java/org/sonar/java/checks/PatternMatchUsingIfCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import java.util.Deque;
import java.util.LinkedList;
import java.util.List;
import java.util.Set;
import javax.annotation.Nullable;
import org.sonar.check.Rule;
import org.sonar.java.checks.helpers.QuickFixHelper;
Expand Down Expand Up @@ -49,6 +50,10 @@ public class PatternMatchUsingIfCheck extends IssuableSubscriptionVisitor implem

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

@Override
public boolean isCompatibleWithJavaVersion(JavaVersion version) {
Expand All @@ -74,7 +79,8 @@ public void visitNode(Tree tree) {
}

var cases = extractCasesFromIfSequence(topLevelIfStat);
if (cases == null || !(cases.get(cases.size() - 1) instanceof DefaultCase) || !casesHaveCommonScrutinee(cases)) {
if (cases == null || !(cases.get(cases.size() - 1) instanceof DefaultCase) || !casesHaveCommonScrutinee(cases)
|| (cases.get(0) instanceof EqualityCase && !hasValidScrutineeTypeForNonPatternSwitch(cases.get(0).scrutinee()))) {
return;
}

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

private static boolean casesHaveCommonScrutinee(List<Case> cases) {
return cases.stream().allMatch(c -> c.scrutinee().equals(cases.get(0).scrutinee()));
return cases.stream().allMatch(c -> c.scrutinee().name().equals(cases.get(0).scrutinee().name()));
}

private static boolean hasValidScrutineeTypeForNonPatternSwitch(IdentifierTree scrutinee) {
if (scrutinee.symbolType().symbol().isEnum()) {
return true;
}
var fullyQualifiedTypeName = scrutinee.symbolType().fullyQualifiedName();
return SCRUTINEE_TYPES_FOR_NON_PATTERN_SWITCH.contains(fullyQualifiedTypeName);
}

private static @Nullable List<Case> extractCasesFromIfSequence(IfStatementTree topLevelIfStat) {
Expand All @@ -111,7 +125,7 @@ private static boolean casesHaveCommonScrutinee(List<Case> cases) {
populateGuardsList(condition, guards);
if (leftmost instanceof PatternInstanceOfTree patInstOf && patInstOf.pattern() != null
&& patInstOf.expression() instanceof IdentifierTree idTree) {
return new PatternMatchCase(idTree.name(), patInstOf.pattern(), guards, body);
return new PatternMatchCase(idTree, patInstOf.pattern(), guards, body);
} else if ((leftmost.kind() == Kind.CONDITIONAL_OR || leftmost.kind() == Kind.EQUAL_TO) && guards.isEmpty()) {
return buildEqualityCase(leftmost, body);
} else {
Expand All @@ -124,35 +138,35 @@ private static boolean casesHaveCommonScrutinee(List<Case> cases) {
*/
private static @Nullable EqualityCase buildEqualityCase(ExpressionTree expr, StatementTree body) {
var constantsList = new LinkedList<ExpressionTree>();
String scrutinee = null;
IdentifierTree scrutinee = null;
while (expr.kind() == Kind.CONDITIONAL_OR) {
var binary = (BinaryExpressionTree) expr;
var varAndCst = extractVarAndConstFromEqualityCheck(binary.rightOperand());
if (varAndCst == null) {
return null;
} else if (scrutinee == null) {
scrutinee = varAndCst.a;
} else if (!varAndCst.a.equals(scrutinee)) {
} else if (!varAndCst.a.name().equals(scrutinee.name())) {
return null;
}
constantsList.addFirst(varAndCst.b);
expr = binary.leftOperand();
}
var varAndCst = extractVarAndConstFromEqualityCheck(expr);
if (varAndCst == null || (scrutinee != null && !varAndCst.a.equals(scrutinee))) {
if (varAndCst == null || (scrutinee != null && !varAndCst.a.name().equals(scrutinee.name()))) {
return null;
}
constantsList.addFirst(varAndCst.b);
return new EqualityCase(scrutinee == null ? varAndCst.a : scrutinee, constantsList, body);
}

private static @Nullable Pair<String, ExpressionTree> extractVarAndConstFromEqualityCheck(ExpressionTree expr) {
private static @Nullable Pair<IdentifierTree, ExpressionTree> extractVarAndConstFromEqualityCheck(ExpressionTree expr) {
if (expr.kind() == Kind.EQUAL_TO) {
var binary = (BinaryExpressionTree) expr;
if (binary.leftOperand() instanceof IdentifierTree idTree && isPossibleConstantForCase(binary.rightOperand())) {
return new Pair<>(idTree.name(), binary.rightOperand());
return new Pair<>(idTree, binary.rightOperand());
} else if (binary.rightOperand() instanceof IdentifierTree idTree && isPossibleConstantForCase(binary.leftOperand())) {
return new Pair<>(idTree.name(), binary.leftOperand());
return new Pair<>(idTree, binary.leftOperand());
}
}
return null;
Expand Down Expand Up @@ -191,13 +205,16 @@ private JavaQuickFix computeQuickFix(List<Case> cases, IfStatementTree topLevelI
if (canLiftReturn) {
sb.append("return ");
}
sb.append("switch (").append(cases.get(0).scrutinee()).append(") {\n");
sb.append("switch (").append(cases.get(0).scrutinee().name()).append(") {\n");
for (Case caze : cases) {
sb.append(" ".repeat(baseIndent + INDENT));
writeCase(caze, sb, baseIndent, canLiftReturn);
sb.append("\n");
}
sb.append(" ".repeat(baseIndent)).append("}");
if (canLiftReturn) {
sb.append(";");
}
var edit = JavaTextEdit.replaceTree(topLevelIfStat, sb.toString());
return JavaQuickFix.newQuickFix(ISSUE_MESSAGE).addTextEdit(edit).build();
}
Expand Down Expand Up @@ -267,21 +284,22 @@ private void join(List<? extends Tree> elems, String sep, StringBuilder sb) {
}

private sealed interface Case permits PatternMatchCase, EqualityCase, DefaultCase {
String scrutinee();
IdentifierTree scrutinee();

StatementTree body();
}

private record PatternMatchCase(String scrutinee, PatternTree pattern, List<ExpressionTree> guards, StatementTree body) implements Case {
private record PatternMatchCase(IdentifierTree scrutinee, PatternTree pattern, List<ExpressionTree> guards,
StatementTree body) implements Case {
}

private record EqualityCase(String scrutinee, List<ExpressionTree> constants, StatementTree body) implements Case {
private record EqualityCase(IdentifierTree scrutinee, List<ExpressionTree> constants, StatementTree body) implements Case {
}

/**
* For simplicity the default case should have the same scrutinee as the cases before it
*/
private record DefaultCase(String scrutinee, StatementTree body) implements Case {
private record DefaultCase(IdentifierTree scrutinee, StatementTree body) implements Case {
}

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

0 comments on commit ef66447

Please sign in to comment.