Skip to content

Converge loops nested in a top-level loop's convergence passes instead of walking them once - #6820

Open
phpstan-bot wants to merge 2 commits into
phpstan:2.3.xfrom
phpstan-bot:create-pull-request/patch-ex1e5r0
Open

phpstan-bot wants to merge 2 commits into
phpstan:2.3.xfrom
phpstan-bot:create-pull-request/patch-ex1e5r0

Conversation

@phpstan-bot

@phpstan-bot phpstan-bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

An array appended inside a loop that also contains a nested loop came out as array{}|array{string} instead of list<string>. With bleeding edge, sortArray.noop then reported a false positive on sort(). This change makes a nested loop converge during the outer loop's convergence passes, instead of running only one iteration there.

Changes

  • src/Analyser/StatementContext.php: new shouldConvergeLoops() and enterLoopPass().
    • A top-level loop's passes run in a deep context whose loops still converge.
    • Those nested loops' own passes run in a plain deep context, where loops are walked once.
    • This keeps the cost polynomial in the nesting depth instead of exponential.
  • src/Analyser/StmtHandler/{Foreach,While,DoWhile,For}Handler.php:
    • Converge when shouldConvergeLoops() instead of isTopLevel().
    • Walk passes with enterLoopPass() instead of enterDeep().
    • Constant-array foreach unrolling stays top-level only.
  • turbo-ext/: native mirrors of the above.
    • StatementContext.cpp: new slot, methods, and pt_statement_context_should_converge_loops() / pt_statement_context_enter_loop_pass().
    • The four loop handlers updated the same way.
    • generated/StatementContext.h regenerated.
  • tests/PHPStan/Analyser/nsrt/bug-10438.php: the old expectation array<string, array{}|array{string}|string> recorded this same bug. The inner foreach appends any number of strings, so array<string, list<string>|string> is correct.

Root cause

Before this change, loops converge only at the top level. Inside an outer loop's convergence pass, every nested loop body was walked exactly once. In the issue's code, $n therefore came out of the inner loop as 0|1 during the passes, and $n > 1 was never true. The outer loop converged with $keys still array{}. Only the final walk, which does converge the inner loop, saw $n as int<0, max>, and it appended once, giving array{}|array{string}. This is not a 2.3 regression: 2.2.9 infers the same type.

The pattern affects every loop kind on both sides. I tested inner foreach/while/for/do-while loops inside an outer foreach, and an inner foreach inside outer while/for/do-while loops. All of them were broken and all are fixed by the same change.

Performance

Self-analysis of src/ (interleaved A/B, user CPU, no turbo extension): 207s before, 221s after, about 6–7% slower. This is the cost of iterating nested loops to a fixpoint during the outer passes, where they used to run once. A variant where the nested loop's final walk did not converge its own nested loops was measured at ~219s, within noise, and was not adopted.

Test

  • tests/PHPStan/Analyser/nsrt/bug-15435.php: the issue's reproducer verbatim, plus analogous cases:

    • inner while, for, and do-while loops;
    • outer while, for, and do-while loops;
    • a flag set inside the inner body when a counter exceeds 1;
    • three levels of nesting.

    All nine assertions fail without the fix (array{}|array{string}) and pass with it.

  • SortWithoutEffectRuleTest::testBug15435: no sortArray.noop false positive on sort($keys). It fails without the fix.

  • Full make tests passes with and without the turbo extension loaded. turbo-ext/tests/walk-trace.php is identical (676,444 trace lines). smoke.php and signature-parity.php are OK, and make phpstan is clean.

Fixes phpstan/phpstan#15435
Closes phpstan/phpstan#15420
Closes phpstan/phpstan#10666
Closes phpstan/phpstan#14935

🤖 Generated with Claude Code

…d of walking them once

- StatementContext: add shouldConvergeLoops() and enterLoopPass(). A top-level
  loop's passes now run in a deep context whose loops still converge; the
  passes of those nested loops run in a plain deep context, where loops are
  walked once, so the cost stays polynomial in the nesting depth.
- ForeachHandler, WhileHandler, DoWhileHandler, ForHandler: converge when
  shouldConvergeLoops() (was isTopLevel()) and walk passes with
  enterLoopPass() (was enterDeep()). Foreach unrolling over constant arrays
  stays top-level only.
- Previously a nested loop inside an outer pass ran a single iteration, so a
  counter it increments stayed 0|1. A condition such as `$n > 1` was then
  never true in the outer loop's passes, and an array appended under it
  converged to array{}|array{string} instead of list<string>.
- Analogous cases fixed by the same change and covered by tests: nested
  while/for/do-while inner loops, while/for/do-while outer loops, a flag set
  inside the inner body, and three levels of nesting.
- turbo-ext: mirror StatementContext (new slot, methods, direct entries) and
  the four loop handlers; regenerate generated/StatementContext.h.
- bug-10438: the expectation recorded the same bug; an inner foreach appends
  any number of strings, so list<string> is correct.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@ondrejmirtes
ondrejmirtes force-pushed the create-pull-request/patch-ex1e5r0 branch from 16f2934 to 0367cfc Compare October 27, 2026 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants