Keep list type when writing to $list[count($list)] and compare the offset's array through its printed expression

[phpstan-bot]

Summary

Assigning $list[count($list)] = $value appends right behind the last element and therefore keeps the array a list, but PHPStan degraded list<int> to non-empty-array<int<0, max>, int> and reported non-empty-array<int<0, max>, int> might not be a list.

AssignHandler::shouldKeepList() already knew a family of list-preserving offset idioms (count($list) - n, array_key_last(), array_key_first(), array_search(), $index + 1) but was missing the plain count($list) one. Probing the neighbours of that heuristic turned up four more defects in the same code, all fixed here.

Changes

All in src/Analyser/ExprHandler/AssignHandler.php:

• shouldKeepList() recognizes $list[count($list)] and $list[sizeof($list)].
• isSameVariable() is replaced by isSameArrayExpr() / isStableExpr(), which compare two array expressions by their printed form after checking they are side-effect free and stable (variables, property fetches, static property fetches, and dim fetches over those with scalar or stable dims). Every heuristic in shouldKeepList() now also fires for $this->list, self::$staticList, $this->nested['x'] and $data[$key].
• New $list[array_key_last($list) + 1] append idiom, guarded by isIterableAtLeastOnce() because array_key_last() returns null for an empty list, making null + 1 === 1 leave a hole.
• isFuncCallOnSameArray() rejects unpacked (count(...$list)) and named arguments, so they no longer masquerade as count($list).
• The array_search() branch required >= 1 arguments but read getArgs()[1]; $list[array_search($list)] crashed with AssignHandler::isSameVariable(): Argument #2 ($b) must be of type PhpParser\Node\Expr, null given. It now requires >= 2.
• produceArrayDimFetchAssignValueToWrite() hands shouldKeepList() the dim fetch carried by $offsetTypes instead of $dimFetchStack[$i]. The loop walks $offsetTypes reversed, so for a nested write the two indexes point at different links of the chain. The hasExpressionType() check above it deliberately keeps using $dimFetchStack[$i] — changing it there regresses nested constant-array precision (assign-nested-arrays.php, pr-4390.php) and is out of scope here.
• The additional expression types registered after the write are skipped when the offset expression no longer resolves to the offset that was just written.

Root cause

Two independent root causes.

1. A missing member of a heuristic family, plus a too-narrow expression comparison. shouldKeepList() enumerates offset expressions that provably keep an array a list. count($list) was simply absent from that list. The comparison that decides "is this the same array?" was isSameVariable(), which only ever returned true for two Variable nodes — so none of the heuristics worked when the array was a property, a static property, or a nested offset. The same narrowness meant nested writes never matched either, compounded by the wrong $dimFetchStack index.

2. The offset expression is re-evaluated after the container changed. applyWrite() registers the written dim fetch in the scope via assignExpression(). MutatingScope::specifyExpressionTypeInPlace() then re-reads $expr->dim in the post-assignment scope and intersects the container with HasOffsetValueType. When the dim depends on the container — count($list) — it resolves to a different value than the one that was written, and the intersection is unsatisfiable:

   $list = [1, 2, 3];
   $list[count($list)] = 37;      // array{1, 2, 3, 37} intersected with hasOffsetValue(4, 37)
   \PHPStan\dumpType($list);      // *NEVER* before, array{1, 2, 3, 37} now

   The same collapse happened for $list[count($list) + 1] = 37. Skipping the registration when the recomputed offset type differs from the written one fixes both.

Test

tests/PHPStan/Analyser/nsrt/bug-15080.php holds the playground reproducer verbatim plus the analogous cases found while probing:

• count() and sizeof() on a local variable, a property, a static property, a nested property, a nested local array, and a dim fetch with a variable key
• $list[count($list)] ??= $v (assign-op path), count($list) - 0, and appending in a for loop
• array_key_last($list) + 1 and 1 + array_key_last($list) on a non-empty list, and the possibly-empty-list counterpart that must not stay a list
• the *NEVER* regressions: $list = [1, 2, 3]; $list[count($list)] = 37; and $list[count($list) + 1] = 37;
• array_key_last(), count($list) - 1 and array_search() on $this->list, which the existing heuristics missed
• negative cases that must keep degrading to non-empty-array: count($other), count($list, COUNT_RECURSIVE), count(...$list)
• $list[array_search($list)], which used to abort the analysis with an internal error

tests/PHPStan/Rules/Functions/ReturnTypeRuleTest::testBug15080() locks the reported symptom itself — the reproducer must analyse without the return.type error.

Both fail without the fix (the rule test with the exact message from the issue, the NSRT file with 18 wrong types and an internal error) and pass with it. make tests, make phpstan and make cs are green.

