Skip to content

[DeadCode] Skip foreach over side-effecting expression in RemoveDeadIfForeachForRector#8106

Merged
TomasVotruba merged 1 commit into
mainfrom
tv-fix-foreach-side-effect
Jun 29, 2026
Merged

[DeadCode] Skip foreach over side-effecting expression in RemoveDeadIfForeachForRector#8106
TomasVotruba merged 1 commit into
mainfrom
tv-fix-foreach-side-effect

Conversation

@TomasVotruba

Copy link
Copy Markdown
Member

Problem

RemoveDeadIfForeachForRector removed a foreach with an empty body even when the iterated expression has side effects, e.g. consuming a generator:

public function run(): WorkflowState
{
    if (! isset($this->result)) {
        foreach ($this->events() as $event) {
        }
    }

    return $this->result;
}

Here $this->events() is a generator that sets $this->result as it is consumed. The empty body is not dead code — iterating is the work. The rule deleted the whole if/foreach, silently breaking behavior.

Spotted in the wild: https://github.com/neuron-core/neuron-ai/blob/3.x/rector.php#L23-L27 had to withSkip() this rule on WorkflowHandler.php.

Fix

processForForeach() only checked whether loop variables were used in the next statement; it ignored side effects in the iterated expression itself. Bail out when $for->expr contains a CallLike/Assign (reusing the existing hasNodeSideEffect() helper already used by processIf()).

Added skip_foreach_over_call_side_effect.php.inc fixture covering the case.

@TomasVotruba

Copy link
Copy Markdown
Member Author

@TomasVotruba TomasVotruba merged commit 07d1687 into main Jun 29, 2026
65 checks passed
@TomasVotruba TomasVotruba deleted the tv-fix-foreach-side-effect branch June 29, 2026 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant