Skip to content

[scope] Swap instanceof chain for property_exists guard in scope expr dispatch - #8789

Closed
TomasVotruba wants to merge 2 commits into
mainfrom
tv-bench-expr-check
Closed

TomasVotruba wants to merge 2 commits into
mainfrom
tv-bench-expr-check

Conversation

@TomasVotruba

@TomasVotruba TomasVotruba commented Oct 23, 2026 •

Copy link
Copy Markdown
Member

Replaces the 17-branch instanceof chain in PHPStanNodeScopeResolver (the guard that propagates scope to $node->expr) with property_exists($node, 'expr') plus 5 explicit exclusions.

Equivalence

A raw property_exists swap is NOT equivalent - it also matches Assign, AssignOp, AssignRef, ArrowFunction and Foreach_. The first three and Foreach_ have dedicated branches right below and must fall through to them; ArrowFunction->expr must not inherit the scope here.

Excluding those 5 makes it match the chain exactly. Verified against all 42 expr-bearing php-parser node classes (AssignOp is abstract and covers its 13 subclasses): the guarded predicate matches the same 25 node types the chain lists, no more, no less.

if (property_exists($node, 'expr')
    && $node->expr instanceof Expr
    && ! $node instanceof Assign
    && ! $node instanceof AssignOp
    && ! $node instanceof AssignRef
    && ! $node instanceof ArrowFunction
    && ! $node instanceof Foreach_
) {
    $node->expr->setAttribute(AttributeKey::SCOPE, $mutatingScope);
    return;
}

Speed

The bare property lookup is ~3.6x cheaper than the chain (~184 ns vs ~668 ns/call). The shipped guarded variant adds 5 instanceof back for the expr-bearing case, but the common path (non-expr nodes, the majority of visited nodes) short-circuits on the single property_exists miss instead of walking 17 instanceof.

Bench

bench/scope_expr_dispatch_bench.php prints the divergent set for the raw swap, confirms the guarded variant is identical to the chain, and the CPU ratio:

php bench/scope_expr_dispatch_bench.php

Full vendor/bin/phpunit green (4869 tests). property_exists is whitelisted for this one file in phpstan.neon.

@TomasVotruba TomasVotruba changed the title Bench: instanceof chain vs property_exists for scope expr guard Swap instanceof chain for property_exists guard in scope expr dispatch Oct 23, 2026
@TomasVotruba TomasVotruba changed the title Swap instanceof chain for property_exists guard in scope expr dispatch [scope] Swap instanceof chain for property_exists guard in scope expr dispatch Oct 23, 2026
@TomasVotruba
TomasVotruba marked this pull request as draft October 23, 2026 19:07
@TomasVotruba

Copy link
Copy Markdown
Member Author

Folded the bench into #8790 together with the NoLongInstanceofChainRule. Note: the property_exists($node, 'expr') swap commit on this branch was left out on purpose - it is not equivalent to the instanceof chain (it also matches Assign/AssignOp/AssignRef, which have dedicated processAssign handling, plus ArrowFunction and Foreach_), so it changes behavior.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant