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.
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