[NodeAnalyzer] Skip trailing comments and require \Throwable catch in try/catch termination check

Author: TomasVotruba

…ture/always_terminated_try_catch.php.inc …nated_try_catch_comment_in_catch.php.incrules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch.php.inc renamed to rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch_comment_in_catch.php.inc rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch.php.inc renamed to 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
         }
     }
 }

…y_catch_with_stmts_before_return.php.inc …ys_terminated_try_infinite_while.php.incrules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch_with_stmts_before_return.php.inc renamed to rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_infinite_while.php.inc rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/always_terminated_try_catch_with_stmts_before_return.php.inc renamed to 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;
         }
     }

rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_not_throwable_try_catch.php.inc

@@ -0,0 +1,21 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector\Fixture;
+
+use Exception;
+
+class SkipNotThrowableTryCatch
+{
+    public function run()
+    {
+        try {
+            return something();
+        } catch (Exception $e) {
+            return null;
+        }
+
+        echo 'reachable when an Error escapes the catch';
+    }
+}
+
+?>

rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/skip_not_throwable_try_catch_with_stmts_before_return.php.inc

@@ -0,0 +1,23 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Stmt\RemoveUnreachableStatementRector\Fixture;
+
+use Exception;
+
+class SkipNotThrowableTryCatchWithStmtsBeforeReturn
+{
+    public function run()
+    {
+        try {
+            echo 'try';
+            return something();
+        } catch (Exception $e) {
+            echo 'catch';
+            return null;
+        }
+
+        echo 'reachable when an Error escapes the catch';
+    }
+}
+
+?>

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_;
     }
 }