From 0398dc7c7173d4e2cc37dba7fbfe072b0b5c0ea3 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Tue, 8 Sep 2026 21:28:24 +0200 Subject: [PATCH] [NodeAnalyzer] Skip trailing comments and require \Throwable catch in try/catch termination check --- ...inated_try_catch_comment_in_catch.php.inc} | 10 +++-- ...ays_terminated_try_infinite_while.php.inc} | 20 +++++----- .../skip_not_throwable_try_catch.php.inc | 21 ++++++++++ ...try_catch_with_stmts_before_return.php.inc | 23 +++++++++++ src/NodeAnalyzer/TerminatedNodeAnalyzer.php | 38 ++++++++++++++++--- 5 files changed, 93 insertions(+), 19 deletions(-) rename rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/{always_terminated_try_catch.php.inc => always_terminated_try_catch_comment_in_catch.php.inc} (70%) rename rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/{always_terminated_try_catch_with_stmts_before_return.php.inc => always_terminated_try_infinite_while.php.inc} (57%) create mode 100644 rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_not_throwable_try_catch.php.inc create mode 100644 rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_not_throwable_try_catch_with_stmts_before_return.php.inc diff --git a/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch.php.inc b/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch_comment_in_catch.php.inc similarity index 70% rename from rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch.php.inc rename to rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch_comment_in_catch.php.inc index 451646bcf04..58801064107 100644 --- a/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch.php.inc +++ b/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch_comment_in_catch.php.inc @@ -4,14 +4,15 @@ namespace Rector\Tests\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector\Fix use Exception; -class AlwaysTerminatedTryCatch +class AlwaysTerminatedTryCatchCommentInCatch { public function run() { try { return something(); - } catch (Exception $e) { + } catch (\Throwable $e) { return null; + // some comment } echo 'never executed'; @@ -26,14 +27,15 @@ namespace Rector\Tests\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector\Fix use Exception; -class AlwaysTerminatedTryCatch +class AlwaysTerminatedTryCatchCommentInCatch { public function run() { try { return something(); - } catch (Exception $e) { + } catch (\Throwable $e) { return null; + // some comment } } } diff --git a/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch_with_stmts_before_return.php.inc b/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_infinite_while.php.inc similarity index 57% rename from rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch_with_stmts_before_return.php.inc rename to rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_infinite_while.php.inc index 1b8b580b4ce..3e52e801bb2 100644 --- a/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch_with_stmts_before_return.php.inc +++ b/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_infinite_while.php.inc @@ -4,15 +4,15 @@ namespace Rector\Tests\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector\Fix use Exception; -class AlwaysTerminatedTryCatchWithStmtsBeforeReturn +class AlwaysTerminatedTryInfiniteWhile { public function run() { try { - echo 'try'; - return something(); - } catch (Exception $e) { - echo 'catch'; + while (true) { + doThings(); + } + } catch (\Throwable $e) { return null; } @@ -28,15 +28,15 @@ namespace Rector\Tests\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector\Fix use Exception; -class AlwaysTerminatedTryCatchWithStmtsBeforeReturn +class AlwaysTerminatedTryInfiniteWhile { public function run() { try { - echo 'try'; - return something(); - } catch (Exception $e) { - echo 'catch'; + while (true) { + doThings(); + } + } catch (\Throwable $e) { return null; } } diff --git a/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_not_throwable_try_catch.php.inc b/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_not_throwable_try_catch.php.inc new file mode 100644 index 00000000000..640b07b4aa9 --- /dev/null +++ b/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_not_throwable_try_catch.php.inc @@ -0,0 +1,21 @@ + diff --git a/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_not_throwable_try_catch_with_stmts_before_return.php.inc b/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_not_throwable_try_catch_with_stmts_before_return.php.inc new file mode 100644 index 00000000000..7f956fac915 --- /dev/null +++ b/rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_not_throwable_try_catch_with_stmts_before_return.php.inc @@ -0,0 +1,23 @@ + diff --git a/src/NodeAnalyzer/TerminatedNodeAnalyzer.php b/src/NodeAnalyzer/TerminatedNodeAnalyzer.php index bb9752dfbac..92380b8f494 100644 --- a/src/NodeAnalyzer/TerminatedNodeAnalyzer.php +++ b/src/NodeAnalyzer/TerminatedNodeAnalyzer.php @@ -241,9 +241,28 @@ private function isTerminatedInLastStmtsTryCatch(TryCatch $tryCatch): bool } } + // catching less than \Throwable lets an Error escape the try, + // so the try/catch is not guaranteed to end control flow here + if ($tryCatch->catches !== [] && ! $this->hasThrowableCatch($tryCatch)) { + return false; + } + return $this->isTerminatedInLastStmts($tryCatch->stmts); } + private function hasThrowableCatch(TryCatch $tryCatch): bool + { + foreach ($tryCatch->catches as $catch) { + foreach ($catch->types as $type) { + if ($type->toString() === 'Throwable') { + return true; + } + } + } + + return false; + } + private function isTerminatedInLastStmtsIf(If_ $if): bool { // Without ElseIf_[] and Else_, after If_ is possibly executable @@ -273,17 +292,26 @@ private function isTerminatedInLastStmtsIf(If_ $if): bool */ private function isTerminatedInLastStmts(array $stmts): bool { + // trailing comments are parsed as Nop stmts, skip them to reach the real last stmt + while ($stmts !== [] && end($stmts) instanceof Nop) { + array_pop($stmts); + } + if ($stmts === []) { return false; } - $lastKey = array_key_last($stmts); - $lastNode = $stmts[$lastKey]; + $stmt = end($stmts); + + if ($stmt instanceof Expression) { + return $stmt->expr instanceof Exit_ || $stmt->expr instanceof Throw_; + } - if ($lastNode instanceof Expression) { - return $lastNode->expr instanceof Exit_ || $lastNode->expr instanceof Throw_; + // an infinite loop with no break never falls through to the next stmt + if ($stmt instanceof While_ || $stmt instanceof Do_ || $stmt instanceof For_) { + return $this->isTerminatedInfiniteLoop($stmt); } - return $lastNode instanceof Return_; + return $stmt instanceof Return_; } }