Do not let a trailing comment or an infinite loop hide a terminated block

[dchaudhari7177]

Half of rectorphp/rector#9897. This is the analyzer half; the hasChanged half is in rector-symfony and is a separate PR.

The bug

TerminatedNodeAnalyzer::isTerminatedInLastStmts() decides whether a block always terminates by looking at its last statement, and accepts only a Return_ or an exit/throw Expression. Two shapes get past it:

try {
    return something();
    // nothing to do here     <- parsed as a Nop, and it is the last stmt
} catch (Exception $e) {
    return null;
}

try {
    while (true) {            <- never falls through, but is not a Return_
        if ($a) { return 'A'; }
        $a = doSomething();
    }
} catch (Exception $e) {
    return null;
}

Both blocks always terminate, so isAlwaysTerminated() should return true for the TryCatch and returns false. In #9897 that is what makes ConsoleExecuteReturnIntRector append an unreachable return Command::SUCCESS;, which PHPStan then reports as deadCode.unreachable.

The infinite-loop case is the odd one, because isAlwaysTerminated() already handles it — via isTerminatedInfiniteLoop() — but only for a loop at the top level of the enclosing block. Nested one level down, inside a try, the same loop is judged by isTerminatedInLastStmts(), which has never known about loops.

The fix

Trailing Nops are popped before the last statement is read, and a trailing loop is routed through the existing isTerminatedInfiniteLoop() — so the break/goto escape analysis that helper already does is reused rather than duplicated.

Verified

composer install needs PHP ≥ 8.4.1 for phpunit/phpunit ^13.3, and this machine has 8.3.6, so I could not run the suite — CI will have to be the judge of the three fixtures. I did not want to leave the change unproven, so I checked the analyzer logic directly instead: a script that parses each shape with nikic/php-parser 5.8 and runs a faithful copy of isTerminatedInLastStmts() before and after the change, with isTerminatedInfiniteLoop() inlined for these shapes.

try block                                                    | last stmt in try | before | after
-------------------------------------------------------------------------------------------------
try block ending in a trailing comment                       | Nop              | false  | true
try block ending in while (true)                             | While_           | false  | true
try block ending in a plain return (control)                 | Return_          | true   | true
try block that really does fall through (control)            | Expression       | false  | false
while (true) with a break, so it does fall through (control) | While_           | false  | false

The two reported shapes flip; the three controls do not move. The last row is the one worth pointing at — a while (true) carrying a break still counts as falling through, because isTerminatedInfiniteLoop() does that check, which is exactly why the fix delegates to it rather than pattern-matching on the loop.

Fixtures

Three, in rules-tests/DeadCode/Rector/Stmt/RemoveUnreachableStatementRector/Fixture/, next to the existing always_terminated_* set:

• always_terminated_try_catch_with_trailing_comment.php.inc
• always_terminated_try_catch_with_infinite_while.php.inc
• always_terminated_if_else_with_trailing_comment.php.inc

The third one covers isTerminatedInLastStmtsIf(), which reaches the same helper — the bug is in the shared leaf, not in the try/catch branch.

Deliberately not done

isTerminatedInLastStmts() still does not recurse into a nested TryCatch, If_ or Switch_ sitting as the last statement of a block, even though isAlwaysTerminated() handles all three one level up. That is the same class of gap, but widening it would change the verdict for a lot of existing code, and #9897 does not need it. Happy to follow up separately if you want it closed properly.

Note

This PR was drafted with AI assistance. I understand the change and can defend or revise it; the evidence table above is a run I did rather than a claim.

[TomasVotruba]

LGTM, thanks