SONARJAVA-4961 Fix quickfix parenthesis on conditional expressions (#…

…4863)
SonarSource · Sep 19, 2024 · 93106f5 · 93106f5
1 parent cbd3b34
commit 93106f5
Show file tree

Hide file tree

Showing 5 changed files with 65 additions and 16 deletions.
diff --git a/...t-sources/default/src/main/java/checks/SingleIfInsteadOfPatternMatchGuardCheckSample.java b/...t-sources/default/src/main/java/checks/SingleIfInsteadOfPatternMatchGuardCheckSample.java
@@ -2,6 +2,24 @@
 
 public class SingleIfInsteadOfPatternMatchGuardCheckSample {
 
+  // fix@qf4 {{Merge this "if" statement with the enclosing pattern match guard.}}
+  // edit@qf4 [[sl=+0;el=+2;sc=9;ec=10]] {{System.out.println("even");}}
+  // edit@qf4 [[sl=-1;el=-1;sc=44;ec=44]] {{ && x % 2 == 0 }}
+  // edit@qf4 [[sl=-1;el=-1;sc=28;ec=28]] {{(}}
+  // edit@qf4 [[sl=-1;el=-1;sc=43;ec=43]] {{)}}
+  void test(Foo foo){
+    switch (foo){
+      case Foo(var x) when x < 0 || x > 10 -> {
+        if (x % 2 == 0) { // Noncompliant [[quickfixes=qf4]]
+          System.out.println("even");
+        }
+      }
+      default -> {}
+    }
+  }
+
+  record Foo (int x) {}
+
   // fix@qf3 {{Merge this "if" statement with the enclosing pattern match guard.}}
   // edit@qf3 [[sl=+0;el=+2;sc=9;ec=10]] {{System.out.println("two");}}
   // edit@qf3 [[sl=-2;el=-2;sc=44;ec=44]] {{ && s.length() == 2 }}

diff --git a/java-checks/src/main/java/org/sonar/java/checks/CollapsibleIfCandidateCheck.java b/java-checks/src/main/java/org/sonar/java/checks/CollapsibleIfCandidateCheck.java
@@ -29,9 +29,7 @@
 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.Tree;
@@ -100,25 +98,13 @@ private static JavaQuickFix computeQuickFix(IfStatementTree innerIf, IfStatement
     var quickFixBuilder = JavaQuickFix.newQuickFix("Merge this if statement with the enclosing one");
     quickFixBuilder.addTextEdit(
       JavaTextEdit.replaceBetweenTree(outerIf.condition(), false, innerIf.condition(), false, " && "));
-    addParenthesisIfRequired(quickFixBuilder, outerIf.condition());
-    addParenthesisIfRequired(quickFixBuilder, innerIf.condition());
+    QuickFixHelper.addParenthesisIfRequired(quickFixBuilder, outerIf.condition());
+    QuickFixHelper.addParenthesisIfRequired(quickFixBuilder, innerIf.condition());
 
     if (outerIf.thenStatement() instanceof BlockTree outerBlock) {
       quickFixBuilder.addTextEdit(JavaTextEdit.removeTree(outerBlock.closeBraceToken()));
     }
     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);
-  }
 }
diff --git a/java-checks/src/main/java/org/sonar/java/checks/SingleIfInsteadOfPatternMatchGuardCheck.java b/java-checks/src/main/java/org/sonar/java/checks/SingleIfInsteadOfPatternMatchGuardCheck.java
@@ -112,6 +112,12 @@ private static JavaQuickFix computeQuickFix(IfStatementTree ifStatement, CaseLab
       JavaTextEdit.insertBeforeTree(caseLabel.colonOrArrowToken(),
         replacementStringPrefix + QuickFixHelper.contentForTree(ifStatement.condition(), context) + " ")
     );
+    QuickFixHelper.addParenthesisIfRequired(quickFixBuilder, ifStatement.condition());
+    for(var expr : caseLabel.expressions()) {
+      if(expr instanceof GuardedPatternTree guardedPattern) {
+        QuickFixHelper.addParenthesisIfRequired(quickFixBuilder, guardedPattern.expression());
+      }
+    }
     return quickFixBuilder.build();
   }
 

diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/QuickFixHelper.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/QuickFixHelper.java
@@ -34,9 +34,11 @@
 import org.sonar.java.model.DefaultJavaFileScannerContext;
 import org.sonar.java.model.JavaTree;
 import org.sonar.java.reporting.InternalJavaIssueBuilder;
+import org.sonar.java.reporting.JavaQuickFix;
 import org.sonar.java.reporting.JavaTextEdit;
 import org.sonar.plugins.java.api.JavaFileScannerContext;
 import org.sonar.plugins.java.api.location.Position;
+import org.sonar.plugins.java.api.tree.BinaryExpressionTree;
 import org.sonar.plugins.java.api.tree.CaseGroupTree;
 import org.sonar.plugins.java.api.tree.ClassTree;
 import org.sonar.plugins.java.api.tree.CompilationUnitTree;
@@ -238,6 +240,18 @@ public static String contentOfTreeTokens(Tree tree, JavaFileScannerContext conte
     return sb.toString();
   }
 
+  public 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);
+  }
 
   /**
    * Check if a given type "requiredType" is available in the current "context". Imports are cached to not have to recompute order all the time.

diff --git a/java-checks/src/test/java/org/sonar/java/checks/helpers/QuickFixHelperTest.java b/java-checks/src/test/java/org/sonar/java/checks/helpers/QuickFixHelperTest.java
@@ -32,11 +32,13 @@
 import org.sonar.java.Preconditions;
 import org.sonar.java.checks.helpers.QuickFixHelper.ImportSupplier;
 import org.sonar.java.model.InternalSyntaxToken;
+import org.sonar.java.reporting.JavaQuickFix;
 import org.sonar.java.reporting.JavaTextEdit;
 import org.sonar.plugins.java.api.JavaFileScannerContext;
 import org.sonar.plugins.java.api.tree.BaseTreeVisitor;
 import org.sonar.plugins.java.api.tree.ClassTree;
 import org.sonar.plugins.java.api.tree.CompilationUnitTree;
+import org.sonar.plugins.java.api.tree.IfStatementTree;
 import org.sonar.plugins.java.api.tree.InferedTypeTree;
 import org.sonar.plugins.java.api.tree.LiteralTree;
 import org.sonar.plugins.java.api.tree.MethodTree;
@@ -199,6 +201,29 @@ void previousToken() {
     assertThat(QuickFixHelper.previousToken(a.declarationKeyword())).isEqualTo(a.declarationKeyword());
   }
 
+  @Test
+  void addParenthesisIfRequired(){
+    CompilationUnitTree cut = JParserTestUtils.parse("class A { void foo(boolean x, boolean y) { " +
+      "if(x && y){} " +
+      "if(x || y){} " +
+      "if(x = 0){}" +
+      "} }");
+    ClassTree a = (ClassTree) cut.types().get(0);
+    MethodTree foo = (MethodTree) a.members().get(0);
+    IfStatementTree ifWithAND = (IfStatementTree) foo.block().body().get(0);
+    var builder = JavaQuickFix.newQuickFix("");
+    QuickFixHelper.addParenthesisIfRequired(builder, ifWithAND.condition());
+    assertThat(builder.build().getTextEdits()).isEmpty();
+
+    IfStatementTree ifWithOR = (IfStatementTree) foo.block().body().get(1);
+    QuickFixHelper.addParenthesisIfRequired(builder, ifWithOR.condition());
+    assertThat(builder.build().getTextEdits()).hasSize(2);
+
+    IfStatementTree ifWithAssignment = (IfStatementTree) foo.block().body().get(2);
+    QuickFixHelper.addParenthesisIfRequired(builder, ifWithAssignment.condition());
+    assertThat(builder.build().getTextEdits()).hasSize(4);
+  }
+
   @Test
   void content_for_empty_token() {
     String content = QuickFixHelper.contentForTree(new InferedTypeTree(), mock(JavaFileScannerContext.class));