Skip to content

Narrow foreach body to non-empty regardless of polluteScopeWithAlwaysIterableForeach - #6094

Draft
zonuexe wants to merge 2 commits into
phpstan:2.2.xfrom
zonuexe:foreach-non-empty-narrowing
Draft

Narrow foreach body to non-empty regardless of polluteScopeWithAlwaysIterableForeach#6094
zonuexe wants to merge 2 commits into
phpstan:2.2.xfrom
zonuexe:foreach-non-empty-narrowing

Conversation

@zonuexe

@zonuexe zonuexe commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Closes phpstan/phpstan#13312

/** @param list<mixed> $arr */
function foo(array $arr): void {
	foreach ($arr as $v) {
		bar($arr);
	}
}

/** @param non-empty-list<mixed> $arr */
function bar(array $arr): void {}

This reproduces with polluteScopeWithAlwaysIterableForeach: false, which phpstan-strict-rules sets.

Inside the loop body $arr is provably non-empty. PHPStan already narrowed it there, but only as a side effect of polluteScopeWithAlwaysIterableForeach: with the flag off the in-body narrowing disappeared. A previous fix (phpstan-src#4162) added a test for it, but only under the default flag value, so the reported case kept failing under phpstan-strict-rules and the issue was reopened.

Opt-in toggle

The narrowing is gated behind a new feature toggle narrowForeachBodyNonEmpty, off by default and not enabled under bleedingEdge.

  • off (default): the body scope is built exactly as before, no behaviour change.
  • on: the foreach body narrows the iterated expression regardless of the flag, so #13312 is fixed.

The narrowing also sharpens the after-loop scope: a value variable can be seen as defined where the iteratee is later proven non-empty (e.g. via count() > 0). This is more precise but changes what polluteScopeWithAlwaysIterableForeach: false reports, so the toggle stays opt-in for now rather than being enabled under bleedingEdge.

Bug13312NoPolluteTest covers the toggle on; Bug13312StableTest covers the default (off), where the foreach body is left as-is.

@zonuexe
zonuexe force-pushed the foreach-non-empty-narrowing branch 2 times, most recently from 568511a to 8aa4092 Compare July 23, 2026 13:07
Comment thread src/Command/AnalyseCommand.php Outdated
@zonuexe
zonuexe marked this pull request as draft July 25, 2026 14:12
@zonuexe
zonuexe force-pushed the foreach-non-empty-narrowing branch from 8aa4092 to b5a6e6e Compare July 25, 2026 14:13
@zonuexe
zonuexe marked this pull request as ready for review July 25, 2026 14:33
@phpstan-bot

Copy link
Copy Markdown
Collaborator

This pull request has been marked as ready for review.

Comment thread src/Type/UnionType.php Outdated
if ($commonReasons !== null && count($commonReasons) > 0) {
$decorated = [];
foreach (array_keys($innerAccepts) as $i) {
foreach (array_keys($innerAccepts) as $innerAcceptsIndex) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have the same problem here.. this change got necessary because the PR broke polluteScopeWithAlwaysIterableForeach?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, and my earlier reply was wrong. Sorry. I checked: $i in UnionType::accepts() becomes defined after the first loop only because of this change, and it matches what polluteScopeWithAlwaysIterableForeach: true already produces. So it is the same leak, not a flag-independent fact.

The leak flows through conditionals between variables the loop body builds (here $innerAccepts), not through the iterated expression itself, and I could not strip it without also dropping non-emptiness that other code legitimately relies on. So instead of chasing full containment, I've gated the whole behaviour behind a bleedingEdge toggle, narrowForeachBodyNonEmpty.

With bleedingEdge off (stable), polluteScopeWithAlwaysIterableForeach: false behaves exactly as before: no in-body narrowing, no after-loop change. With bleedingEdge on, the body narrows and #13312 is fixed. This rename and the ConstantArrayType guards are only needed because the self-analysis config runs with bleedingEdge on.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please remove unrelated changes in

  • src/Type/UnionType.php
  • src/Type/Constant/ConstantArrayType.php

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I dropped both files. The toggle is no longer enabled under bleedingEdge, so the self-analysis config never turns it on and those source changes are not needed. It is opt-in only until the after-loop leak is fully contained.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The toggle is no longer enabled under bleedingEdge, so the self-analysis config never turns it on and those source changes are not needed. It is opt-in only until the after-loop leak is fully contained.

I think this indicates that this PR is still breaking polluteScopeWithAlwaysIterableForeach: false?

@zonuexe
zonuexe force-pushed the foreach-non-empty-narrowing branch from b5a6e6e to 54ac5fd Compare July 25, 2026 17:34
Comment thread src/Analyser/NodeScopeResolver.php Outdated
Comment thread src/Analyser/NodeScopeResolver.php Outdated
Comment on lines +2 to +4
polluteScopeWithAlwaysIterableForeach: false
featureToggles:
narrowForeachBodyNonEmpty: true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we need more tests with other combinations of this toggles?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added Bug13312StableTest for the toggle-off case (foreach body left as list<mixed>, while the separate for-loop narrowing still applies). Bug13312NoPolluteTest covers toggle-on. Happy to add pollute-on combinations too if you want them.

@zonuexe
zonuexe marked this pull request as draft July 26, 2026 01:27
@zonuexe
zonuexe force-pushed the foreach-non-empty-narrowing branch from 54ac5fd to 3af0acb Compare July 26, 2026 01:34
@staabm
staabm force-pushed the foreach-non-empty-narrowing branch from 3af0acb to c3389dc Compare July 26, 2026 05:16
Inside a foreach body the iterated expression is provably non-empty, so it
can be narrowed (list to non-empty-list, array to non-empty-array) at body
entry. Before, this happened only when polluteScopeWithAlwaysIterableForeach
was on, so with the flag off (as phpstan-strict-rules sets it) the narrowing
disappeared even though the body is only entered when the iteratee is
non-empty.

The narrowing is gated behind the new feature toggle narrowForeachBodyNonEmpty,
off by default. With it off the body scope is built exactly as before, so no
behaviour changes. With it on, the body narrows regardless of the flag.

Narrowing the body also feeds the loop-exit scope, which with the flag off
must stay conservative: it must not conclude the loop always iterated. After
the body pass the iterated expression's possibly-empty-ness is restored in the
after-loop scope, keeping any element types the body refined. This containment
is partial: definedness that flows through variables the body builds (rather
than through the iterated expression itself) still reaches the after-loop
scope. Because of that residual, the toggle is not enabled under bleedingEdge
yet; it is opt-in until the leak is fully contained.

Closes phpstan/phpstan#13312
@zonuexe
zonuexe force-pushed the foreach-non-empty-narrowing branch 2 times, most recently from c363781 to 7cfb7ea Compare August 1, 2026 08:18
The restore that kept the after-loop scope conservative under the toggle had a
guard, `!$iterateeCertainty->no()`, whose TrinaryLogicMutator mutant
(`$iterateeCertainty->yes()`) is equivalent: the two differ only when the
iteratee is a maybe-defined variable, and there the value variable's certainty
is already dominated by the iteratee's own maybe-definedness, so no test can
observe the difference. Infection therefore reports it as escaped and there is
no ignore mechanism.

Since the toggle is opt-in, drop the restore instead of chasing the equivalent
mutant. The foreach body still narrows the iterated expression; the after-loop
scope now simply keeps that precision (a value variable can be seen as defined
where the iteratee is later proven non-empty). The only remaining changed line,
the $iterateeScope condition, is fully covered by the existing bug-13312 tests.
@zonuexe
zonuexe force-pushed the foreach-non-empty-narrowing branch from 7cfb7ea to eb6fb26 Compare August 1, 2026 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Narrow list to non-empty-list inside foreach

3 participants