Skip to content

Rebuild GatheringNodeCallback layers in VirtualAssignNodeCallback instead of wrapping them - #6172

Open
phpstan-bot wants to merge 1 commit into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-po5u1tb
Open

Rebuild GatheringNodeCallback layers in VirtualAssignNodeCallback instead of wrapping them#6172
phpstan-bot wants to merge 1 commit into
phpstan:2.2.xfrom
phpstan-bot:create-pull-request/patch-po5u1tb

Conversation

@phpstan-bot

Copy link
Copy Markdown
Collaborator

Summary

match ($x) { A => $this->counterA++, B => $this->counterB++ }; used as a statement was reported as Expression "match ($x) {…" on a separate line does not do anything. even though every arm mutates a property.

NoopRule skips the report when the statement contained an assignment — NodeScopeResolver tracks that with a $hasAssign gatherer that watches for PropertyAssignNode/VariableAssignNode. Inc/dec goes through NodeScopeResolver::processVirtualAssign(), which does emit a PropertyAssignNode, but the node arrived at the gatherer after $hasAssign had already been read.

Changes

  • src/Analyser/VirtualAssignNodeCallback.php — added a create() factory that recursively rebuilds the callback chain: when the wrapped callback is a GatheringNodeCallback, a new GatheringNodeCallback is returned whose gatherer and inner callback are each filtered separately, instead of one filter wrapped around the whole chain. The constructor is now private.
  • src/Analyser/NodeScopeResolver.phpprocessVirtualAssign() builds its callback with VirtualAssignNodeCallback::create().

Analogous cases fixed by the same change (all verified failing before the fix, passing after, and all covered by the regression test):

  • match arms with $this->prop-- (post-decrement)
  • match arms with $this->arr['k']++ / $this->arr['k']-- (array-dim virtual assign)
  • nested match arms
  • $cond && ($this->a++ > 0);Unused result of "&&" operator.
  • $cond || ($this->b-- > 0);Unused result of "||" operator.
  • [$this->a++];Expression "[$this->a++]" on a separate line does not do anything.

Probed and found already correct, so no test kept for them:

  • and / or / xor statements and ternary statements — these are reported before the hasAssign() check in NoopRule by design (precedence tips), so they are unaffected.
  • plain $this->a = 1; and $this->a += 1; inside match arms — AssignHandler/AssignOpHandler emit through the unwrapped node callback, so the gathering layer was never hidden.
  • list/array destructuring targets (AssignHandler::prepareTarget() with AssignTargetWalkMode::virtualAssign()) — that path passes $nodeCallback straight through.
  • NoopNodeCallback, the only other ShallowNodeCallback — it discards every node, so it has nothing to gather and cannot hide a gatherer.

Root cause

FiberNodeScopeResolver::callNodeCallback() peels GatheringNodeCallback layers off the top of the chain and runs their gatherers synchronously, precisely because "their arrays are read as soon as the enclosing body walk returns". Only the rule-facing remainder may be deferred into a fiber.

VirtualAssignNodeCallback broke that invariant: it wrapped the entire chain — gatherers included — in an opaque ShallowNodeCallback. callNodeCallback() therefore saw no gathering layer, put the whole chain into a fiber, and if the rule handling PropertyAssignNode asked for a type of an expression that had not been walked yet (TypesAssignedToPropertiesRule calls $scope->getType($node->getAssignedExpr())), the fiber parked. It was only resumed at the end of the function body by processPendingFibers() — long after NodeScopeResolver had read $hasAssign and emitted NoopExpressionNode($stmt->expr, false).

The pattern is "a node-callback wrapper that hides gathering layers from the fiber resolver". VirtualAssignNodeCallback was the only wrapper with that shape; the fix makes it preserve the chain structure rather than nest inside it. Because it is the single entry point for every virtual assign, all callers of processVirtualAssign() (inc/dec handlers, unset() of an offset, foreach targets, by-ref function-call writes such as preg_match/array_shift) are fixed at once.

Note that the symptom was rule-set dependent: with only NoopRule registered nothing parks and the bug does not appear, which is why the regression test runs the full rule set.

Test

tests/PHPStan/Analyser/data/bug-15038.php + AnalyserIntegrationTest::testBug15038(). The data file starts with the reporter's playground snippet verbatim, then adds a MoreVirtualAssigns class covering the analogous constructs listed above. The test runs the complete level-8 rule set (so a rule actually parks a fiber, as in the reported scenario) and asserts no errors.

Before the fix the test fails with 8 errors (lines 15, 41, 57, 65, 75, 76, 81); after the fix it passes. make tests, make phpstan and make cs-fix are green, and the affected tests also pass with PHPSTAN_FNSR=0 (fibers disabled).

Fixes phpstan/phpstan#15038

… instead of wrapping them

* `VirtualAssignNodeCallback` used to wrap the whole callback chain, which hid any `GatheringNodeCallback` from `FiberNodeScopeResolver::callNodeCallback()`. The gatherer then ran inside the fiber and could be parked past the point where its result is read.
* Added `VirtualAssignNodeCallback::create()`, which recursively rebuilds the chain so gathering layers stay on the outside and keep running at the emission position; `NodeScopeResolver::processVirtualAssign()` now uses it.
* This restores `hasAssign` on `NoopExpressionNode` for every construct routed through `processVirtualAssign()` - `$this->prop++`/`--`, `++$this->prop`/`--`, `$this->arr['k']++`, by-ref function-call writes, unset-offset and foreach virtual assigns - not just the reported `match` arm case.
* Analogous false positives fixed by the same change: `Unused result of "&&"/"||" operator` when the operand only increments a property, `Expression "[$this->a++]" ... does not do anything`, and nested `match` arms.
* Probed and found already correct: `LogicalAnd`/`LogicalOr`/`LogicalXor` and ternary statements (reported before the `hasAssign` check on purpose), plain `=`/`+=` property assignments and list destructuring (they emit through the unwrapped callback), and `NoopNodeCallback` (discards every node, so it has nothing to gather).

@ondrejmirtes ondrejmirtes left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Test should be in NoopRuleTest rather than AnalyserIntegrationTest.

@phpstan-bot

Copy link
Copy Markdown
Collaborator Author

The suite is still running; I'll report as soon as it lands.

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: Expression "match ($...) {…" on a separate line does not do anything when used with increment/decrement operators

2 participants