Skip to content

Commit

Permalink
SONARJAVA-4873 Wrong quickfix in S1066 (#4729)
Browse files Browse the repository at this point in the history
  • Loading branch information
@kaufco
kaufco committed Mar 21, 2024
1 parent 5592a80 commit b02c0fe
Show file tree
Hide file tree
Showing 2 changed files with 129 additions and 22 deletions.
115 changes: 106 additions & 9 deletions java-checks-test-sources/default/src/main/java/checks/CollapsibleIfCandidateCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,17 @@

public class CollapsibleIfCandidateCheck {
private static final Logger LOGGER = Logger.getLogger(CollapsibleIfCandidateCheck.class.getCanonicalName());

void testMyFile(File file) {
if (file != null) {
if (file.isFile() || file.isDirectory()) { // Noncompliant [[sc=7;ec=9;quickfixes=qf1]]
LOGGER.log(Level.INFO, file.getAbsolutePath());
}
// fix@qf1 {{Merge this if statement with the enclosing one}}
// edit@qf1 [[sl=+2;el=+2;sc=7;ec=8]] {{}}
// edit@qf1 [[sc=47;ec=47]] {{)}}
// edit@qf1 [[sl=-1;el=+0;sc=21;ec=9]] {{ && }}
// edit@qf1 [[sl=-1;el=+0;sc=21;ec=11]] {{ && }}
// edit@qf1 [[sc=11;ec=11]] {{(}}
// edit@qf1 [[sc=46;ec=46]] {{)}}
// edit@qf1 [[sl=+8;el=+8;sc=5;ec=6]] {{}}
}
}

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

void noBraceOnInner(File file) {
if (file != null) {
if (file.isFile() || file.isDirectory()) LOGGER.log(Level.INFO, file.getAbsolutePath()); // Noncompliant [[sc=7;ec=9;quickfixes=qf3]]
// fix@qf3 {{Merge this if statement with the enclosing one}}
// edit@qf3 [[sc=48;ec=48]] {{{}}
// edit@qf3 [[sc=47;ec=47]] {{)}}
// edit@qf3 [[sl=-1;el=+0;sc=21;ec=9]] {{ && }}
// edit@qf3 [[sl=-1;el=+0;sc=21;ec=11]] {{ && }}
// edit@qf3 [[sc=11;ec=11]] {{(}}
// edit@qf3 [[sc=46;ec=46]] {{)}}
// edit@qf3 [[sl=+6;el=+6;sc=5;ec=6]] {{}}
}
}

void leftConditionNeedsParenthesis(boolean a, boolean b, boolean c) {
if (a || b) {
if (c) { // Noncompliant [[sc=7;ec=9;quickfixes=qf4]]
// fix@qf4 {{Merge this if statement with the enclosing one}}
// edit@qf4 [[sl=-1;el=+0;sc=15;ec=11]] {{ && }}
// edit@qf4 [[sl=-1;el=-1;sc=9;ec=9]] {{(}}
// edit@qf4 [[sl=-1;el=-1;sc=15;ec=15]] {{)}}
// edit@qf4 [[sl=+7;el=+7;sc=5;ec=6]] {{}}
}
}
}

void rightConditionNeedsParenthesis(boolean a, boolean c, boolean d) {
if (a) {
if (c || d) { // Noncompliant [[sc=7;ec=9;quickfixes=qf5]]
// fix@qf5 {{Merge this if statement with the enclosing one}}
// edit@qf5 [[sl=-1;el=+0;sc=10;ec=11]] {{ && }}
// edit@qf5 [[sc=11;ec=11]] {{(}}
// edit@qf5 [[sc=17;ec=17]] {{)}}
// edit@qf5 [[sl=+7;el=+7;sc=5;ec=6]] {{}}
}
}
}

void bothConditionsNeedParenthesis(boolean a, boolean b, boolean c, boolean d) {
if (a || b) {
if (c || d) { // Noncompliant [[sc=7;ec=9;quickfixes=qf6]]
// fix@qf6 {{Merge this if statement with the enclosing one}}
// edit@qf6 [[sl=-1;el=+0;sc=15;ec=11]] {{ && }}
// edit@qf6 [[sl=-1;el=-1;sc=9;ec=9]] {{(}}
// edit@qf6 [[sl=-1;el=-1;sc=15;ec=15]] {{)}}
// edit@qf6 [[sc=11;ec=11]] {{(}}
// edit@qf6 [[sc=17;ec=17]] {{)}}
// edit@qf6 [[sl=+9;el=+9;sc=5;ec=6]] {{}}
}
}
}

void noConditionNeedsParenthesis(boolean a, boolean c) {
if (a) {
if (c) { // Noncompliant [[sc=7;ec=9;quickfixes=qf7]]
// fix@qf7 {{Merge this if statement with the enclosing one}}
// edit@qf7 [[sl=-1;el=+0;sc=10;ec=11]] {{ && }}
// edit@qf7 [[sl=+5;el=+5;sc=5;ec=6]] {{}}
}
}
}

void noInnerBlockImpliesSingleStatement(boolean a, boolean c) {
if (a) {
if (c); // Compliant
System.out.println();
}
}

void noBraceOnAny(boolean a, boolean c) {
if (a) if (c); // Noncompliant [[sc=12;ec=14;quickfixes=qf10]]
// fix@qf10 {{Merge this if statement with the enclosing one}}
// edit@qf10 [[sc=10;ec=16]] {{ && }}
}

void operatorsWithLowerPrecedenceCoverage(boolean b, boolean c) {
boolean a;
if (a = b) {
if (c) { // Noncompliant [[sc=7;ec=9;quickfixes=qf11]]
// fix@qf11 {{Merge this if statement with the enclosing one}}
// edit@qf11 [[sl=-1;el=+0;sc=14;ec=11]] {{ && }}
// edit@qf11 [[sl=-1;el=-1;sc=9;ec=9]] {{(}}
// edit@qf11 [[sl=-1;el=-1;sc=14;ec=14]] {{)}}
// edit@qf11 [[sl=+7;el=+7;sc=5;ec=6]] {{}}
}
}
if (a? b: c) {
if (c) { // Noncompliant [[sc=7;ec=9;quickfixes=qf12]]
// fix@qf12 {{Merge this if statement with the enclosing one}}
// edit@qf12 [[sl=-1;el=+0;sc=16;ec=11]] {{ && }}
// edit@qf12 [[sl=-1;el=-1;sc=9;ec=9]] {{(}}
// edit@qf12 [[sl=-1;el=-1;sc=16;ec=16]] {{)}}
// edit@qf12 [[sl=+7;el=+7;sc=5;ec=6]] {{}}
}
}
}

void operatorsWithHigherPrecedenceCoverage(boolean a, boolean b, boolean c) {
if (a | b) {
if (c) { // Noncompliant [[sc=7;ec=9;quickfixes=qf13]]
// fix@qf13 {{Merge this if statement with the enclosing one}}
// edit@qf13 [[sl=-1;el=+0;sc=14;ec=11]] {{ && }}
// edit@qf13 [[sl=+5;el=+5;sc=5;ec=6]] {{}}
}
}
}
}
36 changes: 23 additions & 13 deletions java-checks/src/main/java/org/sonar/java/checks/CollapsibleIfCandidateCheck.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,11 @@
import org.sonar.plugins.java.api.JavaFileScanner;
import org.sonar.plugins.java.api.JavaFileScannerContext;
import org.sonar.plugins.java.api.tree.BaseTreeVisitor;
import org.sonar.plugins.java.api.tree.BinaryExpressionTree;
import org.sonar.plugins.java.api.tree.BlockTree;
import org.sonar.plugins.java.api.tree.ExpressionTree;
import org.sonar.plugins.java.api.tree.IfStatementTree;
import org.sonar.plugins.java.api.tree.StatementTree;
import org.sonar.plugins.java.api.tree.SyntaxToken;
import org.sonar.plugins.java.api.tree.Tree;

@Rule(key = "S1066")
Expand Down Expand Up @@ -95,20 +96,29 @@ private static boolean hasBodySingleIfStatement(StatementTree thenStatement) {
return false;
}

private static JavaQuickFix computeQuickFix(IfStatementTree ifStatement, IfStatementTree outerIf) {
private static JavaQuickFix computeQuickFix(IfStatementTree innerIf, IfStatementTree outerIf) {
var quickFixBuilder = JavaQuickFix.newQuickFix("Merge this if statement with the enclosing one");
StatementTree containingStatement = outerIf.thenStatement();
if (containingStatement.is(Tree.Kind.BLOCK)) {
StatementTree thenStatement = ifStatement.thenStatement();
if (thenStatement.is(Tree.Kind.BLOCK)) {
SyntaxToken closingBrace = ((BlockTree) thenStatement).closeBraceToken();
quickFixBuilder.addTextEdit(JavaTextEdit.removeTree(closingBrace));
} else {
quickFixBuilder.addTextEdit(JavaTextEdit.insertBeforeTree(ifStatement.thenStatement(), "{"));
}
quickFixBuilder.addTextEdit(
JavaTextEdit.replaceBetweenTree(outerIf.condition(), false, innerIf.condition(), false, " && "));
addParenthesisIfRequired(quickFixBuilder, outerIf.condition());
addParenthesisIfRequired(quickFixBuilder, innerIf.condition());

if (outerIf.thenStatement() instanceof BlockTree outerBlock) {
quickFixBuilder.addTextEdit(JavaTextEdit.removeTree(outerBlock.closeBraceToken()));
}
quickFixBuilder.addTextEdit(JavaTextEdit.insertAfterTree(ifStatement.closeParenToken(), ")"));
quickFixBuilder.addTextEdit(JavaTextEdit.replaceBetweenTree(outerIf.closeParenToken(), ifStatement.ifKeyword(), " && "));
return quickFixBuilder.build();
}

private static void addParenthesisIfRequired(JavaQuickFix.Builder quickFixBuilder, ExpressionTree expression) {
if (isLowerOperatorPrecedenceThanLogicalAnd(expression)) {
quickFixBuilder.addTextEdit(JavaTextEdit.insertBeforeTree(expression, "("));
quickFixBuilder.addTextEdit(JavaTextEdit.insertAfterTree(expression, ")"));
}
}

private static boolean isLowerOperatorPrecedenceThanLogicalAnd(ExpressionTree expression) {
return (expression instanceof BinaryExpressionTree binExpression)
? "||".equals(binExpression.operatorToken().text())
: expression.is(Tree.Kind.CONDITIONAL_EXPRESSION, Tree.Kind.ASSIGNMENT);
}
}

0 comments on commit b02c0fe

Please sign in to comment.