SONARJAVA-6454 : Rule S8714 - implement quickfix

its/autoscan/src/test/resources/autoscan/diffs/diff_S8714.json

@@ -1,6 +1,6 @@
 {
   "ruleKey": "S8714",
   "hasTruePositives": false,
-  "falseNegatives": 42,
+  "falseNegatives": 44,
   "falsePositives": 0
 }

java-checks-test-sources/default/src/test/java/checks/tests/AssertThrowsInsteadOfTryCatchFailCheckSample.java

@@ -14,7 +14,6 @@ void tests() {
     } catch (Exception _) {
       // test passed
     }
-
     try {
       dontRaise();
     } catch (Exception _) {
@@ -86,6 +85,21 @@ void tests() {
       // test passed
     }
 
+
+    try {
+      raise();
+      fail(() -> "expected exception");  // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}}
+//    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    } catch (Exception _) {
+      // test passed
+    }
+
+    try {
+      raise();
+      fail(() -> "expected exception");  // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}}
+//    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+    } finally {}
+
     assertThrows(IllegalStateException.class, AssertThrowsInsteadOfTryCatchFailCheckSample::raise); // compliant
     assertDoesNotThrow(AssertThrowsInsteadOfTryCatchFailCheckSample::dontRaise); // compliant
     nonAnnotatedFunctionFN();
@@ -98,7 +112,12 @@ private void nonAnnotatedFunctionFN() {
   @org.junit.Test
   public void junit4AnnotationDontRaise() {
     try {
-      fail("expected exception"); // TN - junit5 assert in junit4 test
+      fail("expected exception"); // TN - junit5 fail in junit4 test
+    } catch (Exception _) {
+      // test passed
+    }
+    try {
+      org.junit.Assert.fail("expected exception"); // TN - junit4 fail in junit4 test
     } catch (Exception _) {
       // test passed
     }

java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java

@@ -17,16 +17,25 @@
 package org.sonar.java.checks;
 
 import org.sonar.check.Rule;
+import org.sonar.java.checks.helpers.QuickFixHelper;
 import org.sonar.java.checks.helpers.UnitTestUtils;
+import org.sonar.java.reporting.InternalJavaIssueBuilder;
+import org.sonar.java.reporting.JavaQuickFix;
+import org.sonar.java.reporting.JavaTextEdit;
 import org.sonar.plugins.java.api.IssuableSubscriptionVisitor;
+import org.sonar.plugins.java.api.semantic.Type;
 import org.sonar.plugins.java.api.tree.BaseTreeVisitor;
 import org.sonar.plugins.java.api.tree.BlockTree;
 import org.sonar.plugins.java.api.tree.MethodTree;
 import org.sonar.plugins.java.api.tree.Tree;
 import org.sonar.plugins.java.api.tree.TryStatementTree;
+import org.sonar.plugins.java.api.tree.Arguments;
 
+import javax.annotation.Nullable;
 import java.util.List;
 
+import static org.sonar.java.checks.helpers.TryCatchUtils.getCaughtTypes;
+
 @Rule(key = "S8714")
 public class AssertThrowsInsteadOfTryCatchFailCheck extends IssuableSubscriptionVisitor {
 
@@ -44,26 +53,131 @@ public void visitNode(Tree tree) {
   }
 
   private final class TryStatementsVisitor extends BaseTreeVisitor {
-    private final boolean isJunitJupiter;
+    private final boolean hasJunitJupiterTestAnnotation;
 
-    public TryStatementsVisitor(boolean isJunitJupiter) {
-      this.isJunitJupiter = isJunitJupiter;
+    public TryStatementsVisitor(boolean hasJunitJupiterTestAnnotation) {
+      this.hasJunitJupiterTestAnnotation = hasJunitJupiterTestAnnotation;
     }
 
     @Override
     public void visitTryStatement(TryStatementTree tree) {
-      checkBlock(tree.block(), "Use assertThrows() instead of try/catch and fail() in the try block.");
-      tree.catches().forEach(c -> checkBlock(c.block(), "Use assertDoesNotThrow() instead of try/catch and fail() in the catch block."));
+      checkBlock(tree.block(), tree, true);
+      tree.catches().forEach(c ->
+        checkBlock(c.block(), tree, false)
+      );
       super.visitTryStatement(tree);
     }
 
-    private void checkBlock(BlockTree block, String message) {
+    private void checkBlock(
+      BlockTree block,
+      TryStatementTree tryStatement,
+      boolean isTryBlock
+    ) {
       UnitTestUtils.findFail(block).ifPresent(failMethodInvocation -> {
-          if (isJunitJupiter || failMethodInvocation.methodSymbol().signature().contains("org.assertj")) {
-            reportIssue(failMethodInvocation, message);
+          String issueMessage = isTryBlock ?
+            "Use assertThrows() instead of try/catch and fail() in the try block." :
+            "Use assertDoesNotThrow() instead of try/catch and fail() in the catch block.";
+
+          Arguments failArguments = failMethodInvocation.arguments();
+
+          InternalJavaIssueBuilder issueBuilder = QuickFixHelper
+            .newIssue(context)
+            .forRule(AssertThrowsInsteadOfTryCatchFailCheck.this)
+            .onTree(failMethodInvocation)
+            .withMessage(issueMessage);
+
+          var isAssertJ = failMethodInvocation.methodSymbol().signature().contains("org.assertj");
+          if (hasJunitJupiterTestAnnotation || isAssertJ) {
+            Replacements replacements = isAssertJ ?
+              assertJReplacement(failArguments, tryStatement, isTryBlock) :
+              junitReplacement(failArguments, tryStatement, isTryBlock);
+            var firstCatchFinallyToken = tryStatement.catches().isEmpty() ?
+              tryStatement.finallyKeyword().firstToken() :
+              tryStatement.catches().get(0).firstToken();
+            var lastCatchFinallyToken = tryStatement.finallyBlock() != null ?
+              tryStatement.finallyBlock().lastToken() :
+              tryStatement.catches().get(tryStatement.catches().size() - 1).block().lastToken();
+            issueBuilder.withQuickFix(() ->
+              JavaQuickFix.newQuickFix(issueMessage).addTextEdit(
+                JavaTextEdit.replaceTree(tryStatement.tryKeyword(), replacements.replaceTryWith),
+                JavaTextEdit.replaceTree(failMethodInvocation.parent(), ""),
+                JavaTextEdit.replaceBetweenTree(
+                  firstCatchFinallyToken,
+                  lastCatchFinallyToken,
+                  replacements.replaceCatchesWith
+                )
+              ).build()
+            );
+            issueBuilder.report();
           }
         }
       );
     }
+
+    private Replacements junitReplacement(
+      Arguments failArguments,
+      TryStatementTree tryStatement,
+      boolean isTryBlock
+    ) {
+
+      String argumentsSuffix = failArguments.stream().findFirst().filter(argument ->
+        {
+          Type symbolType = argument.symbolType();
+          return !symbolType.isUnknown() && (symbolType.is("java.lang.String") || symbolType.isSubtypeOf("java.util.function.Supplier<java.lang.String>"));
+        }
+      ).map(argument ->
+        ", %s".formatted(contentFor(argument))
+      ).orElse("");
+
+      return isTryBlock ?
+        new Replacements(
+          "assertThrows(%s, () -> ".formatted(typeClass(firstCaughtTypeInTry(tryStatement))),
+          "%s);".formatted(argumentsSuffix)
+        ) :
+        new Replacements(
+          "assertDoesNotThrow(() -> ",
+          "%s);".formatted(argumentsSuffix)
+        );
+    }
+
+    private Replacements assertJReplacement(
+      Arguments failArguments,
+      TryStatementTree tryStatement,
+      boolean isTryBlock
+    ) {
+      // in assertJ the failure message is mandatory
+      var failureMessage = contentFor(failArguments.get(0));
+      return isTryBlock ?
+        new Replacements(
+          "assertThatCode(() -> ",
+          ").withFailMessage(%s).isInstanceOf(%s);".formatted(failureMessage, typeClass(firstCaughtTypeInTry(tryStatement)))
+        ) :
+        new Replacements(
+          "assertThatCode(() -> ",
+          ").withFailMessage(%s).doesNotThrowAnyException();".formatted(failureMessage)
+        );
+    }
+
+    private String contentFor(Tree tree) {
+      return QuickFixHelper.contentForTree(tree, context);
+    }
+
+    private record Replacements(String replaceTryWith, String replaceCatchesWith) {
+    }
   }
+
+
+  private static String typeClass(@Nullable Type caughtType) {
+    if (caughtType == null) return "Throwable";
+    return caughtType.name() + ".class";
+  }
+
+  @Nullable
+  private static Type firstCaughtTypeInTry(TryStatementTree tryStatement) {
+    if (!tryStatement.catches().isEmpty()) {
+      return getCaughtTypes(tryStatement.catches().get(0)).get(0);
+    }
+    return null;
+  }
+
 }

java-checks/src/main/java/org/sonar/java/checks/InterruptedExceptionCheck.java

@@ -42,10 +42,9 @@
 import org.sonar.plugins.java.api.tree.ThrowStatementTree;
 import org.sonar.plugins.java.api.tree.Tree;
 import org.sonar.plugins.java.api.tree.TryStatementTree;
-import org.sonar.plugins.java.api.tree.TypeTree;
-import org.sonar.plugins.java.api.tree.UnionTypeTree;
 import org.sonar.plugins.java.api.tree.VariableTree;
 
+import static org.sonar.java.checks.helpers.TryCatchUtils.getCaughtTypes;
 import static org.sonar.java.model.ExpressionUtils.extractIdentifierSymbol;
 
 @Rule(key = "S2142")
@@ -85,7 +84,7 @@ public void visitNode(Tree tree) {
 
     for (CatchTree catchTree : tryStatementTree.catches()) {
       VariableTree catchParameter = catchTree.parameter();
-      List<Type> caughtTypes = getCaughtTypes(catchParameter);
+      List<Type> caughtTypes = getCaughtTypes(catchTree);
 
       Optional<Type> interruptType = caughtTypes.stream().filter(INTERRUPTING_TYPE_PREDICATE).findFirst();
       if (interruptType.isPresent()) {
@@ -121,15 +120,6 @@ private boolean wasNotInterrupted(CatchTree catchTree) {
     return !blockVisitor.threadInterrupted && !isWithinInterruptingFinally();
   }
 
-  private static List<Type> getCaughtTypes(VariableTree parameter) {
-    if (parameter.type().is(Tree.Kind.UNION_TYPE)) {
-      return ((UnionTypeTree) parameter.type()).typeAlternatives().stream()
-        .map(TypeTree::symbolType)
-        .toList();
-    }
-    return Collections.singletonList(parameter.symbol().type());
-  }
-
   private boolean isWithinInterruptingFinally() {
     return withinInterruptingFinally.stream().anyMatch(Boolean.TRUE::equals);
   }
@@ -173,7 +163,7 @@ public void visitTryStatement(TryStatementTree tryStatementTree) {
       // interruptions thrown there must be handled within the scope of the outer try.
 
       boolean isCatchingAnyInterruptingTypes = tryStatementTree.catches().stream()
-        .anyMatch(catchTree -> getCaughtTypes(catchTree.parameter()).stream().anyMatch(INTERRUPTING_TYPE_PREDICATE));
+        .anyMatch(catchTree -> getCaughtTypes(catchTree).stream().anyMatch(INTERRUPTING_TYPE_PREDICATE));
 
       scan(tryStatementTree.resourceList());
       if (!isCatchingAnyInterruptingTypes) {

java-checks/src/main/java/org/sonar/java/checks/helpers/TryCatchUtils.java

@@ -0,0 +1,41 @@
+/*
+ * SonarQube Java
+ * Copyright (C) SonarSource Sàrl
+ * mailto:info AT sonarsource DOT com
+ *
+ * You can redistribute and/or modify this program under the terms of
+ * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the Sonar Source-Available License for more details.
+ *
+ * You should have received a copy of the Sonar Source-Available License
+ * along with this program; if not, see https://sonarsource.com/license/ssal/
+ */
+package org.sonar.java.checks.helpers;
+
+import org.sonar.plugins.java.api.semantic.Type;
+import org.sonar.plugins.java.api.tree.CatchTree;
+import org.sonar.plugins.java.api.tree.UnionTypeTree;
+import org.sonar.plugins.java.api.tree.TypeTree;
+import org.sonar.plugins.java.api.tree.VariableTree;
+
+import java.util.List;
+
+public final class TryCatchUtils {
+  private TryCatchUtils() {
+    /* This utility class should not be instantiated */
+  }
+
+  public static List<Type> getCaughtTypes(CatchTree tree) {
+    VariableTree parameter = tree.parameter();
+    if (parameter.type() instanceof UnionTypeTree unionTypeTree) {
+      return unionTypeTree.typeAlternatives().stream()
+        .map(TypeTree::symbolType)
+        .toList();
+    }
+    return List.of(parameter.symbol().type());
+  }
+}

java-checks/src/test/java/org/sonar/java/checks/helpers/TryCatchUtilsTest.java

@@ -0,0 +1,44 @@
+/*
+ * SonarQube Java
+ * Copyright (C) SonarSource Sàrl
+ * mailto:info AT sonarsource DOT com
+ *
+ * You can redistribute and/or modify this program under the terms of
+ * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * See the Sonar Source-Available License for more details.
+ *
+ * You should have received a copy of the Sonar Source-Available License
+ * along with this program; if not, see https://sonarsource.com/license/ssal/
+ */
+package org.sonar.java.checks.helpers;
+
+import org.junit.jupiter.api.Test;
+import org.sonar.plugins.java.api.tree.ClassTree;
+import org.sonar.plugins.java.api.tree.CompilationUnitTree;
+import org.sonar.plugins.java.api.tree.MethodTree;
+import org.sonar.plugins.java.api.tree.TryStatementTree;
+
+import static org.assertj.core.api.Assertions.assertThat;
+
+
+class TryCatchUtilsTest {
+
+  @Test
+  void testGetCaughtTypes() {
+    assertThat(TryCatchUtils.getCaughtTypes(
+      parseTry("try {} catch (ExceptionABCD e) {} ").catches().get(0)
+    )).first().matches(type -> type.name().equals("ExceptionABCD"));
+  }
+
+  private TryStatementTree parseTry(String code) {
+    CompilationUnitTree compilationUnitTree = JParserTestUtils.parse("void main() { %s }".formatted(code));
+    ClassTree classTree = (ClassTree) compilationUnitTree.types().get(0);
+    MethodTree methodTree = (MethodTree) classTree.members().get(0);
+    return (TryStatementTree) methodTree.block().body().get(0);
+  }
+}
+