Skip to content

Keep iterating a for loop when a nested loop hides its back edge - #6744

Open
HawkForge wants to merge 2 commits into
phpstan:2.3.xfrom
HawkForge:fix/loop-dead-back-edge-in-deep-pass
Open

HawkForge wants to merge 2 commits into
phpstan:2.3.xfrom
HawkForge:fix/loop-dead-back-edge-in-deep-pass

Conversation

@HawkForge

Copy link
Copy Markdown
Contributor

Closes phpstan/phpstan#15420

Since 2.3.0, a for loop whose body only loops back after a nested loop has iterated more than once is analysed as if it ran exactly once. The loop condition is then reported as always true:

/** @param string[] $cards */
function f(array $cards): void
{
	for ($i = 0; $i < 10; $i++) { // Comparison operation "<" between 0 and 10 is always true.
		$seen = [];
		$later = [];
		foreach ($cards as $card) {
			if (array_key_exists($card, $seen)) {
				$later[] = $card;
				continue;
			}
			$seen[$card] = true;
		}
		if ($later !== []) {
			$cards = $later;
		} else {
			break;
		}
	}
}

ForHandler's convergence passes walk the body in deep context, where the inner foreach runs its body once and does not converge. In that walk $seen is still array{}, so array_key_exists() is always false, $later stays [] and the body always reaches break. getLoopBackEdgeScope() returns null and the pass stopped with $bodyScope = $prevScope, so $i++ was never applied and $i stayed 0. The final top-level walk does converge the inner foreach and finds the back edge, but by then the condition had already been priced with $i as 0.

2.2.17 continued from the end of the body in every pass, so the pass reached $i++ and the counter widened to int<0, 9>. Going back to that everywhere would undo the precision 2.3 gained for loops that really never loop back (for-loop-i-type.php expects 1 after for ($i = 1; $i < 50; $i++) { break; }, and 2.2.17 gave int<1, max>). This PR therefore uses isReplayableConvergenceBody() to tell the two cases apart:

  • If the body is replayable (no nested loop, label or class-like), the deep walk is the same as the final one, so a dead back edge is still trusted and the pass stops as before.
  • Otherwise, ForHandler continues from $bodyScopeResult->getScope(), as 2.2.17 did.
  • turbo-ext/src/ForHandler.cpp gets the same change, and TurboExtensionEnabler::EXPECTED_EXTENSION_VERSION is bumped in a separate commit.
  • tests/PHPStan/Rules/Comparison/data/bug-15420.php is the issue's code unchanged. The test fails without the change in ForHandler.php (it reports the smaller.alwaysTrue error on line 9) and passes with it.

I left WhileHandler, DoWhileHandler and ForeachHandler alone. They stop early in the same way, but I could not find a case that 2.2.17 analysed correctly and 2.3.x does not. For example, a while loop with the $i++ after the break check reports the same false positive on 2.2.17, because the deep walk never reaches the increment. I can look into those in a separate PR if you want.

Verified:

  • The issue's code and some smaller variants give the same output as 2.2.17.
  • make tests passes with and without the turbo extension loaded (built with print-warn-flags).
  • turbo-ext/tests/smoke.php and signature-parity.php pass.
  • The analysis output on the reproducing files is identical with the extension on and off.

The convergence passes walk the body in deep context, where a nested loop
is walked once instead of to its fixpoint. When the back edge is only
reached after the nested loop has iterated more than once, the pass finds
no back edge and stopped with the counter still at its initial value. The
final walk then saw `$i` as `0` and reported `$i < 10` as always true.

A dead back edge is still trusted when the body is replayable - the deep
walk is then the same as the final one. Otherwise the pass continues from
the end of the body, as 2.2 did.

Closes phpstan/phpstan#15420
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.

False positive smaller.alwaysTrue (2.3.0 regression)

1 participant