From 211440a254592df8c659dbc200477e25f5bbe1e7 Mon Sep 17 00:00:00 2001 From: rombirli Date: Tue, 2 Jun 2026 16:44:49 +0200 Subject: [PATCH 01/27] quickfix skeleton --- ...ssertThrowsInsteadOfTryCatchFailCheck.java | 50 ++++++++++++++++--- 1 file changed, 42 insertions(+), 8 deletions(-) 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..ec9f0b48322 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,7 +17,10 @@ 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.JavaQuickFix; +import org.sonar.java.reporting.JavaTextEdit; import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.tree.BaseTreeVisitor; import org.sonar.plugins.java.api.tree.BlockTree; @@ -25,7 +28,9 @@ import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.TryStatementTree; +import javax.annotation.Nullable; import java.util.List; +import java.util.Optional; @Rule(key = "S8714") public class AssertThrowsInsteadOfTryCatchFailCheck extends IssuableSubscriptionVisitor { @@ -44,24 +49,53 @@ public void visitNode(Tree tree) { } private final class TryStatementsVisitor extends BaseTreeVisitor { - private final boolean isJunitJupiter; + private final boolean isJunit56; - public TryStatementsVisitor(boolean isJunitJupiter) { - this.isJunitJupiter = isJunitJupiter; + public TryStatementsVisitor(boolean isJunit56) { + this.isJunit56 = isJunit56; } @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, "Use assertThrows() instead of try/catch and fail() in the try block."); + tree.catches().forEach(c -> + checkBlock(c.block(), tree, false, "Use assertDoesNotThrow() instead of try/catch and fail() in the catch block.") + ); super.visitTryStatement(tree); } - private void checkBlock(BlockTree block, String message) { + private void checkBlock(BlockTree block, TryStatementTree tryStatement, boolean isTryBlock, String message) { UnitTestUtils.findFail(block).ifPresent(failMethodInvocation -> { - if (isJunitJupiter || failMethodInvocation.methodSymbol().signature().contains("org.assertj")) { - reportIssue(failMethodInvocation, message); + + @Nullable String replacement = ""; + Optional failArgument = failMethodInvocation.arguments().; + + if (isJunit56) { + if (isTryBlock) { + + } else { + + } + } else if (failMethodInvocation.methodSymbol().signature().contains("org.assertj")) { + if (isTryBlock) { + + } else { + + } + } + + if (replacement == null) { + return; } + + var quickfix = JavaQuickFix.newQuickFix(message).addTextEdit(JavaTextEdit.replaceTree(tryStatement, replacement)).build(); + + QuickFixHelper + .newIssue(AssertThrowsInsteadOfTryCatchFailCheck.this.context) + .forRule(AssertThrowsInsteadOfTryCatchFailCheck.this) + .withMessage(message) + .withQuickFix(() -> quickfix) + .onTree(failMethodInvocation); } ); } From 19300f941624607741771b176c5096ba38f05256 Mon Sep 17 00:00:00 2001 From: rombirli Date: Wed, 3 Jun 2026 11:29:36 +0200 Subject: [PATCH 02/27] quickfix skeleton 2 --- .../tests/AssertThrowsInsteadOfTryCatchFailCheckSample.java | 6 +++++- .../java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java | 3 ++- 2 files changed, 7 insertions(+), 2 deletions(-) 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..fbe16f99163 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,7 +9,11 @@ public class AssertThrowsInsteadOfTryCatchFailCheckSample { void tests() { try { raise(); - fail(); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} + fail( + "hello", + // this kind of exception + new RuntimeException("hello") + ); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} // ^^^^^^ } 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 ec9f0b48322..7a35025f91d 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 @@ -68,7 +68,8 @@ private void checkBlock(BlockTree block, TryStatementTree tryStatement, boolean UnitTestUtils.findFail(block).ifPresent(failMethodInvocation -> { @Nullable String replacement = ""; - Optional failArgument = failMethodInvocation.arguments().; + // Optional failArgument = failMethodInvocation.arguments(); + // TODO : use QuickFixHelper.contentForTree(failMethodInvocation.arguments(), AssertThrowsInsteadOfTryCatchFailCheck.this.context) if (isJunit56) { if (isTryBlock) { From 23fb64a6b4009e00b2e73dce53ca37905f89a4c2 Mon Sep 17 00:00:00 2001 From: rombirli Date: Wed, 3 Jun 2026 15:18:27 +0200 Subject: [PATCH 03/27] QuickFix skleton part3 --- ...ssertThrowsInsteadOfTryCatchFailCheck.java | 67 ++++++++++++------- .../checks/InterruptedExceptionCheck.java | 16 +---- .../java/checks/helpers/TryCatchUtils.java | 38 +++++++++++ 3 files changed, 82 insertions(+), 39 deletions(-) create mode 100644 java-checks/src/main/java/org/sonar/java/checks/helpers/TryCatchUtils.java 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 7a35025f91d..87672e01aa2 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 @@ -22,6 +22,7 @@ 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; @@ -32,6 +33,8 @@ import java.util.List; import java.util.Optional; +import static org.sonar.java.checks.helpers.TryCatchUtils.getCaughtTypes; + @Rule(key = "S8714") public class AssertThrowsInsteadOfTryCatchFailCheck extends IssuableSubscriptionVisitor { @@ -57,48 +60,60 @@ public TryStatementsVisitor(boolean isJunit56) { @Override public void visitTryStatement(TryStatementTree tree) { - checkBlock(tree.block(), tree, true, "Use assertThrows() instead of try/catch and fail() in the try block."); + checkBlock(tree.block(), tree, List.of(), "Use assertThrows() instead of try/catch and fail() in the try block."); tree.catches().forEach(c -> - checkBlock(c.block(), tree, false, "Use assertDoesNotThrow() instead of try/catch and fail() in the catch block.") + checkBlock(c.block(), tree, getCaughtTypes(c), "Use assertDoesNotThrow() instead of try/catch and fail() in the catch block.") ); super.visitTryStatement(tree); } - private void checkBlock(BlockTree block, TryStatementTree tryStatement, boolean isTryBlock, String message) { + private void checkBlock(BlockTree block, TryStatementTree tryStatement, List caughtTypes, String message) { UnitTestUtils.findFail(block).ifPresent(failMethodInvocation -> { - @Nullable String replacement = ""; - // Optional failArgument = failMethodInvocation.arguments(); - // TODO : use QuickFixHelper.contentForTree(failMethodInvocation.arguments(), AssertThrowsInsteadOfTryCatchFailCheck.this.context) + var context = AssertThrowsInsteadOfTryCatchFailCheck.this.context; + List arguments = failMethodInvocation.arguments().stream() + .map(argument -> QuickFixHelper.contentForTree(argument, context)) + .toList(); + + InternalJavaIssueBuilder result = QuickFixHelper + .newIssue(AssertThrowsInsteadOfTryCatchFailCheck.this.context) + .forRule(AssertThrowsInsteadOfTryCatchFailCheck.this) + .withMessage(message) + .onTree(failMethodInvocation); if (isJunit56) { - if (isTryBlock) { + result = result.withQuickFix(() -> + JavaQuickFix.newQuickFix(message).addTextEdit(JavaTextEdit.replaceTree(tryStatement, junitReplacement(arguments, caughtTypes, message))).build() + ); + } else if (failMethodInvocation.methodSymbol().signature().contains("org.assertj")) { + result = result.withQuickFix(() -> + JavaQuickFix.newQuickFix(message).addTextEdit(JavaTextEdit.replaceTree(tryStatement, assertJReplacement(arguments, caughtTypes, message))).build() + ); + } - } else { + result.report(); + } + ); + } - } - } else if (failMethodInvocation.methodSymbol().signature().contains("org.assertj")) { - if (isTryBlock) { + private static String junitReplacement(List arguments, List caughtTypes, String message) { + boolean isTryBlock = caughtTypes.isEmpty(); + if (isTryBlock) { - } else { + } else { - } - } + } + return ""; + } - if (replacement == null) { - return; - } + private static String assertJReplacement(List arguments, List caughtTypes, String message) { + boolean isTryBlock = caughtTypes.isEmpty(); + if (isTryBlock) { - var quickfix = JavaQuickFix.newQuickFix(message).addTextEdit(JavaTextEdit.replaceTree(tryStatement, replacement)).build(); + } else { - QuickFixHelper - .newIssue(AssertThrowsInsteadOfTryCatchFailCheck.this.context) - .forRule(AssertThrowsInsteadOfTryCatchFailCheck.this) - .withMessage(message) - .withQuickFix(() -> quickfix) - .onTree(failMethodInvocation); - } - ); + } + return ""; } } } 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..fba6a7862d9 --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/helpers/TryCatchUtils.java @@ -0,0 +1,38 @@ +/* + * 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.*; + +import java.util.*; + +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().is(Tree.Kind.UNION_TYPE)) { + return ((UnionTypeTree) parameter.type()).typeAlternatives().stream() + .map(TypeTree::symbolType) + .toList(); + } + return Collections.singletonList(parameter.symbol().type()); + } +} From 821a7d0e04788359c2a691410074762497c29845 Mon Sep 17 00:00:00 2001 From: rombirli Date: Wed, 3 Jun 2026 15:51:21 +0200 Subject: [PATCH 04/27] Initial quickfixes implementation, arguments not aware --- ...ssertThrowsInsteadOfTryCatchFailCheck.java | 75 ++++++++++++++----- 1 file changed, 57 insertions(+), 18 deletions(-) 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 87672e01aa2..7837e979b29 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 @@ -29,9 +29,7 @@ import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.TryStatementTree; -import javax.annotation.Nullable; import java.util.List; -import java.util.Optional; import static org.sonar.java.checks.helpers.TryCatchUtils.getCaughtTypes; @@ -60,6 +58,7 @@ public TryStatementsVisitor(boolean isJunit56) { @Override public void visitTryStatement(TryStatementTree tree) { + var firstCaught = getCaughtTypes(tree.catches().getFirst()).getFirst(); checkBlock(tree.block(), tree, List.of(), "Use assertThrows() instead of try/catch and fail() in the try block."); tree.catches().forEach(c -> checkBlock(c.block(), tree, getCaughtTypes(c), "Use assertDoesNotThrow() instead of try/catch and fail() in the catch block.") @@ -67,11 +66,16 @@ public void visitTryStatement(TryStatementTree tree) { super.visitTryStatement(tree); } - private void checkBlock(BlockTree block, TryStatementTree tryStatement, List caughtTypes, String message) { + private void checkBlock( + BlockTree block, + TryStatementTree tryStatement, + List caughtTypesInBlock, + String message + ) { UnitTestUtils.findFail(block).ifPresent(failMethodInvocation -> { var context = AssertThrowsInsteadOfTryCatchFailCheck.this.context; - List arguments = failMethodInvocation.arguments().stream() + List failArguments = failMethodInvocation.arguments().stream() .map(argument -> QuickFixHelper.contentForTree(argument, context)) .toList(); @@ -83,11 +87,21 @@ private void checkBlock(BlockTree block, TryStatementTree tryStatement, List - JavaQuickFix.newQuickFix(message).addTextEdit(JavaTextEdit.replaceTree(tryStatement, junitReplacement(arguments, caughtTypes, message))).build() + JavaQuickFix.newQuickFix(message).addTextEdit( + JavaTextEdit.replaceTree( + tryStatement, + junitReplacement(failArguments, tryStatement, caughtTypesInBlock) + ) + ).build() ); } else if (failMethodInvocation.methodSymbol().signature().contains("org.assertj")) { result = result.withQuickFix(() -> - JavaQuickFix.newQuickFix(message).addTextEdit(JavaTextEdit.replaceTree(tryStatement, assertJReplacement(arguments, caughtTypes, message))).build() + JavaQuickFix.newQuickFix(message).addTextEdit( + JavaTextEdit.replaceTree( + tryStatement, + assertJReplacement(failArguments, tryStatement, caughtTypesInBlock) + ) + ).build() ); } @@ -96,24 +110,49 @@ private void checkBlock(BlockTree block, TryStatementTree tryStatement, List arguments, List caughtTypes, String message) { - boolean isTryBlock = caughtTypes.isEmpty(); - if (isTryBlock) { - + private String junitReplacement( + List failArguments, + TryStatementTree tryStatement, + List caughtTypesInBlock + ) { + String tryBlockString = QuickFixHelper.contentForTree(tryStatement.block(), AssertThrowsInsteadOfTryCatchFailCheck.this.context); + if (caughtTypesInBlock.isEmpty()) { + return "assertThrows(%s, () -> %s);".formatted( + typeClass(firstCaughtTypeInTry(tryStatement)), + tryBlockString + ); } else { - + return "assertDoesNotThrow(%s, %s);".formatted( + typeClass(caughtTypesInBlock.getFirst()), + tryBlockString + ); } - return ""; } - private static String assertJReplacement(List arguments, List caughtTypes, String message) { - boolean isTryBlock = caughtTypes.isEmpty(); - if (isTryBlock) { - + private String assertJReplacement( + List failArguments, + TryStatementTree tryStatement, + List caughtTypesInBlock + ) { + String tryBlockString = QuickFixHelper.contentForTree(tryStatement.block(), AssertThrowsInsteadOfTryCatchFailCheck.this.context); + if (caughtTypesInBlock.isEmpty()) { + return "assertThatThrownBy(() -> %s).isInstanceOf(%s);".formatted( + tryBlockString, + typeClass(firstCaughtTypeInTry(tryStatement)) + ); } else { - + return "assertThatThrownBy(() -> %s).doesNotThrowAnyException();".formatted( + tryBlockString + ); } - return ""; + } + + private static String typeClass(Type caughtType) { + return caughtType.name() + ".class"; + } + + private static Type firstCaughtTypeInTry(TryStatementTree tryStatement) { + return getCaughtTypes(tryStatement.catches().getFirst()).getFirst(); } } } From ae3956ff4675b45468072a495486f3699b1a22c6 Mon Sep 17 00:00:00 2001 From: rombirli Date: Wed, 3 Jun 2026 15:55:05 +0200 Subject: [PATCH 05/27] Fix for junit assertDoesntThrow (doesnt takes any throwable as arg) --- ...ssertThrowsInsteadOfTryCatchFailCheck.java | 28 +++++++++---------- 1 file changed, 13 insertions(+), 15 deletions(-) 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 7837e979b29..e36ef963310 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 @@ -58,10 +58,9 @@ public TryStatementsVisitor(boolean isJunit56) { @Override public void visitTryStatement(TryStatementTree tree) { - var firstCaught = getCaughtTypes(tree.catches().getFirst()).getFirst(); - checkBlock(tree.block(), tree, List.of(), "Use assertThrows() instead of try/catch and fail() in the try block."); + checkBlock(tree.block(), tree, true, "Use assertThrows() instead of try/catch and fail() in the try block."); tree.catches().forEach(c -> - checkBlock(c.block(), tree, getCaughtTypes(c), "Use assertDoesNotThrow() instead of try/catch and fail() in the catch block.") + checkBlock(c.block(), tree, false, "Use assertDoesNotThrow() instead of try/catch and fail() in the catch block.") ); super.visitTryStatement(tree); } @@ -69,8 +68,8 @@ public void visitTryStatement(TryStatementTree tree) { private void checkBlock( BlockTree block, TryStatementTree tryStatement, - List caughtTypesInBlock, - String message + boolean isTryBlock, + String issueMessage ) { UnitTestUtils.findFail(block).ifPresent(failMethodInvocation -> { @@ -82,24 +81,24 @@ private void checkBlock( InternalJavaIssueBuilder result = QuickFixHelper .newIssue(AssertThrowsInsteadOfTryCatchFailCheck.this.context) .forRule(AssertThrowsInsteadOfTryCatchFailCheck.this) - .withMessage(message) + .withMessage(issueMessage) .onTree(failMethodInvocation); if (isJunit56) { result = result.withQuickFix(() -> - JavaQuickFix.newQuickFix(message).addTextEdit( + JavaQuickFix.newQuickFix(issueMessage).addTextEdit( JavaTextEdit.replaceTree( tryStatement, - junitReplacement(failArguments, tryStatement, caughtTypesInBlock) + junitReplacement(failArguments, tryStatement, isTryBlock) ) ).build() ); } else if (failMethodInvocation.methodSymbol().signature().contains("org.assertj")) { result = result.withQuickFix(() -> - JavaQuickFix.newQuickFix(message).addTextEdit( + JavaQuickFix.newQuickFix(issueMessage).addTextEdit( JavaTextEdit.replaceTree( tryStatement, - assertJReplacement(failArguments, tryStatement, caughtTypesInBlock) + assertJReplacement(failArguments, tryStatement, isTryBlock) ) ).build() ); @@ -113,17 +112,16 @@ private void checkBlock( private String junitReplacement( List failArguments, TryStatementTree tryStatement, - List caughtTypesInBlock + boolean isTryBlock ) { String tryBlockString = QuickFixHelper.contentForTree(tryStatement.block(), AssertThrowsInsteadOfTryCatchFailCheck.this.context); - if (caughtTypesInBlock.isEmpty()) { + if (isTryBlock) { return "assertThrows(%s, () -> %s);".formatted( typeClass(firstCaughtTypeInTry(tryStatement)), tryBlockString ); } else { return "assertDoesNotThrow(%s, %s);".formatted( - typeClass(caughtTypesInBlock.getFirst()), tryBlockString ); } @@ -132,10 +130,10 @@ private String junitReplacement( private String assertJReplacement( List failArguments, TryStatementTree tryStatement, - List caughtTypesInBlock + boolean isTryBlock ) { String tryBlockString = QuickFixHelper.contentForTree(tryStatement.block(), AssertThrowsInsteadOfTryCatchFailCheck.this.context); - if (caughtTypesInBlock.isEmpty()) { + if (isTryBlock) { return "assertThatThrownBy(() -> %s).isInstanceOf(%s);".formatted( tryBlockString, typeClass(firstCaughtTypeInTry(tryStatement)) From b4a432be0ee6484e6c84b188fbfa7298b3ae35df Mon Sep 17 00:00:00 2001 From: rombirli Date: Wed, 3 Jun 2026 16:17:38 +0200 Subject: [PATCH 06/27] Junit arguments aware --- ...ssertThrowsInsteadOfTryCatchFailCheck.java | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) 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 e36ef963310..933fb19780a 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 @@ -23,11 +23,7 @@ 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.*; import java.util.List; @@ -73,19 +69,16 @@ private void checkBlock( ) { UnitTestUtils.findFail(block).ifPresent(failMethodInvocation -> { - var context = AssertThrowsInsteadOfTryCatchFailCheck.this.context; - List failArguments = failMethodInvocation.arguments().stream() - .map(argument -> QuickFixHelper.contentForTree(argument, context)) - .toList(); + Arguments failArguments = failMethodInvocation.arguments(); - InternalJavaIssueBuilder result = QuickFixHelper + InternalJavaIssueBuilder issueBuilder = QuickFixHelper .newIssue(AssertThrowsInsteadOfTryCatchFailCheck.this.context) .forRule(AssertThrowsInsteadOfTryCatchFailCheck.this) .withMessage(issueMessage) .onTree(failMethodInvocation); if (isJunit56) { - result = result.withQuickFix(() -> + issueBuilder = issueBuilder.withQuickFix(() -> JavaQuickFix.newQuickFix(issueMessage).addTextEdit( JavaTextEdit.replaceTree( tryStatement, @@ -94,7 +87,7 @@ private void checkBlock( ).build() ); } else if (failMethodInvocation.methodSymbol().signature().contains("org.assertj")) { - result = result.withQuickFix(() -> + issueBuilder = issueBuilder.withQuickFix(() -> JavaQuickFix.newQuickFix(issueMessage).addTextEdit( JavaTextEdit.replaceTree( tryStatement, @@ -104,31 +97,39 @@ private void checkBlock( ); } - result.report(); + issueBuilder.report(); } ); } private String junitReplacement( - List failArguments, + Arguments failArguments, TryStatementTree tryStatement, boolean isTryBlock ) { String tryBlockString = QuickFixHelper.contentForTree(tryStatement.block(), AssertThrowsInsteadOfTryCatchFailCheck.this.context); + String argumentsSuffix = failArguments.stream().findFirst().filter(argument -> + argument.symbolType().is("") || argument.symbolType().is("") + ).map(argument -> + ", %s".formatted(QuickFixHelper.contentForTree(argument, context)) + ).orElse(""); + if (isTryBlock) { - return "assertThrows(%s, () -> %s);".formatted( + return "assertThrows(%s, () -> %s%s);".formatted( typeClass(firstCaughtTypeInTry(tryStatement)), - tryBlockString + tryBlockString, + argumentsSuffix ); } else { - return "assertDoesNotThrow(%s, %s);".formatted( - tryBlockString + return "assertDoesNotThrow(%s%s);".formatted( + tryBlockString, + argumentsSuffix ); } } private String assertJReplacement( - List failArguments, + Arguments failArguments, TryStatementTree tryStatement, boolean isTryBlock ) { From 5a5d08e0f74216cf4f469139ee4818581071dc84 Mon Sep 17 00:00:00 2001 From: rombirli Date: Wed, 3 Jun 2026 16:32:28 +0200 Subject: [PATCH 07/27] Add quickfix arguments for assertJ --- ...ssertThrowsInsteadOfTryCatchFailCheck.java | 28 +++++++++++++------ 1 file changed, 19 insertions(+), 9 deletions(-) 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 933fb19780a..ba4906469ee 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 @@ -78,7 +78,7 @@ private void checkBlock( .onTree(failMethodInvocation); if (isJunit56) { - issueBuilder = issueBuilder.withQuickFix(() -> + issueBuilder.withQuickFix(() -> JavaQuickFix.newQuickFix(issueMessage).addTextEdit( JavaTextEdit.replaceTree( tryStatement, @@ -87,7 +87,7 @@ private void checkBlock( ).build() ); } else if (failMethodInvocation.methodSymbol().signature().contains("org.assertj")) { - issueBuilder = issueBuilder.withQuickFix(() -> + issueBuilder.withQuickFix(() -> JavaQuickFix.newQuickFix(issueMessage).addTextEdit( JavaTextEdit.replaceTree( tryStatement, @@ -107,11 +107,11 @@ private String junitReplacement( TryStatementTree tryStatement, boolean isTryBlock ) { - String tryBlockString = QuickFixHelper.contentForTree(tryStatement.block(), AssertThrowsInsteadOfTryCatchFailCheck.this.context); + String tryBlockString = contentFor(tryStatement.block()); String argumentsSuffix = failArguments.stream().findFirst().filter(argument -> argument.symbolType().is("") || argument.symbolType().is("") ).map(argument -> - ", %s".formatted(QuickFixHelper.contentForTree(argument, context)) + ", %s".formatted(contentFor(argument)) ).orElse(""); if (isTryBlock) { @@ -121,7 +121,7 @@ private String junitReplacement( argumentsSuffix ); } else { - return "assertDoesNotThrow(%s%s);".formatted( + return "assertDoesNotThrow(() -> %s%s);".formatted( tryBlockString, argumentsSuffix ); @@ -133,19 +133,28 @@ private String assertJReplacement( TryStatementTree tryStatement, boolean isTryBlock ) { - String tryBlockString = QuickFixHelper.contentForTree(tryStatement.block(), AssertThrowsInsteadOfTryCatchFailCheck.this.context); + // in assertJ the failure message is mandatory + var failureMessage = contentFor(failArguments.getFirst()); + String tryBlockString = contentFor(tryStatement.block()); + if (isTryBlock) { - return "assertThatThrownBy(() -> %s).isInstanceOf(%s);".formatted( + return "assertThatCode(() -> %s).withFailMessage(%s).isInstanceOf(%s);".formatted( tryBlockString, + failureMessage, typeClass(firstCaughtTypeInTry(tryStatement)) ); } else { - return "assertThatThrownBy(() -> %s).doesNotThrowAnyException();".formatted( - tryBlockString + return "assertThatCode(() -> %s).withFailMessage(%s).doesNotThrowAnyException();".formatted( + tryBlockString, + failureMessage ); } } + private String contentFor(Tree tree) { + return QuickFixHelper.contentForTree(tree, context); + } + private static String typeClass(Type caughtType) { return caughtType.name() + ".class"; } @@ -153,5 +162,6 @@ private static String typeClass(Type caughtType) { private static Type firstCaughtTypeInTry(TryStatementTree tryStatement) { return getCaughtTypes(tryStatement.catches().getFirst()).getFirst(); } + } } From 2e8ea2ba8a578e878f919293cdeab191e3181ef9 Mon Sep 17 00:00:00 2001 From: rombirli Date: Wed, 3 Jun 2026 16:44:02 +0200 Subject: [PATCH 08/27] Fix runtime issues (findfirst) and missing class names for String and Supplier --- .../checks/AssertThrowsInsteadOfTryCatchFailCheck.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) 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 ba4906469ee..64e883ec771 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 @@ -74,8 +74,8 @@ private void checkBlock( InternalJavaIssueBuilder issueBuilder = QuickFixHelper .newIssue(AssertThrowsInsteadOfTryCatchFailCheck.this.context) .forRule(AssertThrowsInsteadOfTryCatchFailCheck.this) - .withMessage(issueMessage) - .onTree(failMethodInvocation); + .onTree(failMethodInvocation) + .withMessage(issueMessage); if (isJunit56) { issueBuilder.withQuickFix(() -> @@ -109,7 +109,7 @@ private String junitReplacement( ) { String tryBlockString = contentFor(tryStatement.block()); String argumentsSuffix = failArguments.stream().findFirst().filter(argument -> - argument.symbolType().is("") || argument.symbolType().is("") + argument.symbolType().is("java.lang.String") // || argument.symbolType().is("java.util.function.Supplier") ).map(argument -> ", %s".formatted(contentFor(argument)) ).orElse(""); @@ -134,7 +134,7 @@ private String assertJReplacement( boolean isTryBlock ) { // in assertJ the failure message is mandatory - var failureMessage = contentFor(failArguments.getFirst()); + var failureMessage = contentFor(failArguments.get(0)); String tryBlockString = contentFor(tryStatement.block()); if (isTryBlock) { @@ -160,7 +160,7 @@ private static String typeClass(Type caughtType) { } private static Type firstCaughtTypeInTry(TryStatementTree tryStatement) { - return getCaughtTypes(tryStatement.catches().getFirst()).getFirst(); + return getCaughtTypes(tryStatement.catches().get(0)).get(0); } } From 8560cd2f2fbd38b2a761abdb740ea0a7186cfdc9 Mon Sep 17 00:00:00 2001 From: rombirli Date: Wed, 3 Jun 2026 17:20:49 +0200 Subject: [PATCH 09/27] Filter out fail method invocation from replacement --- ...hrowsInsteadOfTryCatchFailCheckSample.java | 6 +--- ...ssertThrowsInsteadOfTryCatchFailCheck.java | 31 +++++++++++++------ 2 files changed, 23 insertions(+), 14 deletions(-) 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 fbe16f99163..04a4e874ed4 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,7 @@ public class AssertThrowsInsteadOfTryCatchFailCheckSample { void tests() { try { raise(); - fail( - "hello", - // this kind of exception - new RuntimeException("hello") - ); // 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.}} // ^^^^^^ } 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 64e883ec771..ffd01b60858 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 @@ -19,6 +19,9 @@ import org.sonar.check.Rule; import org.sonar.java.checks.helpers.QuickFixHelper; import org.sonar.java.checks.helpers.UnitTestUtils; +import org.sonar.java.model.InternalSyntaxToken; +import org.sonar.java.model.statement.BlockTreeImpl; +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; @@ -41,7 +44,7 @@ public List nodesToVisit() { public void visitNode(Tree tree) { MethodTree methodTree = (MethodTree) tree; tree.accept( - new TryStatementsVisitor(UnitTestUtils.hasJUnitJupiterAnnotation(methodTree)) + new TryStatementsVisitor(UnitTestUtils.hasJUnit56TestAnnotation(methodTree)) ); } @@ -77,12 +80,22 @@ private void checkBlock( .onTree(failMethodInvocation) .withMessage(issueMessage); + // Compute try block without tne fail method invocation + var filteredTryBlock = new BlockTreeImpl( + (InternalSyntaxToken) tryStatement.block().openBraceToken(), + tryStatement.block().body().stream() + .filter(statement -> + statement instanceof ExpressionStatementTree expression && expression != failMethodInvocation + ).toList(), + (InternalSyntaxToken) tryStatement.block().closeBraceToken()); + var filteredTryBlockString = contentFor(filteredTryBlock); + if (isJunit56) { issueBuilder.withQuickFix(() -> JavaQuickFix.newQuickFix(issueMessage).addTextEdit( JavaTextEdit.replaceTree( tryStatement, - junitReplacement(failArguments, tryStatement, isTryBlock) + junitReplacement(failArguments, tryStatement, filteredTryBlockString, isTryBlock) ) ).build() ); @@ -91,7 +104,7 @@ private void checkBlock( JavaQuickFix.newQuickFix(issueMessage).addTextEdit( JavaTextEdit.replaceTree( tryStatement, - assertJReplacement(failArguments, tryStatement, isTryBlock) + assertJReplacement(failArguments, tryStatement, filteredTryBlockString, isTryBlock) ) ).build() ); @@ -105,9 +118,9 @@ private void checkBlock( private String junitReplacement( Arguments failArguments, TryStatementTree tryStatement, + String filteredTryBlockString, boolean isTryBlock ) { - String tryBlockString = contentFor(tryStatement.block()); String argumentsSuffix = failArguments.stream().findFirst().filter(argument -> argument.symbolType().is("java.lang.String") // || argument.symbolType().is("java.util.function.Supplier") ).map(argument -> @@ -117,12 +130,12 @@ private String junitReplacement( if (isTryBlock) { return "assertThrows(%s, () -> %s%s);".formatted( typeClass(firstCaughtTypeInTry(tryStatement)), - tryBlockString, + filteredTryBlockString, argumentsSuffix ); } else { return "assertDoesNotThrow(() -> %s%s);".formatted( - tryBlockString, + filteredTryBlockString, argumentsSuffix ); } @@ -131,21 +144,21 @@ private String junitReplacement( private String assertJReplacement( Arguments failArguments, TryStatementTree tryStatement, + String filteredTryBlockString, boolean isTryBlock ) { // in assertJ the failure message is mandatory var failureMessage = contentFor(failArguments.get(0)); - String tryBlockString = contentFor(tryStatement.block()); if (isTryBlock) { return "assertThatCode(() -> %s).withFailMessage(%s).isInstanceOf(%s);".formatted( - tryBlockString, + filteredTryBlockString, failureMessage, typeClass(firstCaughtTypeInTry(tryStatement)) ); } else { return "assertThatCode(() -> %s).withFailMessage(%s).doesNotThrowAnyException();".formatted( - tryBlockString, + filteredTryBlockString, failureMessage ); } From 186df6f7cf4c33e65789104d70d810b5be02aef9 Mon Sep 17 00:00:00 2001 From: rombirli Date: Thu, 4 Jun 2026 08:16:12 +0200 Subject: [PATCH 10/27] Fix quickfix fail filterign & try to test quickfix --- .../tests/AssertThrowsInsteadOfTryCatchFailCheckSample.java | 5 ++++- .../java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java | 4 ++-- 2 files changed, 6 insertions(+), 3 deletions(-) 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..7f81e34c1e0 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,10 +9,13 @@ 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]] // ^^^^^^ + // fix@qf1 {{Use assertThrows() instead of try/catch and fail() in the try block.}} + // edit@qf1 [[sl=10;sc=5;el=18;ec=6]]{{ assertThrows(Exception.class, () -> {\n raise();\n fail(); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} [[quickfixes=qf1]]\n// ^^^^^^\n // fix@qf1 {{Use assertThrows() instead of try/catch and fail() in the try block.}}\n // edit@qf1 [[sl=10;sc=5;el=18;ec=6]]{{assertThrows(Exception.class, () -> {raise();});}}\n }); }} } catch (Exception _) { // test passed + } try { 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 ffd01b60858..a0457966ff9 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 @@ -75,7 +75,7 @@ private void checkBlock( Arguments failArguments = failMethodInvocation.arguments(); InternalJavaIssueBuilder issueBuilder = QuickFixHelper - .newIssue(AssertThrowsInsteadOfTryCatchFailCheck.this.context) + .newIssue(context) .forRule(AssertThrowsInsteadOfTryCatchFailCheck.this) .onTree(failMethodInvocation) .withMessage(issueMessage); @@ -85,7 +85,7 @@ private void checkBlock( (InternalSyntaxToken) tryStatement.block().openBraceToken(), tryStatement.block().body().stream() .filter(statement -> - statement instanceof ExpressionStatementTree expression && expression != failMethodInvocation + statement instanceof ExpressionStatementTree expression && expression.expression() != failMethodInvocation ).toList(), (InternalSyntaxToken) tryStatement.block().closeBraceToken()); var filteredTryBlockString = contentFor(filteredTryBlock); From cfa4b99ee4b52ce37127271a7438b83635b6ff89 Mon Sep 17 00:00:00 2001 From: rombirli Date: Thu, 4 Jun 2026 11:01:26 +0200 Subject: [PATCH 11/27] Improve test (still failing), add comment how to generate quickfix correctly --- ...ssertThrowsInsteadOfTryCatchFailCheckSample.java | 13 ++++++++----- .../AssertThrowsInsteadOfTryCatchFailCheck.java | 6 ++++++ 2 files changed, 14 insertions(+), 5 deletions(-) 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 7f81e34c1e0..de3910dff77 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 @@ -5,19 +5,22 @@ import static org.junit.jupiter.api.Assertions.*; public class AssertThrowsInsteadOfTryCatchFailCheckSample { + + // fix@qf1 {{Use assertThrows() instead of try/catch and fail() in the try block.}} + // edit@qf1 [[sl=15;sc=4;el=18;ec=5]]{{assertThrows(Exception.class, () -> {\n raise();\n fail();\n// ^^^^^^\n });}} + @Test void tests() { + // Noncompliant@+3 {{Use assertThrows() instead of try/catch and fail() in the try block.}} [[quickfixes=qf1]] try { raise(); - fail(); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} [[quickfixes=qf1]] + fail(); // ^^^^^^ - // fix@qf1 {{Use assertThrows() instead of try/catch and fail() in the try block.}} - // edit@qf1 [[sl=10;sc=5;el=18;ec=6]]{{ assertThrows(Exception.class, () -> {\n raise();\n fail(); // Noncompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} [[quickfixes=qf1]]\n// ^^^^^^\n // fix@qf1 {{Use assertThrows() instead of try/catch and fail() in the try block.}}\n // edit@qf1 [[sl=10;sc=5;el=18;ec=6]]{{assertThrows(Exception.class, () -> {raise();});}}\n }); }} } catch (Exception _) { // test passed - + Runnable x = () -> { + System.out.println("hgello"); }; } - try { dontRaise(); } catch (Exception _) { 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 a0457966ff9..97715daf607 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 @@ -92,6 +92,12 @@ private void checkBlock( if (isJunit56) { issueBuilder.withQuickFix(() -> + /** Replace single text edit by 3 : + * 1 : Try token -> assertThrows( + * 2 : fail method invocation .-> "" + * 3 : catch -> ); + */ + JavaQuickFix.newQuickFix(issueMessage).addTextEdit( JavaTextEdit.replaceTree( tryStatement, From dc9c19f08dba59c5431c8c92e82f2c104f108bfe Mon Sep 17 00:00:00 2001 From: rombirli Date: Thu, 4 Jun 2026 12:12:44 +0200 Subject: [PATCH 12/27] Refine implementation plan for 3 steps replacement for excluding fail method invocation --- .../AssertThrowsInsteadOfTryCatchFailCheck.java | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) 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 97715daf607..d6d63cb64b8 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 @@ -93,9 +93,14 @@ private void checkBlock( if (isJunit56) { issueBuilder.withQuickFix(() -> /** Replace single text edit by 3 : - * 1 : Try token -> assertThrows( - * 2 : fail method invocation .-> "" - * 3 : catch -> ); + * JUNIT : + * 1 : Try token -> 'assertThrows({exception type}, () ->' or 'assertDoesntThrow(() ->' + * 2 : fail method invocation -> "" + * 3 : catches -> ); + * ASSERTJ : + * 1 : Try token -> assertThatCode(() -> + * 2 : fail method invocation -> "" + * 3 : catches -> ').isInstanceOf({exception type});' or ').doesNotThrowAnyException();' */ JavaQuickFix.newQuickFix(issueMessage).addTextEdit( From b34d8517b04a97bf996b2a0a7a7e7da02fc6353d Mon Sep 17 00:00:00 2001 From: rombirli Date: Thu, 4 Jun 2026 14:33:12 +0200 Subject: [PATCH 13/27] 3 steps edit implementation --- ...ssertThrowsInsteadOfTryCatchFailCheck.java | 123 ++++++++---------- 1 file changed, 52 insertions(+), 71 deletions(-) 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 d6d63cb64b8..a9b0a8076e3 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 @@ -44,15 +44,15 @@ public List nodesToVisit() { public void visitNode(Tree tree) { MethodTree methodTree = (MethodTree) tree; tree.accept( - new TryStatementsVisitor(UnitTestUtils.hasJUnit56TestAnnotation(methodTree)) + new TryStatementsVisitor(UnitTestUtils.hasJUnitJupiterAnnotation(methodTree)) ); } private final class TryStatementsVisitor extends BaseTreeVisitor { - private final boolean isJunit56; + private final boolean hasJunitJupiterTestAnnotation; - public TryStatementsVisitor(boolean isJunit56) { - this.isJunit56 = isJunit56; + public TryStatementsVisitor(boolean hasJunitJupiterTestAnnotation) { + this.hasJunitJupiterTestAnnotation = hasJunitJupiterTestAnnotation; } @Override @@ -80,56 +80,41 @@ private void checkBlock( .onTree(failMethodInvocation) .withMessage(issueMessage); - // Compute try block without tne fail method invocation - var filteredTryBlock = new BlockTreeImpl( - (InternalSyntaxToken) tryStatement.block().openBraceToken(), - tryStatement.block().body().stream() - .filter(statement -> - statement instanceof ExpressionStatementTree expression && expression.expression() != failMethodInvocation - ).toList(), - (InternalSyntaxToken) tryStatement.block().closeBraceToken()); - var filteredTryBlockString = contentFor(filteredTryBlock); - - if (isJunit56) { - issueBuilder.withQuickFix(() -> - /** Replace single text edit by 3 : - * JUNIT : - * 1 : Try token -> 'assertThrows({exception type}, () ->' or 'assertDoesntThrow(() ->' - * 2 : fail method invocation -> "" - * 3 : catches -> ); - * ASSERTJ : - * 1 : Try token -> assertThatCode(() -> - * 2 : fail method invocation -> "" - * 3 : catches -> ').isInstanceOf({exception type});' or ').doesNotThrowAnyException();' - */ - - JavaQuickFix.newQuickFix(issueMessage).addTextEdit( - JavaTextEdit.replaceTree( - tryStatement, - junitReplacement(failArguments, tryStatement, filteredTryBlockString, isTryBlock) - ) - ).build() - ); - } else if (failMethodInvocation.methodSymbol().signature().contains("org.assertj")) { + var isAssertJ = failMethodInvocation.methodSymbol().signature().contains("org.assertj"); + if (hasJunitJupiterTestAnnotation || isAssertJ) { + Replacements replacements = isAssertJ ? + assertJReplacement(failArguments, tryStatement, isTryBlock) : + junitReplacement(failArguments, tryStatement, isTryBlock); + /** Replace single text edit by 3 : + * JUNIT : + * 1 : Try token -> 'assertThrows({exception type}, () ->' or 'assertDoesntThrow(() ->' + * 2 : fail method invocation -> "" + * 3 : catches -> ); + * ASSERTJ : + * 1 : Try token -> assertThatCode(() -> + * 2 : fail method invocation -> "" + * 3 : catches -> ').isInstanceOf({exception type});' or ').doesNotThrowAnyException();' + */ issueBuilder.withQuickFix(() -> JavaQuickFix.newQuickFix(issueMessage).addTextEdit( - JavaTextEdit.replaceTree( - tryStatement, - assertJReplacement(failArguments, tryStatement, filteredTryBlockString, isTryBlock) + JavaTextEdit.replaceTree(tryStatement.tryKeyword(), replacements.replaceTryWith), + JavaTextEdit.replaceTree(failMethodInvocation, ""), + JavaTextEdit.replaceBetweenTree( + tryStatement.catches().get(0).catchKeyword(), + tryStatement.catches().get(tryStatement.catches().size() - 1).block().closeBraceToken(), + replacements.replaceCatchesWith ) ).build() ); } - issueBuilder.report(); } ); } - private String junitReplacement( + private Replacements junitReplacement( Arguments failArguments, TryStatementTree tryStatement, - String filteredTryBlockString, boolean isTryBlock ) { String argumentsSuffix = failArguments.stream().findFirst().filter(argument -> @@ -138,54 +123,50 @@ private String junitReplacement( ", %s".formatted(contentFor(argument)) ).orElse(""); - if (isTryBlock) { - return "assertThrows(%s, () -> %s%s);".formatted( - typeClass(firstCaughtTypeInTry(tryStatement)), - filteredTryBlockString, - argumentsSuffix - ); - } else { - return "assertDoesNotThrow(() -> %s%s);".formatted( - filteredTryBlockString, - argumentsSuffix + return isTryBlock ? + new Replacements( + "assertThrows(%s, () -> ".formatted(typeClass(firstCaughtTypeInTry(tryStatement))), + "%s);".formatted(argumentsSuffix) + ) : + new Replacements( + "assertDoesNotThrow(() -> ", + "%s);".formatted(argumentsSuffix) ); - } } - private String assertJReplacement( + private Replacements assertJReplacement( Arguments failArguments, TryStatementTree tryStatement, - String filteredTryBlockString, boolean isTryBlock ) { // in assertJ the failure message is mandatory var failureMessage = contentFor(failArguments.get(0)); - - if (isTryBlock) { - return "assertThatCode(() -> %s).withFailMessage(%s).isInstanceOf(%s);".formatted( - filteredTryBlockString, - failureMessage, - typeClass(firstCaughtTypeInTry(tryStatement)) - ); - } else { - return "assertThatCode(() -> %s).withFailMessage(%s).doesNotThrowAnyException();".formatted( - filteredTryBlockString, - failureMessage + 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 static String typeClass(Type caughtType) { - return caughtType.name() + ".class"; + private record Replacements(String replaceTryWith, String replaceCatchesWith) { } + } - private static Type firstCaughtTypeInTry(TryStatementTree tryStatement) { - return getCaughtTypes(tryStatement.catches().get(0)).get(0); - } + private static String typeClass(Type caughtType) { + return caughtType.name() + ".class"; } + + private static Type firstCaughtTypeInTry(TryStatementTree tryStatement) { + return getCaughtTypes(tryStatement.catches().get(0)).get(0); + } + } From e8cfbb289fd2e6152e70cd012bc986b540b33a2e Mon Sep 17 00:00:00 2001 From: rombirli Date: Fri, 5 Jun 2026 16:13:32 +0200 Subject: [PATCH 14/27] Fix tests, try to remove semicolon after fail in quickfix (dont work) --- .../AssertThrowsInsteadOfTryCatchFailCheckSample.java | 9 +-------- .../checks/AssertThrowsInsteadOfTryCatchFailCheck.java | 2 +- 2 files changed, 2 insertions(+), 9 deletions(-) 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 de3910dff77..66143d0d3ea 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 @@ -5,21 +5,14 @@ import static org.junit.jupiter.api.Assertions.*; public class AssertThrowsInsteadOfTryCatchFailCheckSample { - - // fix@qf1 {{Use assertThrows() instead of try/catch and fail() in the try block.}} - // edit@qf1 [[sl=15;sc=4;el=18;ec=5]]{{assertThrows(Exception.class, () -> {\n raise();\n fail();\n// ^^^^^^\n });}} - @Test void tests() { - // Noncompliant@+3 {{Use assertThrows() instead of try/catch and fail() in the try block.}} [[quickfixes=qf1]] try { raise(); - fail(); + fail(); // NonCompliant {{Use assertThrows() instead of try/catch and fail() in the try block.}} // ^^^^^^ } catch (Exception _) { // test passed - Runnable x = () -> { - System.out.println("hgello"); }; } try { dontRaise(); 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 a9b0a8076e3..08c56cecff7 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 @@ -98,7 +98,7 @@ private void checkBlock( issueBuilder.withQuickFix(() -> JavaQuickFix.newQuickFix(issueMessage).addTextEdit( JavaTextEdit.replaceTree(tryStatement.tryKeyword(), replacements.replaceTryWith), - JavaTextEdit.replaceTree(failMethodInvocation, ""), + JavaTextEdit.replaceTree(failMethodInvocation.parent(), ""), JavaTextEdit.replaceBetweenTree( tryStatement.catches().get(0).catchKeyword(), tryStatement.catches().get(tryStatement.catches().size() - 1).block().closeBraceToken(), From 3c122bcd9768422525f8d8c7593e341693d0bdb0 Mon Sep 17 00:00:00 2001 From: rombirli Date: Mon, 8 Jun 2026 10:03:28 +0200 Subject: [PATCH 15/27] Fix : report only on junit5 tests / assertJ fail --- ...sertThrowsInsteadOfTryCatchFailCheckSample.java | 3 ++- .../AssertThrowsInsteadOfTryCatchFailCheck.java | 14 +------------- 2 files changed, 3 insertions(+), 14 deletions(-) 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 66143d0d3ea..0f6b7b0e649 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 @@ -97,7 +97,8 @@ private void nonAnnotatedFunctionFN() { @org.junit.Test public void junit4AnnotationDontRaise() { try { - fail("expected exception"); // TN - junit5 assert in junit4 test + fail("expected exception"); // TN - junit fail in junit4 test + org.junit.Assert.fail("expected exception"); // TN - junit 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 08c56cecff7..6da95273ca4 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 @@ -19,8 +19,6 @@ import org.sonar.check.Rule; import org.sonar.java.checks.helpers.QuickFixHelper; import org.sonar.java.checks.helpers.UnitTestUtils; -import org.sonar.java.model.InternalSyntaxToken; -import org.sonar.java.model.statement.BlockTreeImpl; import org.sonar.java.reporting.InternalJavaIssueBuilder; import org.sonar.java.reporting.JavaQuickFix; import org.sonar.java.reporting.JavaTextEdit; @@ -85,16 +83,6 @@ private void checkBlock( Replacements replacements = isAssertJ ? assertJReplacement(failArguments, tryStatement, isTryBlock) : junitReplacement(failArguments, tryStatement, isTryBlock); - /** Replace single text edit by 3 : - * JUNIT : - * 1 : Try token -> 'assertThrows({exception type}, () ->' or 'assertDoesntThrow(() ->' - * 2 : fail method invocation -> "" - * 3 : catches -> ); - * ASSERTJ : - * 1 : Try token -> assertThatCode(() -> - * 2 : fail method invocation -> "" - * 3 : catches -> ').isInstanceOf({exception type});' or ').doesNotThrowAnyException();' - */ issueBuilder.withQuickFix(() -> JavaQuickFix.newQuickFix(issueMessage).addTextEdit( JavaTextEdit.replaceTree(tryStatement.tryKeyword(), replacements.replaceTryWith), @@ -106,8 +94,8 @@ private void checkBlock( ) ).build() ); + issueBuilder.report(); } - issueBuilder.report(); } ); } From d6890e46bde0ad7e75dca0dd9da4c5151fc6c460 Mon Sep 17 00:00:00 2001 From: rombirli Date: Mon, 8 Jun 2026 10:14:48 +0200 Subject: [PATCH 16/27] remove import * --- .../checks/AssertThrowsInsteadOfTryCatchFailCheck.java | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 6da95273ca4..f7f0ad2c0c2 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 @@ -24,7 +24,12 @@ 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.*; +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 java.util.List; From 66841ffece76926e2f928085d81d41c933a0054f Mon Sep 17 00:00:00 2001 From: rombirli Date: Mon, 8 Jun 2026 10:15:28 +0200 Subject: [PATCH 17/27] NonCompliant -> Noncompliant --- .../tests/AssertThrowsInsteadOfTryCatchFailCheckSample.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 0f6b7b0e649..7f9c3298187 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,7 +9,7 @@ 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.}} // ^^^^^^ } catch (Exception _) { // test passed From da01116caa311865e01aed3bb33b775d2981b3bf Mon Sep 17 00:00:00 2001 From: rombirli Date: Mon, 8 Jun 2026 10:47:37 +0200 Subject: [PATCH 18/27] Support Supplier as argument for junit fail --- .../AssertThrowsInsteadOfTryCatchFailCheckSample.java | 9 +++++++++ .../checks/AssertThrowsInsteadOfTryCatchFailCheck.java | 6 +++++- 2 files changed, 14 insertions(+), 1 deletion(-) 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 7f9c3298187..8181122ad29 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 @@ -85,6 +85,15 @@ 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 + } + assertThrows(IllegalStateException.class, AssertThrowsInsteadOfTryCatchFailCheckSample::raise); // compliant assertDoesNotThrow(AssertThrowsInsteadOfTryCatchFailCheckSample::dontRaise); // compliant nonAnnotatedFunctionFN(); 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 f7f0ad2c0c2..68360d1db3f 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 @@ -110,8 +110,12 @@ private Replacements junitReplacement( TryStatementTree tryStatement, boolean isTryBlock ) { + String argumentsSuffix = failArguments.stream().findFirst().filter(argument -> - argument.symbolType().is("java.lang.String") // || argument.symbolType().is("java.util.function.Supplier") + { + 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(""); From cbad93136180c3101b5e65f318d4487b59c39c5d Mon Sep 17 00:00:00 2001 From: rombirli Date: Mon, 8 Jun 2026 10:49:24 +0200 Subject: [PATCH 19/27] Remove import * in TryCatchUtils.java --- .../java/org/sonar/java/checks/helpers/TryCatchUtils.java | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) 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 index fba6a7862d9..1fa73a2efd6 100644 --- 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 @@ -17,7 +17,11 @@ package org.sonar.java.checks.helpers; import org.sonar.plugins.java.api.semantic.Type; -import org.sonar.plugins.java.api.tree.*; +import org.sonar.plugins.java.api.tree.CatchTree; +import org.sonar.plugins.java.api.tree.Tree; +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.*; From 5d27fcdcf86d9b3bbac6fdbf7c63f6053aa0a2ba Mon Sep 17 00:00:00 2001 From: rombirli Date: Mon, 8 Jun 2026 10:50:38 +0200 Subject: [PATCH 20/27] Remove import * in TryCatchUtils.java --- .../java/org/sonar/java/checks/helpers/TryCatchUtils.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) 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 index 1fa73a2efd6..962a4211e0c 100644 --- 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 @@ -22,8 +22,8 @@ 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.*; +import java.util.List; +import java.util.Collections; public final class TryCatchUtils { private TryCatchUtils() { From 0336d07647c9c58fb7ba922064e571be0d9d3017 Mon Sep 17 00:00:00 2001 From: rombirli Date: Mon, 8 Jun 2026 11:22:36 +0200 Subject: [PATCH 21/27] Avoid NPE crash when there is no catch --- ...hrowsInsteadOfTryCatchFailCheckSample.java | 6 +++++ ...ssertThrowsInsteadOfTryCatchFailCheck.java | 22 +++++++++++++++---- 2 files changed, 24 insertions(+), 4 deletions(-) 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 8181122ad29..634e858dc4f 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 @@ -94,6 +94,12 @@ void tests() { // 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(); 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 68360d1db3f..260710056a8 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 @@ -18,6 +18,7 @@ import org.sonar.check.Rule; import org.sonar.java.checks.helpers.QuickFixHelper; +import org.sonar.java.checks.helpers.TryCatchUtils; import org.sonar.java.checks.helpers.UnitTestUtils; import org.sonar.java.reporting.InternalJavaIssueBuilder; import org.sonar.java.reporting.JavaQuickFix; @@ -31,7 +32,9 @@ 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 java.util.Optional; import static org.sonar.java.checks.helpers.TryCatchUtils.getCaughtTypes; @@ -88,13 +91,19 @@ private void checkBlock( 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( - tryStatement.catches().get(0).catchKeyword(), - tryStatement.catches().get(tryStatement.catches().size() - 1).block().closeBraceToken(), + firstCatchFinallyToken, + lastCatchFinallyToken, replacements.replaceCatchesWith ) ).build() @@ -158,12 +167,17 @@ private record Replacements(String replaceTryWith, String replaceCatchesWith) { } - private static String typeClass(Type caughtType) { + private static String typeClass(@Nullable Type caughtType) { + if (caughtType == null) return "Throwable"; return caughtType.name() + ".class"; } + @Nullable private static Type firstCaughtTypeInTry(TryStatementTree tryStatement) { - return getCaughtTypes(tryStatement.catches().get(0)).get(0); + if (tryStatement.catches().size() > 0) { + return getCaughtTypes(tryStatement.catches().get(0)).get(0); + } + return null; } } From 8197a53c30660db64497b07b28674128e0507eef Mon Sep 17 00:00:00 2001 From: rombirli Date: Mon, 8 Jun 2026 16:20:19 +0200 Subject: [PATCH 22/27] Clarify intentionality of sample changes --- .../AssertThrowsInsteadOfTryCatchFailCheckSample.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) 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 634e858dc4f..f26d0aa5910 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 @@ -112,8 +112,12 @@ private void nonAnnotatedFunctionFN() { @org.junit.Test public void junit4AnnotationDontRaise() { try { - fail("expected exception"); // TN - junit fail in junit4 test - org.junit.Assert.fail("expected exception"); // TN - junit fail 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 } From 5bc8f38cc8d3675228962427618c41806aba39ec Mon Sep 17 00:00:00 2001 From: rombirli Date: Mon, 8 Jun 2026 16:20:56 +0200 Subject: [PATCH 23/27] Fix autoscan --- its/autoscan/src/test/resources/autoscan/diffs/diff_S8714.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 } From daa7e91cbc28304be7797d7e145b9e0dde3199e9 Mon Sep 17 00:00:00 2001 From: rombirli Date: Mon, 8 Jun 2026 16:44:09 +0200 Subject: [PATCH 24/27] FIX (Address comments, refactor, ...) TryCatchUtils and unit test it --- .../java/checks/helpers/TryCatchUtils.java | 9 ++-- .../checks/helpers/TryCatchUtilsTest.java | 45 +++++++++++++++++++ 2 files changed, 49 insertions(+), 5 deletions(-) create mode 100644 java-checks/src/test/java/org/sonar/java/checks/helpers/TryCatchUtilsTest.java 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 index 962a4211e0c..7f7d79c3f57 100644 --- 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 @@ -18,12 +18,11 @@ import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.CatchTree; -import org.sonar.plugins.java.api.tree.Tree; 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; -import java.util.Collections; public final class TryCatchUtils { private TryCatchUtils() { @@ -32,11 +31,11 @@ private TryCatchUtils() { public static List getCaughtTypes(CatchTree tree) { VariableTree parameter = tree.parameter(); - if (parameter.type().is(Tree.Kind.UNION_TYPE)) { - return ((UnionTypeTree) parameter.type()).typeAlternatives().stream() + if (parameter.type() instanceof UnionTypeTree unionTypeTree) { + return unionTypeTree.typeAlternatives().stream() .map(TypeTree::symbolType) .toList(); } - return Collections.singletonList(parameter.symbol().type()); + 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..29547874779 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/helpers/TryCatchUtilsTest.java @@ -0,0 +1,45 @@ +/* + * 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)); + System.out.println(compilationUnitTree); + ClassTree classTree = (ClassTree) compilationUnitTree.types().get(0); + MethodTree methodTree = (MethodTree) classTree.members().get(0); + return (TryStatementTree) methodTree.block().body().get(0); + } +} + From 25ee664870b809cb69ddf8f7f84ba57ad7b2762c Mon Sep 17 00:00:00 2001 From: rombirli Date: Mon, 8 Jun 2026 16:52:39 +0200 Subject: [PATCH 25/27] Address comment : "The fourth argument is a function of the third. Let's remove it." --- .../AssertThrowsInsteadOfTryCatchFailCheck.java | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) 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 260710056a8..60324ac479a 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 @@ -18,7 +18,6 @@ import org.sonar.check.Rule; import org.sonar.java.checks.helpers.QuickFixHelper; -import org.sonar.java.checks.helpers.TryCatchUtils; import org.sonar.java.checks.helpers.UnitTestUtils; import org.sonar.java.reporting.InternalJavaIssueBuilder; import org.sonar.java.reporting.JavaQuickFix; @@ -34,7 +33,6 @@ import javax.annotation.Nullable; import java.util.List; -import java.util.Optional; import static org.sonar.java.checks.helpers.TryCatchUtils.getCaughtTypes; @@ -63,9 +61,9 @@ public TryStatementsVisitor(boolean hasJunitJupiterTestAnnotation) { @Override public void visitTryStatement(TryStatementTree tree) { - checkBlock(tree.block(), tree, true, "Use assertThrows() instead of try/catch and fail() in the try block."); + checkBlock(tree.block(), tree, true); tree.catches().forEach(c -> - checkBlock(c.block(), tree, false, "Use assertDoesNotThrow() instead of try/catch and fail() in the catch block.") + checkBlock(c.block(), tree, false) ); super.visitTryStatement(tree); } @@ -73,10 +71,12 @@ public void visitTryStatement(TryStatementTree tree) { private void checkBlock( BlockTree block, TryStatementTree tryStatement, - boolean isTryBlock, - String issueMessage + boolean isTryBlock ) { UnitTestUtils.findFail(block).ifPresent(failMethodInvocation -> { + 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(); From d8a0c38b381c22b1d4a8bfab8379c094c6e1fc5a Mon Sep 17 00:00:00 2001 From: rombirli Date: Mon, 8 Jun 2026 16:54:05 +0200 Subject: [PATCH 26/27] Fix QG : use !isEmpty() instead of size() > 0 --- .../java/checks/AssertThrowsInsteadOfTryCatchFailCheck.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 60324ac479a..82470e4ab0e 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 @@ -174,7 +174,7 @@ private static String typeClass(@Nullable Type caughtType) { @Nullable private static Type firstCaughtTypeInTry(TryStatementTree tryStatement) { - if (tryStatement.catches().size() > 0) { + if (!tryStatement.catches().isEmpty()) { return getCaughtTypes(tryStatement.catches().get(0)).get(0); } return null; From 1affe0f7dae352903c8cac0a3e44f94edec05043 Mon Sep 17 00:00:00 2001 From: rombirli Date: Mon, 8 Jun 2026 16:55:41 +0200 Subject: [PATCH 27/27] Remove Leftover debug System.out.println in new test --- .../java/org/sonar/java/checks/helpers/TryCatchUtilsTest.java | 1 - 1 file changed, 1 deletion(-) 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 index 29547874779..12c542bb99e 100644 --- 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 @@ -36,7 +36,6 @@ void testGetCaughtTypes() { private TryStatementTree parseTry(String code) { CompilationUnitTree compilationUnitTree = JParserTestUtils.parse("void main() { %s }".formatted(code)); - System.out.println(compilationUnitTree); ClassTree classTree = (ClassTree) compilationUnitTree.types().get(0); MethodTree methodTree = (MethodTree) classTree.members().get(0); return (TryStatementTree) methodTree.block().body().get(0);