Skip to content

Do not read scope state in an array argument skeleton after a sibling that may change it - #6709

Open
ondrejmirtes wants to merge 2 commits into
2.3.xfrom
skeleton-stale-scope-state
Open

ondrejmirtes wants to merge 2 commits into
2.3.xfrom
skeleton-stale-scope-state

Conversation

@ondrejmirtes

Copy link
Copy Markdown
Member

Found while reviewing #6701 (point 3 in #6701 (comment)).

ArgumentsHandler::gatherArrayArgTypeSkeleton() prices the keys/values of an array literal argument from the scope as it was before the array is evaluated. An earlier sibling can change that state, so the closures nested in the array were typed from stale values. Because of that, 2.3.x reports a false positive that 2.2.x doesn't:

/**
 * @template T
 * @param array{first: mixed, value: T, callback: callable(T): void} $spec
 */
function run(array $spec): void {}

$i = 0;
run([
	'first' => $i++,
	'value' => $i,                  // the skeleton priced this as 0, at runtime it's 1
	'callback' => function ($v): void {
		if ($v === 1) {             // "Strict comparison using === between 0 and 1 will always evaluate to false."
		}
	},
]);

The same happens with 'first' => $x = 'str', 'value' => $x and with 'first' => $this->reset(), 'value' => $this->prop.

Fix

Before the skeleton is built, the array literal is walked once in PHP's evaluation order. Every key/value evaluated after the first one that may change the scope skips findScopeStateType(), and only position-independent pricing is used for it (the constant-expression resolver, otherwise mixed).

  • Scope-neutral leaves: literals, variable reads, constants, arrow functions, closures without by-ref use, and first-class callables on such operands. Anything else counts as possibly changing the scope.
  • Order inside an item: the key runs before the value, but a plain variable is only read when the item is added to the array, after both. So [$i++ => $i] reads $i after the key ran, and [$j => $j++] reads the key $j after the value ran. Both cases have tests.

A mixed slot only falls back to the template's bound, so this can't make a resolution wrong. Arrays with only neutral leaves before the closures are priced exactly as before, and the existing bug-15269.php assertions are unchanged.

The C++ mirror in turbo-ext/src/ArgumentsHandler.cpp gets the same change, followed by the separate version bump commit.

Verification

  • New tests: nsrt/array-arg-skeleton-stale-scope.php (6 failing assertions before the fix, plus controls that must stay precise) and the false positive above in StrictComparisonOfDifferentTypesRuleTest. Both fail before the fix and pass after.
  • With turbo active, the C++ port was checked the same way: the old C++ fails the same 6 assertions, the new C++ passes.
  • Full suite, without and with the extension loaded: 22,512 tests each. The only failure in the first runs was a missing // lint >= 8.1 on the new fixture, now fixed; RequiredPhpVersionCommentTest and the fixture pass after it.
  • make phpstan, make cs, turbo smoke test, side-by-side method parity and signature parity (16,392 members) all pass. clang-tidy 21 reports nothing on ArgumentsHandler.cpp.

🤖 Generated with Claude Code

https://claude.ai/code/session_01DVpJHCeFGj338Zo5kQhMuJ

ondrejmirtes and others added 2 commits October 8, 2026 20:35
… that may change it

gatherArrayArgTypeSkeleton() prices the keys/values of an array literal
argument from the scope before the array is evaluated. A sibling evaluated
earlier (an assignment, ++/--, a call) can change that state, so the closures
nested in the array were typed from stale values:

    $i = 0;
    run(['first' => $i++, 'value' => $i, 'callback' => function ($v) {
        // $v was 0, it is 1 at runtime
    }]);

Keys/values evaluated after the first one that may change the scope now skip
the scope state. The walk follows PHP's evaluation order: an item's key runs
before its value, but a plain variable is only read when the item is added
to the array, after both.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DVpJHCeFGj338Zo5kQhMuJ
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.

1 participant