diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S8714.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S8714.json index 48ee9d30c1a..1cc8dfa0261 100644 --- a/its/autoscan/src/test/resources/autoscan/diffs/diff_S8714.json +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S8714.json @@ -1,6 +1,6 @@ { "ruleKey": "S8714", "hasTruePositives": false, - "falseNegatives": 42, + "falseNegatives": 44, "falsePositives": 0 } diff --git a/java-checks-test-sources/default/src/test/java/checks/tests/AssertThrowsInsteadOfTryCatchFailCheckSample.java b/java-checks-test-sources/default/src/test/java/checks/tests/AssertThrowsInsteadOfTryCatchFailCheckSample.java index 04a4e874ed4..8fbe077fd4a 100644 --- a/java-checks-test-sources/default/src/test/java/checks/tests/AssertThrowsInsteadOfTryCatchFailCheckSample.java +++ b/java-checks-test-sources/default/src/test/java/checks/tests/AssertThrowsInsteadOfTryCatchFailCheckSample.java @@ -9,11 +9,15 @@ public class AssertThrowsInsteadOfTryCatchFailCheckSample { void tests() { try { raise(); - fail(); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} + fail(); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} [[quickfixes=qf1]] // ^^^^^^ } catch (Exception _) { // test passed } + // fix@qf1 {{Use assertThrows() instead of try/catch and fail() in the try block.}} + // edit@qf1 [[sl=10;sc=5;el=10;ec=8]] {{assertThrows(Exception.class, () -> }} + // edit@qf1 [[sl=12;sc=7;el=12;ec=14]] {{}} + // edit@qf1 [[sl=14;sc=7;el=16;ec=6]] {{);}} try { dontRaise(); @@ -48,11 +52,15 @@ void tests() { try { raise(); - org.assertj.core.api.Fail.fail("expected exception"); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} + org.assertj.core.api.Fail.fail("expected exception"); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} [[quickfixes=qf2]] // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ } catch (Exception _) { // test passed } + // fix@qf2 {{Use assertThrows() instead of try/catch and fail() in the try block.}} + // edit@qf2 [[sl=53;sc=5;el=53;ec=8]] {{assertThatCode(() -> }} + // edit@qf2 [[sl=55;sc=7;el=55;ec=60]] {{}} + // edit@qf2 [[sl=57;sc=7;el=59;ec=6]] {{).withFailMessage("expected exception").isInstanceOf(Exception.class);}} try { raise(); @@ -86,6 +94,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 +121,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 } diff --git a/java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java b/java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java index 160948e5a71..d72208c32ce 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java @@ -17,16 +17,26 @@ 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.MethodInvocationTree; 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 +54,148 @@ 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); + + var isAssertJ = failMethodInvocation.methodSymbol().signature().contains("org.assertj"); + if (hasJunitJupiterTestAnnotation || isAssertJ) { + 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."; + InternalJavaIssueBuilder issueBuilder = QuickFixHelper + .newIssue(context) + .forRule(AssertThrowsInsteadOfTryCatchFailCheck.this) + .onTree(failMethodInvocation) + .withMessage(issueMessage) + .withQuickFix(() -> + provideQuickFix( + tryStatement, + failMethodInvocation, + issueMessage, + isAssertJ, + isTryBlock + ) + ); + issueBuilder.report(); } } ); } + + private JavaQuickFix provideQuickFix( + TryStatementTree tryStatement, + MethodInvocationTree failMethodInvocation, + String issueMessage, + boolean isAssertJ, + boolean isTryBlock + ) { + Arguments failArguments = failMethodInvocation.arguments(); + + 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(); + + return JavaQuickFix.newQuickFix(issueMessage).addTextEdit( + JavaTextEdit.replaceTree(tryStatement.tryKeyword(), replacements.replaceTryWith), + JavaTextEdit.replaceTree(failMethodInvocation.parent(), ""), + JavaTextEdit.replaceBetweenTree( + firstCatchFinallyToken, + lastCatchFinallyToken, + replacements.replaceCatchesWith + ) + ).build(); + } + + 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")); + } + ).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; + } + } diff --git a/java-checks/src/main/java/org/sonar/java/checks/InterruptedExceptionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/InterruptedExceptionCheck.java index e06703c07d0..9bf3c23e991 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/InterruptedExceptionCheck.java +++ b/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 caughtTypes = getCaughtTypes(catchParameter); + List caughtTypes = getCaughtTypes(catchTree); Optional 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 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) { diff --git a/java-checks/src/main/java/org/sonar/java/checks/helpers/TryCatchUtils.java b/java-checks/src/main/java/org/sonar/java/checks/helpers/TryCatchUtils.java new file mode 100644 index 00000000000..7f7d79c3f57 --- /dev/null +++ b/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 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()); + } +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/helpers/TryCatchUtilsTest.java b/java-checks/src/test/java/org/sonar/java/checks/helpers/TryCatchUtilsTest.java new file mode 100644 index 00000000000..12c542bb99e --- /dev/null +++ b/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); + } +} +