Fixes phpstan/phpstan#15080

[SanderMuller]

I went through this one and rebuilt the claims rather than reading them, since the diff reaches a fair way past the issue it closes. Short version: the parts I expected to be risky measured out fine, and the one thing I would want your call on is the index question the description already raises.

Verified

The tests do fail without the source change. Reverting only AssignHandler.php and keeping both test files: ReturnTypeRuleTest::testBug15080 fails with the message from the issue, and the NSRT file errors out. Full suite green with the change (21331 tests), self analysis clean.

The index mismatch is real. At the construction site $dimFetchStack is reversed before $offsetTypes is built from it, so $offsetTypes[$k][1] === $dimFetchStack[$k]. The consumer then walks array_reverse($offsetTypes) while indexing $dimFetchStack[$i], so from depth 2 the two point at opposite ends of the chain. At depth 1 they coincide, which is why this only ever showed up nested.

The "out of scope" note is honest, and understated. Aligning $arrayDimFetch with $writtenDimFetch on the line above regresses five NSRT files, not the two named: pr-4390, bug-13786, bug-13637, assign-nested-arrays, bug-14084.

Concerns I had and could not substantiate

Recording these so nobody has to re-litigate them.

Comparing offsets through the printed expression is not on a hot path. This was my main worry, since isSameVariable() was two instanceof checks and a string compare. Counting calls with the branch instrumented:

corpus	files	isSameArrayExpr()	printExpr pairs
phpstan-src src/	1746	3	3
vendor/	3003	0	0
PHPStan extension packages	1090	0	0

Three calls across about 5800 files of real code. The function name checks in isFuncCallOnSameArray() run first and reject nearly everything, so printing is reached only for a write shaped like $x[count(...)] on a list.

The re-evaluation added to every additional expression is cheap too. 4531 extra getType() calls across all of src/, 5495 across vendor/, all memoized lookups.

No new constructor dependency (exprPrinter was already injected), and AssignHandler carries no #[ShadowedByTurboExtension], so there is no native mirror to keep in step.

The new skip guard does not change any finding on real code. Analysing vendor/ (3003 files, which contains every site where the guard fires) gives 29250 findings, identical with and without the change. Nothing gained, nothing lost.

One thing worth adding to the description

The guard fires on real code, but never for the reason given. Across vendor/ it fires 20 times, on five distinct expressions and no count() call among them:

10x  $result[++$i]              phpunit/php-code-coverage
 7x  $matches[$numMatches++]    symfony/console
 1x  $lines[key($lines)]        ondrejmirtes/php-merge
 1x  $merged[key($merged)]      ondrejmirtes/php-merge
 1x  $conflicts[key($conflicts)] ondrejmirtes/php-merge

The increments are clearly right to skip: re-evaluating $i++ after the write cannot describe the offset that was written.

The key() ones are a different case, and they are the reason I would reword rather than change anything. Those sites are the end($lines); $lines[key($lines)] = $x; idiom, so the offset does not move; the guard fires because the recomputed type widened once the container changed, not because the expression stopped designating the same element. The condition therefore conflates "the offset moved" with "the offset's type got wider". It happens to be inert either way, per the diff above, so this is about the description being accurate rather than about the code being wrong.

Questions

1. The change leaves two adjacent lines disagreeing about which index is right: hasExpressionType($dimFetchStack[$i]) above, shouldKeepList($writtenDimFetch) below. Both cannot be correct. Either the mirrored index happens to select the right link for the cases those five NSRT files cover, or the loop's index convention is wrong and those files have been encoding the compensation. Worth settling before this lands, because the next person in that loop will hit it.

2. This is five fixes in one PR. The array_search() argument count fix stands alone and could land on its own: $list[array_search($list)] aborts analysis on current 2.2.x with Argument #2 ($b) must be of type PhpParser\Node\Expr, null given. The call is malformed either way, but an internal error rather than a reported one is a different class of problem to the rest of this.

[staabm]

This is five fixes in one PR. The array_search() argument count fix stands alone and could land on its own: $list[array_search($list)] aborts analysis on current 2.2.x with Argument #2 ($b) must be of type PhpParser\Node\Expr, null given. The call is malformed either way, but an internal error rather than a reported one is a different class of problem to the rest of this.

@SanderMuller please send a separate PR

[phpstan-bot]

Pushed. Here's the response to the review.

---

Thanks for rebuilding rather than reading — that made both questions answerable with data instead of opinion.

Question 1 — the index disagreement

I went and settled it. You were right that both cannot be correct, and the answer is neither convenient nor a one-line fix.

