From d43e827648adb499983342afa55dbf6f40333062 Mon Sep 17 00:00:00 2001 From: Romain Brenguier Date: Mon, 26 Oct 2026 12:28:04 +0100 Subject: [PATCH] SONARJAVA-7243 Improve S8989: Do not report when all checked exceptions are thrown before any write Use the control flow graph of the method to skip the issue when every point that may throw a checked exception is executed before any write operation. Co-Authored-By: Claude Opus 5.5 (1M context) --- ...eckedExceptionCheckSampleNonCompiling.java | 27 ++++ ...onalMethodCheckedExceptionCheckSample.java | 141 ++++++++++++++++++ ...nsactionalMethodCheckedExceptionCheck.java | 98 ++++++++++++ 3 files changed, 266 insertions(+) diff --git a/java-checks-test-sources/default/src/main/files/non-compiling/checks/spring/TransactionalMethodCheckedExceptionCheckSampleNonCompiling.java b/java-checks-test-sources/default/src/main/files/non-compiling/checks/spring/TransactionalMethodCheckedExceptionCheckSampleNonCompiling.java index e8b14751c9e..e25c7ece2d8 100644 --- a/java-checks-test-sources/default/src/main/files/non-compiling/checks/spring/TransactionalMethodCheckedExceptionCheckSampleNonCompiling.java +++ b/java-checks-test-sources/default/src/main/files/non-compiling/checks/spring/TransactionalMethodCheckedExceptionCheckSampleNonCompiling.java @@ -19,4 +19,31 @@ public void knownException() throws java.io.IOException { // Noncompliant [[seco public void propagationFromMethodCall() throws java.io.IOException { // Noncompliant [[secondary=18]] // ^^^^^^^^^^^^^^^^^^^^^^^^^ } + + @Transactional + public void unknownThrownTypeAfterWrite(Repository repository) throws java.io.IOException { // Noncompliant [[secondary=24]] +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + repository.save(null); + throw new UnknownException(); + } + + @Transactional + public void unknownMethodAfterWrite(Repository repository) throws java.io.IOException { // Noncompliant [[secondary=31]] +// ^^^^^^^^^^^^^^^^^^^^^^^ + repository.save(null); + unknownMethod(); + } + + @Transactional + public void checkedThrowBeforeWrite(Repository repository) throws java.io.IOException { // Compliant + if (repository == null) { + throw new java.io.IOException(); + } + repository.save(null); + } + + static class Repository { + void save(Object o) { + } + } } diff --git a/java-checks-test-sources/default/src/main/java/checks/spring/TransactionalMethodCheckedExceptionCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/spring/TransactionalMethodCheckedExceptionCheckSample.java index 0b2b5edc2ad..0b6732a51aa 100644 --- a/java-checks-test-sources/default/src/main/java/checks/spring/TransactionalMethodCheckedExceptionCheckSample.java +++ b/java-checks-test-sources/default/src/main/java/checks/spring/TransactionalMethodCheckedExceptionCheckSample.java @@ -414,4 +414,145 @@ public void directSaveAndLambda() throws IOException { // Noncompliant [[seconda Runnable r = () -> deleteEntity(null); } } + + static class Repository { + void save(Object o) { + } + + void saveChecked(Object o) throws IOException { + } + } + + static class Validator { + static void validate(Object o) throws IOException { + } + + Validator() throws SQLException { + } + } + + @Transactional + static class ThrowsBeforeWrites { + private final Repository repository = new Repository(); + + public void saveAfterThrow(Object o) throws IOException { // Compliant - all checked exceptions are thrown before any write + if (o == null) { + throw new IOException("null"); + } + repository.save(o); + } + + public void saveAfterValidation(Object o) throws IOException { // Compliant - validation throwing checked exception happens before any write + Validator.validate(o); + repository.save(o); + } + + public void saveAfterConstructor(Object o) throws SQLException { // Compliant - constructor throwing checked exception happens before any write + new Validator(); + repository.save(o); + } + + public void saveAfterThrowInLoop(java.util.List list) throws IOException { // Compliant + for (Object o : list) { + if (o == null) { + throw new IOException("null"); + } + } + list.forEach(repository::save); + list.forEach(o -> repository.save(o)); + } + + public void saveBeforeThrow(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^ + repository.save(o); + if (o == null) { + throw new IOException("null"); + } + } + + public void saveBeforeValidation(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^ + repository.save(o); + Validator.validate(o); + } + + public void saveBeforeConstructor(Object o) throws SQLException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^ + repository.save(o); + new Validator(); + } + + public void saveThrowingChecked(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^ + repository.saveChecked(o); + } + + public void saveInLoopWithThrow(java.util.List list) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^ + for (Object o : list) { + if (o == null) { + throw new IOException("null"); + } + repository.save(o); + } + } + + public void saveInLambdaBeforeThrow(java.util.List list) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^ + list.forEach(o -> repository.save(o)); + Validator.validate(list); + } + + public void saveInTryThenThrowInCatch(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^ + try { + repository.save(o); + } catch (RuntimeException e) { + throw new IOException(e); + } + } + + public void saveThenRethrowUnknownType(Object o, Exception e) throws Exception { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^ + repository.save(o); + throw e; + } + + public void saveAfterUncheckedThrow(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^ + if (o == null) { + throw new IllegalArgumentException(); + } + repository.save(o); + throw new IllegalStateException(); + } + } + + private final Repository repository = new Repository(); + + @Transactional + public void methodLevelThrowBeforeWrite(Object o) throws IOException { // Compliant - all checked exceptions are thrown before any write + if (o == null) { + throw new IOException("null"); + } + repository.save(o); + } + + @Transactional + public void methodLevelThrowWithoutWrite(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + if (o == null) { + throw new IOException("null"); + } + o.toString(); + } + + @Transactional + public void methodLevelWriteBeforeThrow(Object o) throws IOException { // Noncompliant +// ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + repository.save(o); + if (o == null) { + throw new IOException("null"); + } + } } diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java index 2af80683774..0da39d946cf 100644 --- a/java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/TransactionalMethodCheckedExceptionCheck.java @@ -16,11 +16,15 @@ */ package org.sonar.java.checks.spring; +import java.util.ArrayDeque; import java.util.ArrayList; import java.util.Collections; +import java.util.Deque; +import java.util.HashSet; import java.util.List; import java.util.Locale; import java.util.Optional; +import java.util.Set; import java.util.function.Function; import java.util.stream.Collectors; import org.sonar.check.Rule; @@ -34,6 +38,8 @@ import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; import org.sonar.plugins.java.api.JavaFileScannerContext; import org.sonar.plugins.java.api.Version; +import org.sonar.plugins.java.api.cfg.ControlFlowGraph; +import org.sonar.plugins.java.api.semantic.Symbol; import org.sonar.plugins.java.api.semantic.Type; import org.sonar.plugins.java.api.tree.AnnotationTree; import org.sonar.plugins.java.api.tree.Arguments; @@ -50,6 +56,7 @@ import org.sonar.plugins.java.api.tree.NewClassTree; import org.sonar.plugins.java.api.tree.Modifier; import org.sonar.plugins.java.api.tree.SyntaxToken; +import org.sonar.plugins.java.api.tree.ThrowStatementTree; import org.sonar.plugins.java.api.tree.Tree; import org.sonar.plugins.java.api.tree.TypeTree; @@ -113,6 +120,10 @@ public void visitNode(Tree tree) { return; } + if (allCheckedExceptionsThrownBeforeWrites(method)) { + return; + } + var issueBuilder = QuickFixHelper.newIssue(context) .forRule(this) .onTree(method.simpleName()); @@ -296,6 +307,93 @@ private static boolean looksTransactional(MethodTree method) { return visitor.foundTransactionalCall; } + /** + * Uses the control flow graph of the method to determine whether every point that may throw a checked exception + * is executed before any write operation. In that case, no write can have happened when a checked exception is + * thrown, so the rollback behavior is irrelevant. + * Returns false when the method has no body, no write operation or no point throwing a checked exception. + */ + private static boolean allCheckedExceptionsThrownBeforeWrites(MethodTree method) { + ControlFlowGraph cfg = method.cfg(); + if (cfg == null) { + return false; + } + boolean hasWrite = false; + boolean hasThrowPoint = false; + for (ControlFlowGraph.Block block : cfg.blocks()) { + List items = blockItems(block); + for (int i = 0; i < items.size(); i++) { + hasThrowPoint |= isCheckedThrowPoint(items.get(i)); + if (isWriteOperation(items.get(i))) { + hasWrite = true; + if (items.subList(i, items.size()).stream().anyMatch(TransactionalMethodCheckedExceptionCheck::isCheckedThrowPoint) + || hasReachableThrowPoint(block)) { + return false; + } + } + } + } + return hasWrite && hasThrowPoint; + } + + private static List blockItems(ControlFlowGraph.Block block) { + Tree terminator = block.terminator(); + if (terminator == null) { + return block.elements(); + } + List items = new ArrayList<>(block.elements()); + items.add(terminator); + return items; + } + + private static boolean hasReachableThrowPoint(ControlFlowGraph.Block start) { + Set visited = new HashSet<>(); + Deque toVisit = new ArrayDeque<>(); + addSuccessors(start, toVisit); + while (!toVisit.isEmpty()) { + ControlFlowGraph.Block block = toVisit.pop(); + if (visited.add(block)) { + if (blockItems(block).stream().anyMatch(TransactionalMethodCheckedExceptionCheck::isCheckedThrowPoint)) { + return true; + } + addSuccessors(block, toVisit); + } + } + return false; + } + + private static void addSuccessors(ControlFlowGraph.Block block, Deque toVisit) { + toVisit.addAll(block.successors()); + toVisit.addAll(block.exceptions()); + } + + private static boolean isWriteOperation(Tree tree) { + if (!(tree instanceof MethodInvocationTree) && !(tree instanceof NewClassTree)) { + return false; + } + var visitor = new MethodInvocationVisitor(); + tree.accept(visitor); + return visitor.foundTransactionalCall; + } + + private static boolean isCheckedThrowPoint(Tree tree) { + if (tree instanceof ThrowStatementTree throwStatement) { + Type type = throwStatement.expression().symbolType(); + return type.isUnknown() || isCheckedException(type); + } + if (tree instanceof MethodInvocationTree mit) { + return mayThrowCheckedException(mit.methodSymbol()); + } + if (tree instanceof NewClassTree newClass) { + return mayThrowCheckedException(newClass.methodSymbol()); + } + return false; + } + + private static boolean mayThrowCheckedException(Symbol.MethodSymbol symbol) { + return symbol.isUnknown() || symbol.thrownTypes().stream().anyMatch(type -> type.isUnknown() || isCheckedException(type)); + } + private static boolean hasTransactionalPrefix(String name) { String lowerName = name.toLowerCase(Locale.ROOT); return TRANSACTIONAL_PREFIXES.stream().anyMatch(prefix -> {