Skip to content

Read the updated value of consumed prefix mutations - #6692

Merged
staabm merged 3 commits into
phpstan:2.3.xfrom
drewmt:fix/preincrement-value-flow
Oct 7, 2026
Merged

staabm merged 3 commits into
phpstan:2.3.xfrom
drewmt:fix/preincrement-value-flow

Conversation

@drewmt

@drewmt drewmt commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

Prefix increment/decrement expressions evaluate to the updated value. Their variable flow currently records only consumption of the mutation's inputs, leaving the new write marked unused even when it is returned, such as return (string) ++$count.

Record a target read after the prefix write, preserving the enclosing value-flow target. This also covers consumed array-offset mutations. Postfix expressions keep their existing pre-mutation input flow, and genuinely unused mutation results remain diagnosed. Mirror the change in the native handlers and bump the expected Turbo version.

Closes phpstan/phpstan#15411.

The regression retains the reported reproducer and covers decrement, assignment to a used result and array offsets. It fails with six unused-value diagnostics before the fix and passes after it.

Validation

  • UnusedVariableRuleTest: 33 tests / 33 assertions, with and without Turbo; includes existing unused prefix/postfix controls.
  • Configured PHP 8.4 unit suite, with and without Turbo: 22,450 tests / 96,879 assertions / 106 skips, no failures.
  • PHP 8.3 unit suite: 22,303 tests / 96,630 assertions / 204 skips, no failures.
  • Strict native build, signature parity (16,390 members), differential smoke (ALL OK) and method/declaration parity on PHP 8.3 pass.
  • PHP/native walk traces match across dead-code fixtures and prefix/condition type-inference fixtures (23,342 lines). Raw diagnostics and stderr match byte-for-byte with bleeding edge enabled.
  • Full configured self-analysis with Turbo, scoped PHPStan without Turbo, four-file coding standards, PHP syntax and whitespace checks pass. Full integration and cross-platform CI have not run locally.

Reproduce the rule regression with php vendor/bin/phpunit tests/PHPStan/Rules/DeadCode/UnusedVariableRuleTest.php --filter testBug15411.

@drewmt

drewmt commented Oct 7, 2026

Copy link
Copy Markdown
Contributor Author

CI follow-up: the core tests and static analysis pass. I reproduced Larastan's two failures with both the base and PR PHARs (1,526 tests / 1,868 assertions each). All 12 failing-job diagnostics also match #6691 on the same base; the PHP 8.6 extension jobs fail during Composer resolution. No patch regression is evident, so I've kept this fix focused.

$assignedScope,
beforeScope: $scope,
expr: $expr,
variableFlow: VariableFlow::sequence($varResult->getVariableFlow(), $valueFlowWrite !== null && $context->isValueConsumed() ? VariableFlow::inputs($valueFlowWrite->getId(), $context->getValueFlowTarget() !== null ? $context->getValueFlowTarget()->getId() : null) : null, VariableFlowBuilder::targetWrite($expr->var, VariableWrite::KIND_PRE_DEC, $assignedScope, $storage)),

@staabm staabm Oct 7, 2026 •

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.

maybe there is some code-style rule we could enforce so such long expressions will be split over more lines by default - which would ease reading diffs in such lines (separate PR)

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.

Thanks for reviewing! Agreed, a separate formatting-rule PR would keep this fix focused and make future diffs easier to read.

@staabm

staabm commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

//cc @SanderMuller

@SanderMuller SanderMuller left a comment

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 reviewed this at f4a22bc21 against its base f9a915969, and it fixes phpstan/phpstan#15411 without hiding real unused writes.

The reporter's snippet, at level 10 with bleedingEdge, reports preInc.unused on the base and nothing with this PR. var_dump(++$c) and if (++$c > 3) had the same false positive, and both are fixed too.

These are still reported, the same as on the base:

  • a bare ++$c; and --$c;
  • a second ++$c after $y = ++$c, when only $y is read
  • return $c++; (postInc.unused)
  • an unused ++$a['x']

One result changes, and I think the new one is right. $x = ++$c; return 1; now reports only Variable $x is never read, not also the ++, because the new value goes into $x.

testBug15411 fails with the src changes reverted and passes with them. The full suite passes with and without the turbo extension (22503 tests, 74 skipped). make phpstan reports no errors, and phpcs passes on the touched files.

Turbo: the strict build has no compiler warnings, smoke.php reports ALL OK, and signature-parity.php reports OK. The raw output is identical with and without the extension. I compared my probe file plus the new test data, and src/Analyser plus the dead-code test data. With only the .cpp/.h changes reverted, the turbo build still reports the three false positives, so the port is needed. The C++ builds the target read before the target write, but targetRead() for a variable only builds value objects, and the output matches.

Performance: I ran a cold bin/phpstan analyse src/Type src/Analyser with bleedingEdge and without turbo, 3 interleaved runs. The base took 6.83-7.54 s and 56.2-57.2 s user CPU, and this PR took 6.90-6.99 s and 56.6-57.1 s. The output is identical.

CI: the red checks are the same set as on #6690, which has the same base. I compared the error output of the Rector, Larastan, laravel/framework, magento and shopware jobs between the two PRs, and it is identical. The same holds for the 8.6 nette and phpunit make phpstan jobs.

@staabm
staabm merged commit 48d4823 into phpstan:2.3.x Oct 7, 2026
915 of 927 checks passed
@staabm

staabm commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

thank you!

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 for preInc.unused after upgrading to 2.3.0

3 participants