Skip to content

Re-process a stored scope-changing expression at a foreign ask position instead of re-anchoring it - #6832

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

phpstan-bot wants to merge 1 commit into
phpstan:2.3.xfrom
phpstan-bot:create-pull-request/patch-g831x5g

Conversation

@phpstan-bot

Copy link
Copy Markdown
Collaborator

Summary

Since 2.3.0, array_filter(array_map(function (string $a): ?string { $id = f($a); if ($id === null) { ... } return $id; }, explode('|', (string) ($data['x'] ?? '')))) was inferred as filtering a non-empty-list<null>, which reported a false arrayFilter.alwaysEmpty. The callback's return type was computed wrong. The fix re-processes stored expressions that changed the scope when an on-demand walk asks for them at a different position. Before, they were re-anchored, and that dropped their effect.

Changes

  • src/Analyser/NodeScopeResolver.php (processExprNode()): in the stored-result path (not consume mode), when the stored result's after-scope is not its before-scope and the node is not a closure or arrow function, call processExprNodeInternal() on the asking scope. Before, the code returned atAskPosition($scope).
  • turbo-ext/src/NodeScopeResolver.cpp (processExprNodeBody()): the same change in the native mirror.
  • Analogous cases I probed, all in the same "callback priced on demand inside another call's argument" setup:
    • had the same bug, now fixed: $i += ..., $i++, [$id] = [...]
    • already worked, kept as coverage of the same path: ??=, by-ref out parameter (preg_match(..., $m)), an arrow function whose body assigns, an array_filter() callback

Root cause

ArrayMapFunctionReturnTypeExtension prices the element type with a synthetic call, $callback(TypeExpr(...)). It is processed on demand with returnStoredExpressionResults enabled, so the real nodes in the closure body come from storage. A stored result is reused at a foreign position when the asking scope agrees on the variables the node reads. ExpressionResult::atAskPosition() then rebuilds the result with scope = beforeScope = asking scope. That is fine for pure nodes, but it loses every write a scope-changing node makes. Here $id = idFromAlias($alias) read $alias, which matched, but $id was never defined. After the if ($id === null) {} merge, $id was only "maybe null", so the callback appeared to return null. The ?? / ternary in the other argument and the outer call (array_filter) are only what made the body's stored results visible to this on-demand walk. Re-processing nodes whose stored walk changed the scope restores their effects. Their side-effect-free subnodes (idFromAlias($alias)) still answer from storage.

Test

  • tests/PHPStan/Analyser/nsrt/bug-15491.php: the issue's reproducer (assertType on the filtered result), plus the analogous cases above (+=, ??=, ++, by-ref out parameter, list destructuring, arrow function, array_filter callback). Without the fix, the reproducer, +=, ++ and list destructuring fail.
  • tests/PHPStan/Rules/Functions/ArrayFilterRuleTest::testBug15491 with data/bug-15491.php: the playground sample verbatim. It expects no error; without the fix it reports arrayFilter.alwaysEmpty on line 16.

Fixes phpstan/phpstan#15491

🤖 Generated with Claude Code

…on instead of re-anchoring it

- NodeScopeResolver::processExprNode() answered an on-demand walk from a
  node's stored ExpressionResult re-anchored with atAskPosition(), which sets
  both the before and the after scope to the asking scope. For a node that
  changed the scope (assignment, assign-op, ++/--, list destructuring, ...)
  this dropped its effect: when ArrayMapFunctionReturnTypeExtension priced
  the synthetic `$callback(...)` call, the closure body's `$id = f()` was
  answered from storage without defining `$id`, so only the `if ($id ===
  null)` branch survived and the callback seemed to return `null` only.
- Such a node (stored after-scope !== before-scope) is now re-processed on
  the asking scope; its pure subnodes still answer from storage. Closures and
  arrow functions keep the existing answer (their stored result is their
  converged walk). The consume mode is unchanged.
- Mirrored in turbo-ext/src/NodeScopeResolver.cpp (processExprNodeBody()).
- Probed `+=`, `??=`, `++`, by-ref out parameter (preg_match), list
  destructuring, arrow function with an assignment and an array_filter()
  callback: `+=`, `++` and list destructuring had the same bug; the others
  already worked and are kept as coverage of the same path.
@ondrejmirtes
ondrejmirtes force-pushed the create-pull-request/patch-g831x5g branch from d291bf2 to c8d8c11 Compare October 27, 2026 18:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant