diff --git a/python-checks/src/main/java/org/sonar/python/checks/InvariantReturnCheck.java b/python-checks/src/main/java/org/sonar/python/checks/InvariantReturnCheck.java index 476e0b2ea..ee65063cc 100644 --- a/python-checks/src/main/java/org/sonar/python/checks/InvariantReturnCheck.java +++ b/python-checks/src/main/java/org/sonar/python/checks/InvariantReturnCheck.java @@ -80,17 +80,37 @@ public void initialize(Context context) { private static List collectLatestExecutedBlocks(ControlFlowGraph cfg) { List collectedBlocks = new ArrayList<>(); + Set reachableBlocks = reachableBlocks(cfg.start()); for (CfgBlock predecessor : cfg.end().predecessors()) { + if (!reachableBlocks.contains(predecessor)) { + continue; + } if (predecessor instanceof PythonCfgBranchingBlock pythonCfgBranchingBlock) { - collectBranchingBlock(collectedBlocks, pythonCfgBranchingBlock); - } else if (!endsWithElementKind(predecessor, Kind.RAISE_STMT)) { + collectBranchingBlock(collectedBlocks, pythonCfgBranchingBlock, reachableBlocks); + } else { collectedBlocks.add(new LatestExecutedBlock(predecessor)); } } return collectedBlocks; } - private static void collectBranchingBlock(List collectedBlocks, PythonCfgBranchingBlock branchingBlock) { + private static Set reachableBlocks(CfgBlock start) { + Set reachable = new HashSet<>(); + Deque blockToVisit = new ArrayDeque<>(); + blockToVisit.push(start); + reachable.add(start); + while (!blockToVisit.isEmpty()) { + CfgBlock block = blockToVisit.pop(); + for (CfgBlock successor : block.successors()) { + if (reachable.add(successor)) { + blockToVisit.push(successor); + } + } + } + return reachable; + } + + private static void collectBranchingBlock(List collectedBlocks, PythonCfgBranchingBlock branchingBlock, Set reachableBlocks) { Tree branchingTree = branchingBlock.branchingTree(); if (branchingTree.is(Kind.TRY_STMT)) { TryStatement tryStatement = (TryStatement) branchingTree; @@ -100,16 +120,19 @@ private static void collectBranchingBlock(List collectedBlo } else if (branchingTree.is(Kind.IF_STMT) || branchingTree instanceof Pattern) { collectedBlocks.add(new LatestExecutedBlock(branchingBlock)); } else { - collectBlocksHavingReturnBeforeExceptOrFinallyBlock(collectedBlocks, branchingBlock); + collectBlocksHavingReturnBeforeExceptOrFinallyBlock(collectedBlocks, branchingBlock, reachableBlocks); } } - private static void collectBlocksHavingReturnBeforeExceptOrFinallyBlock(List collectedBlocks, PythonCfgBranchingBlock branchingBlock) { + private static void collectBlocksHavingReturnBeforeExceptOrFinallyBlock(List collectedBlocks, PythonCfgBranchingBlock branchingBlock, Set reachableBlocks) { if (branchingBlock.branchingTree().is(Kind.EXCEPT_CLAUSE, Kind.FINALLY_CLAUSE)) { for (CfgBlock predecessor : branchingBlock.predecessors()) { + if (!reachableBlocks.contains(predecessor)) { + continue; + } if (predecessor instanceof PythonCfgBranchingBlock pythonCfgBranchingBlock) { - collectBlocksHavingReturnBeforeExceptOrFinallyBlock(collectedBlocks, pythonCfgBranchingBlock); - } else if (endsWithElementKind(predecessor, Kind.RETURN_STMT)) { + collectBlocksHavingReturnBeforeExceptOrFinallyBlock(collectedBlocks, pythonCfgBranchingBlock, reachableBlocks); + } else if (endsWithElementKind(predecessor, Kind.RETURN_STMT) || endsWithElementKind(predecessor, Kind.RAISE_STMT)) { collectedBlocks.add(new LatestExecutedBlock(predecessor)); } } diff --git a/python-checks/src/test/resources/checks/invariantReturn.py b/python-checks/src/test/resources/checks/invariantReturn.py index 3648cb887..14408417d 100644 --- a/python-checks/src/test/resources/checks/invariantReturn.py +++ b/python-checks/src/test/resources/checks/invariantReturn.py @@ -433,15 +433,56 @@ def f_raise_is_not_a_return(a, b): raise b return b -def f_function_exit_through_raise_should_be_ignored(a, b, c): # Noncompliant -# ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +def f_function_exit_through_raise_prevents_reporting(a, b, c): if a: return b -# ^^^^^^^^< if c: raise return b -# ^^^^^^^^< + +def f_early_return_with_raise_guard(already_valid, invalid, obj): + if already_valid: + return obj + if invalid: + raise ValueError() + return obj + +def f_unreachable_raise_does_not_prevent_reporting(a, b): # Noncompliant + if a: + return b + return b + raise ValueError() + +def f_raise_in_try_with_finally(early, invalid, value): + if early: + return value + try: + if invalid: + raise ValueError() + finally: + pass + return value + +def f_raise_in_try_with_except(early, invalid, value): + if early: + return value + try: + if invalid: + raise ValueError() + except TypeError: + cleanup() + return value + +def f_unreachable_raise_before_finally_does_not_prevent_reporting(a, b): # Noncompliant + try: + if a: + return b + else: + return b + raise ValueError() + finally: + pass + return b def f_same_binding_through_multiple_paths(a): # Noncompliant d = 3