Skip to content

Commit b02c0fe

Browse files
kaufcokaufco
authored
SONARJAVA-4873 Wrong quickfix in S1066 (#4729)
1 parent 5592a80 commit b02c0fe

2 files changed

Lines changed: 129 additions & 22 deletions

File tree

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

Lines changed: 106 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -5,15 +5,17 @@
55

66
public class CollapsibleIfCandidateCheck {
77
private static final Logger LOGGER = Logger.getLogger(CollapsibleIfCandidateCheck.class.getCanonicalName());
8+
89
void testMyFile(File file) {
910
if (file != null) {
1011
if (file.isFile() || file.isDirectory()) { // Noncompliant [[sc=7;ec=9;quickfixes=qf1]]
1112
LOGGER.log(Level.INFO, file.getAbsolutePath());
1213
}
1314
// fix@qf1 {{Merge this if statement with the enclosing one}}
14-
// edit@qf1 [[sl=+2;el=+2;sc=7;ec=8]] {{}}
15-
// edit@qf1 [[sc=47;ec=47]] {{)}}
16-
// edit@qf1 [[sl=-1;el=+0;sc=21;ec=9]] {{ && }}
15+
// edit@qf1 [[sl=-1;el=+0;sc=21;ec=11]] {{ && }}
16+
// edit@qf1 [[sc=11;ec=11]] {{(}}
17+
// edit@qf1 [[sc=46;ec=46]] {{)}}
18+
// edit@qf1 [[sl=+8;el=+8;sc=5;ec=6]] {{}}
1719
}
1820
}
1921

@@ -22,18 +24,113 @@ void noBraceOnOuter(File file) {
2224
if (file.isFile() || file.isDirectory()) { // Noncompliant [[sc=7;ec=9;quickfixes=qf2]]
2325
LOGGER.log(Level.INFO, file.getAbsolutePath());
2426
}
25-
// fix@qf2 {{Merge this if statement with the enclosing one}}
26-
// edit@qf2 [[sc=47;ec=47]] {{)}}
27-
// edit@qf2 [[sl=-1;el=+0;sc=21;ec=9]] {{ && }}
27+
// fix@qf2 {{Merge this if statement with the enclosing one}}
28+
// edit@qf2 [[sl=-1;el=+0;sc=21;ec=11]] {{ && }}
29+
// edit@qf2 [[sc=11;ec=11]] {{(}}
30+
// edit@qf2 [[sc=46;ec=46]] {{)}}
2831
}
2932

3033
void noBraceOnInner(File file) {
3134
if (file != null) {
3235
if (file.isFile() || file.isDirectory()) LOGGER.log(Level.INFO, file.getAbsolutePath()); // Noncompliant [[sc=7;ec=9;quickfixes=qf3]]
3336
// fix@qf3 {{Merge this if statement with the enclosing one}}
34-
// edit@qf3 [[sc=48;ec=48]] {{{}}
35-
// edit@qf3 [[sc=47;ec=47]] {{)}}
36-
// edit@qf3 [[sl=-1;el=+0;sc=21;ec=9]] {{ && }}
37+
// edit@qf3 [[sl=-1;el=+0;sc=21;ec=11]] {{ && }}
38+
// edit@qf3 [[sc=11;ec=11]] {{(}}
39+
// edit@qf3 [[sc=46;ec=46]] {{)}}
40+
// edit@qf3 [[sl=+6;el=+6;sc=5;ec=6]] {{}}
41+
}
42+
}
43+
44+
void leftConditionNeedsParenthesis(boolean a, boolean b, boolean c) {
45+
if (a || b) {
46+
if (c) { // Noncompliant [[sc=7;ec=9;quickfixes=qf4]]
47+
// fix@qf4 {{Merge this if statement with the enclosing one}}
48+
// edit@qf4 [[sl=-1;el=+0;sc=15;ec=11]] {{ && }}
49+
// edit@qf4 [[sl=-1;el=-1;sc=9;ec=9]] {{(}}
50+
// edit@qf4 [[sl=-1;el=-1;sc=15;ec=15]] {{)}}
51+
// edit@qf4 [[sl=+7;el=+7;sc=5;ec=6]] {{}}
52+
}
53+
}
54+
}
55+
56+
void rightConditionNeedsParenthesis(boolean a, boolean c, boolean d) {
57+
if (a) {
58+
if (c || d) { // Noncompliant [[sc=7;ec=9;quickfixes=qf5]]
59+
// fix@qf5 {{Merge this if statement with the enclosing one}}
60+
// edit@qf5 [[sl=-1;el=+0;sc=10;ec=11]] {{ && }}
61+
// edit@qf5 [[sc=11;ec=11]] {{(}}
62+
// edit@qf5 [[sc=17;ec=17]] {{)}}
63+
// edit@qf5 [[sl=+7;el=+7;sc=5;ec=6]] {{}}
64+
}
65+
}
66+
}
67+
68+
void bothConditionsNeedParenthesis(boolean a, boolean b, boolean c, boolean d) {
69+
if (a || b) {
70+
if (c || d) { // Noncompliant [[sc=7;ec=9;quickfixes=qf6]]
71+
// fix@qf6 {{Merge this if statement with the enclosing one}}
72+
// edit@qf6 [[sl=-1;el=+0;sc=15;ec=11]] {{ && }}
73+
// edit@qf6 [[sl=-1;el=-1;sc=9;ec=9]] {{(}}
74+
// edit@qf6 [[sl=-1;el=-1;sc=15;ec=15]] {{)}}
75+
// edit@qf6 [[sc=11;ec=11]] {{(}}
76+
// edit@qf6 [[sc=17;ec=17]] {{)}}
77+
// edit@qf6 [[sl=+9;el=+9;sc=5;ec=6]] {{}}
78+
}
79+
}
80+
}
81+
82+
void noConditionNeedsParenthesis(boolean a, boolean c) {
83+
if (a) {
84+
if (c) { // Noncompliant [[sc=7;ec=9;quickfixes=qf7]]
85+
// fix@qf7 {{Merge this if statement with the enclosing one}}
86+
// edit@qf7 [[sl=-1;el=+0;sc=10;ec=11]] {{ && }}
87+
// edit@qf7 [[sl=+5;el=+5;sc=5;ec=6]] {{}}
88+
}
89+
}
90+
}
91+
92+
void noInnerBlockImpliesSingleStatement(boolean a, boolean c) {
93+
if (a) {
94+
if (c); // Compliant
95+
System.out.println();
96+
}
97+
}
98+
99+
void noBraceOnAny(boolean a, boolean c) {
100+
if (a) if (c); // Noncompliant [[sc=12;ec=14;quickfixes=qf10]]
101+
// fix@qf10 {{Merge this if statement with the enclosing one}}
102+
// edit@qf10 [[sc=10;ec=16]] {{ && }}
103+
}
104+
105+
void operatorsWithLowerPrecedenceCoverage(boolean b, boolean c) {
106+
boolean a;
107+
if (a = b) {
108+
if (c) { // Noncompliant [[sc=7;ec=9;quickfixes=qf11]]
109+
// fix@qf11 {{Merge this if statement with the enclosing one}}
110+
// edit@qf11 [[sl=-1;el=+0;sc=14;ec=11]] {{ && }}
111+
// edit@qf11 [[sl=-1;el=-1;sc=9;ec=9]] {{(}}
112+
// edit@qf11 [[sl=-1;el=-1;sc=14;ec=14]] {{)}}
113+
// edit@qf11 [[sl=+7;el=+7;sc=5;ec=6]] {{}}
114+
}
115+
}
116+
if (a? b: c) {
117+
if (c) { // Noncompliant [[sc=7;ec=9;quickfixes=qf12]]
118+
// fix@qf12 {{Merge this if statement with the enclosing one}}
119+
// edit@qf12 [[sl=-1;el=+0;sc=16;ec=11]] {{ && }}
120+
// edit@qf12 [[sl=-1;el=-1;sc=9;ec=9]] {{(}}
121+
// edit@qf12 [[sl=-1;el=-1;sc=16;ec=16]] {{)}}
122+
// edit@qf12 [[sl=+7;el=+7;sc=5;ec=6]] {{}}
123+
}
124+
}
125+
}
126+
127+
void operatorsWithHigherPrecedenceCoverage(boolean a, boolean b, boolean c) {
128+
if (a | b) {
129+
if (c) { // Noncompliant [[sc=7;ec=9;quickfixes=qf13]]
130+
// fix@qf13 {{Merge this if statement with the enclosing one}}
131+
// edit@qf13 [[sl=-1;el=+0;sc=14;ec=11]] {{ && }}
132+
// edit@qf13 [[sl=+5;el=+5;sc=5;ec=6]] {{}}
133+
}
37134
}
38135
}
39136
}

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

Lines changed: 23 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,11 @@
2929
import org.sonar.plugins.java.api.JavaFileScanner;
3030
import org.sonar.plugins.java.api.JavaFileScannerContext;
3131
import org.sonar.plugins.java.api.tree.BaseTreeVisitor;
32+
import org.sonar.plugins.java.api.tree.BinaryExpressionTree;
3233
import org.sonar.plugins.java.api.tree.BlockTree;
34+
import org.sonar.plugins.java.api.tree.ExpressionTree;
3335
import org.sonar.plugins.java.api.tree.IfStatementTree;
3436
import org.sonar.plugins.java.api.tree.StatementTree;
35-
import org.sonar.plugins.java.api.tree.SyntaxToken;
3637
import org.sonar.plugins.java.api.tree.Tree;
3738

3839
@Rule(key = "S1066")
@@ -95,20 +96,29 @@ private static boolean hasBodySingleIfStatement(StatementTree thenStatement) {
9596
return false;
9697
}
9798

98-
private static JavaQuickFix computeQuickFix(IfStatementTree ifStatement, IfStatementTree outerIf) {
99+
private static JavaQuickFix computeQuickFix(IfStatementTree innerIf, IfStatementTree outerIf) {
99100
var quickFixBuilder = JavaQuickFix.newQuickFix("Merge this if statement with the enclosing one");
100-
StatementTree containingStatement = outerIf.thenStatement();
101-
if (containingStatement.is(Tree.Kind.BLOCK)) {
102-
StatementTree thenStatement = ifStatement.thenStatement();
103-
if (thenStatement.is(Tree.Kind.BLOCK)) {
104-
SyntaxToken closingBrace = ((BlockTree) thenStatement).closeBraceToken();
105-
quickFixBuilder.addTextEdit(JavaTextEdit.removeTree(closingBrace));
106-
} else {
107-
quickFixBuilder.addTextEdit(JavaTextEdit.insertBeforeTree(ifStatement.thenStatement(), "{"));
108-
}
101+
quickFixBuilder.addTextEdit(
102+
JavaTextEdit.replaceBetweenTree(outerIf.condition(), false, innerIf.condition(), false, " && "));
103+
addParenthesisIfRequired(quickFixBuilder, outerIf.condition());
104+
addParenthesisIfRequired(quickFixBuilder, innerIf.condition());
105+
106+
if (outerIf.thenStatement() instanceof BlockTree outerBlock) {
107+
quickFixBuilder.addTextEdit(JavaTextEdit.removeTree(outerBlock.closeBraceToken()));
109108
}
110-
quickFixBuilder.addTextEdit(JavaTextEdit.insertAfterTree(ifStatement.closeParenToken(), ")"));
111-
quickFixBuilder.addTextEdit(JavaTextEdit.replaceBetweenTree(outerIf.closeParenToken(), ifStatement.ifKeyword(), " && "));
112109
return quickFixBuilder.build();
113110
}
111+
112+
private static void addParenthesisIfRequired(JavaQuickFix.Builder quickFixBuilder, ExpressionTree expression) {
113+
if (isLowerOperatorPrecedenceThanLogicalAnd(expression)) {
114+
quickFixBuilder.addTextEdit(JavaTextEdit.insertBeforeTree(expression, "("));
115+
quickFixBuilder.addTextEdit(JavaTextEdit.insertAfterTree(expression, ")"));
116+
}
117+
}
118+
119+
private static boolean isLowerOperatorPrecedenceThanLogicalAnd(ExpressionTree expression) {
120+
return (expression instanceof BinaryExpressionTree binExpression)
121+
? "||".equals(binExpression.operatorToken().text())
122+
: expression.is(Tree.Kind.CONDITIONAL_EXPRESSION, Tree.Kind.ASSIGNMENT);
123+
}
114124
}

0 commit comments

Comments
 (0)