-
Notifications
You must be signed in to change notification settings - Fork 727
SONARJAVA-6454 : Rule S8714 - implement quickfix #5663
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
211440a
19300f9
23fb64a
821a7d0
ae3956f
b4a432b
5a5d08e
2e8ea2b
8560cd2
186df6f
cfa4b99
dc9c19f
b34d851
e8cfbb2
3c122bc
d6890e4
66841ff
da01116
cbad931
5d27fcd
0336d07
8197a53
5bc8f38
daa7e91
25ee664
d8a0c38
1affe0f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,6 @@ | ||
| { | ||
| "ruleKey": "S8714", | ||
| "hasTruePositives": false, | ||
| "falseNegatives": 42, | ||
| "falseNegatives": 44, | ||
| "falsePositives": 0 | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,16 +17,25 @@ | |
| package org.sonar.java.checks; | ||
|
|
||
| import org.sonar.check.Rule; | ||
| import org.sonar.java.checks.helpers.QuickFixHelper; | ||
| import org.sonar.java.checks.helpers.UnitTestUtils; | ||
| import org.sonar.java.reporting.InternalJavaIssueBuilder; | ||
| import org.sonar.java.reporting.JavaQuickFix; | ||
| import org.sonar.java.reporting.JavaTextEdit; | ||
| import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; | ||
| import org.sonar.plugins.java.api.semantic.Type; | ||
| import org.sonar.plugins.java.api.tree.BaseTreeVisitor; | ||
| import org.sonar.plugins.java.api.tree.BlockTree; | ||
| import org.sonar.plugins.java.api.tree.MethodTree; | ||
| import org.sonar.plugins.java.api.tree.Tree; | ||
| import org.sonar.plugins.java.api.tree.TryStatementTree; | ||
| import org.sonar.plugins.java.api.tree.Arguments; | ||
|
|
||
| import javax.annotation.Nullable; | ||
| import java.util.List; | ||
|
|
||
| import static org.sonar.java.checks.helpers.TryCatchUtils.getCaughtTypes; | ||
|
|
||
| @Rule(key = "S8714") | ||
| public class AssertThrowsInsteadOfTryCatchFailCheck extends IssuableSubscriptionVisitor { | ||
|
|
||
|
|
@@ -44,26 +53,131 @@ public void visitNode(Tree tree) { | |
| } | ||
|
|
||
| private final class TryStatementsVisitor extends BaseTreeVisitor { | ||
| private final boolean isJunitJupiter; | ||
| private final boolean hasJunitJupiterTestAnnotation; | ||
|
|
||
| public TryStatementsVisitor(boolean isJunitJupiter) { | ||
| this.isJunitJupiter = isJunitJupiter; | ||
| public TryStatementsVisitor(boolean hasJunitJupiterTestAnnotation) { | ||
| this.hasJunitJupiterTestAnnotation = hasJunitJupiterTestAnnotation; | ||
| } | ||
|
|
||
| @Override | ||
| public void visitTryStatement(TryStatementTree tree) { | ||
| checkBlock(tree.block(), "Use assertThrows() instead of try/catch and fail() in the try block."); | ||
| tree.catches().forEach(c -> checkBlock(c.block(), "Use assertDoesNotThrow() instead of try/catch and fail() in the catch block.")); | ||
| checkBlock(tree.block(), tree, true); | ||
| tree.catches().forEach(c -> | ||
| checkBlock(c.block(), tree, false) | ||
| ); | ||
| super.visitTryStatement(tree); | ||
| } | ||
|
|
||
| private void checkBlock(BlockTree block, String message) { | ||
| private void checkBlock( | ||
| BlockTree block, | ||
| TryStatementTree tryStatement, | ||
| boolean isTryBlock | ||
| ) { | ||
| UnitTestUtils.findFail(block).ifPresent(failMethodInvocation -> { | ||
| if (isJunitJupiter || failMethodInvocation.methodSymbol().signature().contains("org.assertj")) { | ||
| reportIssue(failMethodInvocation, message); | ||
| String issueMessage = isTryBlock ? | ||
| "Use assertThrows() instead of try/catch and fail() in the try block." : | ||
| "Use assertDoesNotThrow() instead of try/catch and fail() in the catch block."; | ||
|
|
||
| Arguments failArguments = failMethodInvocation.arguments(); | ||
|
|
||
| InternalJavaIssueBuilder issueBuilder = QuickFixHelper | ||
| .newIssue(context) | ||
| .forRule(AssertThrowsInsteadOfTryCatchFailCheck.this) | ||
| .onTree(failMethodInvocation) | ||
| .withMessage(issueMessage); | ||
|
|
||
| var isAssertJ = failMethodInvocation.methodSymbol().signature().contains("org.assertj"); | ||
| if (hasJunitJupiterTestAnnotation || isAssertJ) { | ||
| Replacements replacements = isAssertJ ? | ||
| assertJReplacement(failArguments, tryStatement, isTryBlock) : | ||
| junitReplacement(failArguments, tryStatement, isTryBlock); | ||
| var firstCatchFinallyToken = tryStatement.catches().isEmpty() ? | ||
| tryStatement.finallyKeyword().firstToken() : | ||
| tryStatement.catches().get(0).firstToken(); | ||
| var lastCatchFinallyToken = tryStatement.finallyBlock() != null ? | ||
| tryStatement.finallyBlock().lastToken() : | ||
| tryStatement.catches().get(tryStatement.catches().size() - 1).block().lastToken(); | ||
| issueBuilder.withQuickFix(() -> | ||
| JavaQuickFix.newQuickFix(issueMessage).addTextEdit( | ||
| JavaTextEdit.replaceTree(tryStatement.tryKeyword(), replacements.replaceTryWith), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I commented out code here and the unit tests still passed. Is this at all executed in tests?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes : currently unit testing quickfixes is very hard to setup because of improper logging, and those unit tests don't make much sense. They only assert that the list of edits matches the expected list of edits but not that the fixed version of the code matches the expected fixed code. Let me know if you find it strictly necessary to test this part, i can cover it (and make it fail if you comment it out). |
||
| JavaTextEdit.replaceTree(failMethodInvocation.parent(), ""), | ||
| JavaTextEdit.replaceBetweenTree( | ||
| firstCatchFinallyToken, | ||
| lastCatchFinallyToken, | ||
| replacements.replaceCatchesWith | ||
| ) | ||
| ).build() | ||
| ); | ||
|
gitar-bot[bot] marked this conversation as resolved.
|
||
| issueBuilder.report(); | ||
| } | ||
| } | ||
| ); | ||
| } | ||
|
|
||
| private Replacements junitReplacement( | ||
| Arguments failArguments, | ||
| TryStatementTree tryStatement, | ||
| boolean isTryBlock | ||
| ) { | ||
|
|
||
| String argumentsSuffix = failArguments.stream().findFirst().filter(argument -> | ||
| { | ||
| Type symbolType = argument.symbolType(); | ||
| return !symbolType.isUnknown() && (symbolType.is("java.lang.String") || symbolType.isSubtypeOf("java.util.function.Supplier<java.lang.String>")); | ||
| } | ||
| ).map(argument -> | ||
| ", %s".formatted(contentFor(argument)) | ||
| ).orElse(""); | ||
|
|
||
| return isTryBlock ? | ||
| new Replacements( | ||
| "assertThrows(%s, () -> ".formatted(typeClass(firstCaughtTypeInTry(tryStatement))), | ||
| "%s);".formatted(argumentsSuffix) | ||
| ) : | ||
| new Replacements( | ||
| "assertDoesNotThrow(() -> ", | ||
| "%s);".formatted(argumentsSuffix) | ||
| ); | ||
| } | ||
|
|
||
| private Replacements assertJReplacement( | ||
| Arguments failArguments, | ||
| TryStatementTree tryStatement, | ||
| boolean isTryBlock | ||
| ) { | ||
| // in assertJ the failure message is mandatory | ||
| var failureMessage = contentFor(failArguments.get(0)); | ||
| return isTryBlock ? | ||
| new Replacements( | ||
| "assertThatCode(() -> ", | ||
| ").withFailMessage(%s).isInstanceOf(%s);".formatted(failureMessage, typeClass(firstCaughtTypeInTry(tryStatement))) | ||
| ) : | ||
| new Replacements( | ||
| "assertThatCode(() -> ", | ||
| ").withFailMessage(%s).doesNotThrowAnyException();".formatted(failureMessage) | ||
|
gitar-bot[bot] marked this conversation as resolved.
|
||
| ); | ||
| } | ||
|
|
||
| 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; | ||
| } | ||
|
|
||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,41 @@ | ||
| /* | ||
| * SonarQube Java | ||
| * Copyright (C) SonarSource Sàrl | ||
| * mailto:info AT sonarsource DOT com | ||
| * | ||
| * You can redistribute and/or modify this program under the terms of | ||
| * the Sonar Source-Available License Version 1, as published by SonarSource Sàrl. | ||
| * | ||
| * This program is distributed in the hope that it will be useful, | ||
| * but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. | ||
| * See the Sonar Source-Available License for more details. | ||
| * | ||
| * You should have received a copy of the Sonar Source-Available License | ||
| * along with this program; if not, see https://sonarsource.com/license/ssal/ | ||
| */ | ||
| package org.sonar.java.checks.helpers; | ||
|
|
||
| import org.sonar.plugins.java.api.semantic.Type; | ||
| import org.sonar.plugins.java.api.tree.CatchTree; | ||
| import org.sonar.plugins.java.api.tree.UnionTypeTree; | ||
| import org.sonar.plugins.java.api.tree.TypeTree; | ||
| import org.sonar.plugins.java.api.tree.VariableTree; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| public final class TryCatchUtils { | ||
| private TryCatchUtils() { | ||
| /* This utility class should not be instantiated */ | ||
| } | ||
|
|
||
| public static List<Type> getCaughtTypes(CatchTree tree) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This method should have a test.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. But the test is actually more complicated and contains more logic than the method itself (it tests more test code than prod code) |
||
| VariableTree parameter = tree.parameter(); | ||
| if (parameter.type() instanceof UnionTypeTree unionTypeTree) { | ||
| return unionTypeTree.typeAlternatives().stream() | ||
| .map(TypeTree::symbolType) | ||
| .toList(); | ||
| } | ||
| return List.of(parameter.symbol().type()); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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); | ||
| } | ||
| } | ||
|
|
Uh oh!
There was an error while loading. Please reload this page.