Narrow foreach body to non-empty regardless of polluteScopeWithAlwaysIterableForeach#6094
Narrow foreach body to non-empty regardless of polluteScopeWithAlwaysIterableForeach#6094zonuexe wants to merge 1 commit into
Conversation
568511a to
8aa4092
Compare
8aa4092 to
b5a6e6e
Compare
|
This pull request has been marked as ready for review. |
| if ($commonReasons !== null && count($commonReasons) > 0) { | ||
| $decorated = []; | ||
| foreach (array_keys($innerAccepts) as $i) { | ||
| foreach (array_keys($innerAccepts) as $innerAcceptsIndex) { |
There was a problem hiding this comment.
I think we have the same problem here.. this change got necessary because the PR broke polluteScopeWithAlwaysIterableForeach?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
please remove unrelated changes in
- src/Type/UnionType.php
- src/Type/Constant/ConstantArrayType.php
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
b5a6e6e to
54ac5fd
Compare
| polluteScopeWithAlwaysIterableForeach: false | ||
| featureToggles: | ||
| narrowForeachBodyNonEmpty: true |
There was a problem hiding this comment.
do we need more tests with other combinations of this toggles?
There was a problem hiding this comment.
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.
54ac5fd to
3af0acb
Compare
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
3af0acb to
c3389dc
Compare
Closes phpstan/phpstan#13312
This reproduces with
polluteScopeWithAlwaysIterableForeach: false, whichphpstan-strict-rulessets.Inside the loop body
$arris provably non-empty. PHPStan already narrowed it there, but only as a side effect ofpolluteScopeWithAlwaysIterableForeach: 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 underphpstan-strict-rulesand the issue was reopened.Opt-in toggle
The narrowing is gated behind a new feature toggle
narrowForeachBodyNonEmpty, off by default.Narrowing the body also feeds the loop-exit scope, which with the flag off should 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 leak, the toggle is not enabled under bleedingEdge yet and stays opt-in until it is fully contained. That also keeps the change out of self-analysis, so no unrelated source needs touching.
Bug13312NoPolluteTestcovers the toggle on;Bug13312StableTestcovers the default (off), where the foreach body is left as-is.