Where the index came from. $arrayDimFetch = $dimFetchStack[$i] ?? null was introduced in 9efcdf5 ("Fix lost list-type if substituted a element via loop") solely for the list-keeping check — at that point $offsetTypes was still list<Type|null> and carried no dim fetch, so $dimFetchStack[$i] was the only way to reach one. 7cb1163 later reused the same variable for the setExistingOffsetValueType() branch. Both were written against the non-nested case, where the mirrored and the written index coincide. So the convention is not a deliberate choice on either line; the second usage inherited the first one's index.

What the loop actually holds. I instrumented it to stop guessing. At iteration $i, $offsetType is the offset written by $writtenDimFetch, and $offsetValueType is the type of $writtenDimFetch->var — the container it goes into. For $array[$i][$j]['baz'] = 2:

i=0  offsetType='baz'  container=array{bar: 1}                                written=$array[$i][$j]['baz']  mirror=$array[$i]
i=1  offsetType=int    container=non-empty-array<int, array{bar: 1}>          written=$array[$i][$j]        mirror=$array[$i][$j]
i=2  offsetType=int    container=non-empty-array<int, non-empty-array<...>>   written=$array[$i]            mirror=$array[$i][$j]['baz']

So $dimFetchStack[$lastDimKey - $i] is the written link and $dimFetchStack[$lastDimKey - $i - 1] is the container. The mirror is the container at depth 2, the written link at depth 1, and neither from depth 3.

Why the five files regress, concretely. It is $i = 2 above that decides it. The mirror $array[$i][$j]['baz'] is untracked before the assignment, so the write falls to setOffsetValueType() and replaces the container's value type. $writtenDimFetch ($array[$i]) is tracked, so it would take setExistingOffsetValueType() and union the old value type into the new one — array{bar: 1} ∪ array{bar: 1, baz: 2} = array{bar: 1, baz?: 2}, which is exactly the reported failure. Every one of the five is that same widening at the outermost level of a nested write.

And the other index doesn't work either. I measured all four candidates:

$arrayDimFetch =	regressions
$dimFetchStack[$i] (today)	none
written — $dimFetchStack[$lastDimKey - $i]	assign-nested-arrays, pr-4390, bug-13637, bug-13786, bug-14084
container — $dimFetchStack[$lastDimKey - $i - 1] ?? null	bug-12274 (isset($list[$i]); $list[$i] = 21 loses list at depth 1)
container with the root var as fallback	6 files, incl. bug-15080 over-firing
written OR container	4 files

So the mirror is not selecting the right link — it is standing in for a distinction the branch cannot currently make. The condition wants "this write replaces an element I already know about", and hasExpressionType() is a proxy for that which answers differently depending on which link you hand it. Fixing the index properly means first teaching that branch to tell replacing a known element from widening a maybe-existing one — a behavioural change across those five files, not a rename.

I've left the behaviour alone and instead made the trap legible (c097427): the variable is now $mirroredDimFetch, and the comment states what the loop holds, that the mirror is not the written link, what happens if you swap it, and which files prove it. The next person in that loop gets the measurement instead of having to redo it.

The key() observation

You're right, and the comment was wrong on its face. The condition is an equality on the offset type, so it fires for three different reasons — the offset moved because it derives from the container (count($list)), it moved because of a side effect ($i++), or it merely widened while still designating the same element (end($a); $a[key($a)] = …). I confirmed your inertness result directly rather than by diffing findings: end($a); $a[key($a)] = 5; dumps non-empty-array<string, int> / 5 both with and without the guard, byte-identical, because key($a) is re-resolved lazily on read anyway. Same for $result[++$i].

I kept the equality — it's the only cheap evidence that the expression still points at what was written, and loosening it to a supertype check would register a value under an offset that may have moved — and rewrote the comment to name all three cases and say why they're treated alike (69412bc). Nothing observable to lock in a test, so I didn't add one.

Question 2 — the array_search() crash

Agreed it's a different class of problem, and it is genuinely self-contained: the guard is isset($args[$argPosition]) in isFuncCallOnSameArray(), plus arraySearchWithSingleArg() in the NSRT file. I've left it in place here rather than rewriting the branch, since force-pushing a split would detach your verification from the diff it was measured against — but if you'd rather have it as a standalone PR against 2.2.x I'll extract those two pieces and rebase this one on top. Your call.

I also checked whether the same count-guard-disagrees-with-index shape exists elsewhere in this file; it doesn't. There are unguarded getArgs()[N] reads across src/Type/Php/, but those sit behind their own arity checks, so I left that sweep alone.

Verification

make tests 21278 passing (97 skipped), make phpstan no errors, make cs clean, on top of both new commits. Pushed as 69412bc21 and c0974275f.