Skip to content

ConsoleExecuteReturnIntRector appends unreachable return 0 after a break-less while(true) loop (same TerminatedNodeAnalyzer gap as #9852) #9853

Description

@NathanTaco

Bug Report

Rector version: 2.6.2

Rule: ConsoleExecuteReturnIntRector (Symfony 4.4 set, pulled in via RectorConfig::withComposerBased(symfony: true) in a Symfony 8.1 app)

Related issue

Same root shape as #9852, but a different rule with its own (apparently separate) reachability check —
processReturn0ToMethod() calls TerminatedNodeAnalyzer::isAlwaysTerminated($classMethod, $lastStmt, $return)
before appending, so this rule does attempt a reachability guard, and it still gets it wrong for the same
input shape. Filing separately since it's a different rule/class, but the maintainers may want to fix both via
the same TerminatedNodeAnalyzer change.

Minimal reproduction

Input — a command whose only loop is while (true) { ... } with no break, where every exit path is an explicit return from inside the loop, and execute() is already typed : int:

<?php

use Symfony\Component\Console\Command\Command;

class ExampleCommand extends Command
{
    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        while (true) {
            if ($this->shouldStop()) {
                return Command::SUCCESS;
            }

            if ($this->hasFailed()) {
                return Command::FAILURE;
            }

            $this->tick();
        }
    }
}

PHPStan already accepts this as-is: the loop has no break, so the method can never fall off the end without
returning.

Actual output (--fix)

    protected function execute(InputInterface $input, OutputInterface $output): int
    {
        while (true) {
            if ($this->shouldStop()) {
                return Command::SUCCESS;
            }

            if ($this->hasFailed()) {
                return Command::FAILURE;
            }

            $this->tick();
        }

        return 0;
    }

TerminatedNodeAnalyzer::isAlwaysTerminated() returns false for the while (true) loop as the last statement,
so processReturn0ToMethod() appends return 0; — a statement PHPStan then flags as unreachable
(deadCode.unreachable), since the loop has no break and can only be left via the returns already inside it.

Impact

Hit on the same two commands as #9852 (WmsProductsCommand, WcStockSimulatorCommand) — confusingly, a
combined rector process --dry-run run attributed this specific diff hunk to an unrelated rule
(CommandConfigureToAttributeRector) in its "Applied rules" footer; running ConsoleExecuteReturnIntRector in
isolation via --only=... on the same file reproduced the diff on its own with 0 involvement from
CommandConfigureToAttributeRector, confirming the report's rule attribution for that hunk was misleading.

Expected behavior

TerminatedNodeAnalyzer::isAlwaysTerminated() should recognize a while (true) (or for (;;)) loop with no
break targeting it as always-terminating from the perspective of "control never reaches past this statement" —
the same gap as #9852, just exercised through a different call site.

Workaround

->withSkip([
    ConsoleExecuteReturnIntRector::class,
])

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions