[phpstan-bot]

Summary

After if (count($arr) > 2) {}, merging the two branches leaves a remembered count($arr) of int<0, max> in the scope. Later narrowing of $arr (the issue used count($arr, COUNT_RECURSIVE) > 2) correctly turned $arr into non-empty-array. But count($arr) was read from the stale remembered entry and stayed int<0, max> when it should have been int<1, max>.

With this change, narrowing an array expression also narrows any remembered count()/sizeof() of that expression.

Changes

• src/Analyser/MutatingScope.php: specifyExpressionTypeInPlace() calls the new private narrowRememberedCountsInPlace(). For each of the keys count(...), sizeof(...), \count(...) and \sizeof(...) built from the expression, if an entry exists and the new type is an array, it is intersected with getArraySize(). This applies to both the PHPDoc and the native expression tables. Certainty is kept, and a result of never is skipped. The key names live in the new private constant COUNT_FUNCTION_KEY_NAMES.
• turbo-ext/src/MutatingScope.cpp: the same logic in the native mirror (narrowRememberedCountsInPlace() / narrowRememberedCountInTable()), plus the native declaration of COUNT_FUNCTION_KEY_NAMES.

Analogous cases probed: all of these were broken the same way, and all are fixed by this change and covered by the test:

• $arr !== [] and $arr === []
• if ($arr) and !empty($arr)
• isset($arr[0]) and array_key_exists()
• sizeof() and recursive sizeof()
• property fetches (count($this->prop))

Root cause

Narrowing an expression's type only rewrote that expression's own entry. Remembered expressions derived from it, here count($expr), were neither invalidated nor refined, so their stale type took precedence over what CountFunctionReturnTypeExtension would have computed from the narrowed array. Assignments were already safe because assignExpression() invalidates dependent entries, and unset() explicitly invalidates count/sizeof. Only narrowing had the gap, and this change closes it.

Test

tests/PHPStan/Analyser/nsrt/bug-13750.php contains the issue's reproducer verbatim, plus the analogous narrowing forms listed above. One extra case checks that an already narrower remembered count (int<3, max>) is kept. The test fails without the fix (9 failing assertions), both with the PHP implementation and with the turbo extension loaded.

The full test suite passes with and without the turbo extension. make phpstan, the turbo smoke test and signature parity also pass.

Fixes phpstan/phpstan#13750

[VincentLanglet]

Is it something really specific to count ?

Also, why do we remember count($expr) given the fact int<0, max> doesn't give any type improvement

[VincentLanglet]

What's the perf impact of checking this every time ?

[phpstan-bot]

I didn't change any code. The measurements below show the check costs nothing measurable, so there was nothing to commit or push, and the working tree is clean.

What's the perf impact of checking this every time?

It's negligible. The expensive part only runs when it has something to do:

• The cheap path: most merges never get past the first loop. That loop only looks at keys whose types differ between the two branches, keeps those where the merged entry is certain, and checks with an instanceof that it is a function, method or static call. Only if such an entry exists does it build a scope without them and resolve the calls again.
• How often it runs (PHPStan analysing its own source, make phpstan): findUninformativeRememberedCalls() was called 79,480 times. Only 8,692 of those (≈11%) had any candidate calls and built the extra scope. They checked 11,737 candidate calls in total, and 6,104 entries were dropped.
• Time spent inside it: 1.27 s of CPU summed over all 4 workers, out of about 307 s of worker CPU. That's about 0.4%.

End-to-end comparison (with a cleared result cache, this PR against 9d55d96a8, the code before the PR):

run	wall	user CPU
base	80.98 s	307.8 s
head	80.91 s	308.1 s
base	80.48 s	305.0 s
head	80.41 s	306.4 s

tests/bench (phpbench, all 86 cases, one iteration each): 36.4 s total before the PR, 36.7 s with it (+0.8%, within noise). The largest single difference was bug-14996.php at +7.8%. Running it by itself three times per side gave 2.83–2.97 s for base and 2.85–2.92 s for head, so that was noise too.

Part of the cost pays for itself. When a remembered call is dropped, the scope gets smaller, and each later merge, scope comparison and conditional-expression pass has fewer entries to handle.

I didn't re-measure with the turbo extension loaded (it isn't installed here). The C++ mirror follows the same logic, but its numbers are unconfirmed.

[staabm]

why do we remember a type if it does not give us more information? can't we detect this on the writing side, instead of filtering things out after the fact?

[phpstan-bot]

The full runs are in progress; I'll wait for the completion notice.

I changed the code so uninformative merged calls are never written into the merged scope. Before, the full merged scope was built and the entries were filtered out afterwards. This is pushed as bf0738afc.

Why is the type remembered at all? Neither branch writes anything useless. if (count($arr) > 2) stores count($arr) as int<3, max> in the truthy branch and int<0, 2> in the falsy branch. Both are more precise than what count($arr) resolves to by itself. The entry only becomes useless when the two branches are merged into their union, int<0, max>. So the merge is the only point where this can be detected, and it is also where the merged scope gets built.

What changed in mergeWithVariableState():

• Before: it built three scopes. First the full merged scope, then a copy without the candidate calls (to resolve them), then a third copy without the uninformative ones.
• Now: it picks the candidates first, straight from the merged tables. Candidates are certain function, method or static calls whose type differs between the branches; that selection is now a static helper, findMergedCallCandidates().
• The merged scope is built without the candidates. Each candidate is resolved on that scope by findUninformativeMergedCalls(), which compares both the PHPDoc and the native type using equals().
• Only if a candidate still carries information is a second scope built with those entries put back.
• Result: when there are no candidates, or all of them are uninformative, only one scope is built. These are the common cases.

The same restructuring is ported to the C++ mirror in turbo-ext/src/MutatingScope.cpp. Inference doesn't change: the existing bug-13750.php assertions pass unchanged.

Verification:

• make tests passes both without and with the rebuilt turbo extension loaded (23404 tests each).
• make phpstan reports no errors.
• The turbo smoke test prints ALL OK, and signature parity passes (18145 members compared).
• I didn't re-run the timing benchmark. Every case now builds the same number of scopes as before or fewer, so it can't be slower than the earlier measured 0.4% of worker CPU.

The PR title and description still describe only the original count-only narrowing. They should also cover the merge-time change.

[phpstan-bot]

Waiting for the test run to finish.

The suites and make phpstan are running in the background.

I replaced the count-specific narrowing with a general fix. The stale int<0, max> entry is no longer kept when scopes merge. The new commit b4a5c18bc is pushed to the PR branch.

Is it specific to count? No. strlen() had the same bug: after if (strlen($s) > 2) {}, a later if ($s !== '') still gave strlen($s) as int<0, max> instead of int<1, max>. Any remembered call expression can go stale like this, so a fix that only handles count()/sizeof() was the wrong level.

Why was count($expr) remembered as int<0, max>? if (count($arr) > 2) sets count($arr) to int<3, max> in one branch and int<0, 2> in the other, and both are certain. Merging the branches kept their union, int<0, max>. That adds nothing over what count($arr) resolves to by itself, but it took precedence over the narrowed $arr later on.

The change:

• When scopes merge: a remembered function, method or static call is now dropped if its merged type equals what the call resolves to without the entry. Both the PHPDoc and the native type have to match. Entries that still carry information are kept, e.g. int<1, max> after an earlier return on count($arr) === 0.
• Equality check: it uses equals() rather than isSuperTypeOf(). With the looser check, an existing test (count-type.php) changed from 1|2 to int<1, 2>: the same set of values, printed differently.
• Removed: the earlier count-only code (narrowRememberedCountsInPlace(), COUNT_FUNCTION_KEY_NAMES) is gone from both the PHP and the C++ side.
• Turbo mirror: the same logic is ported in turbo-ext/src/MutatingScope.cpp.
• Tests: bug-13750.php gains a strlen() case and a case where an informative merged count is kept.

Verification:

• On the pre-PR code, 10 assertions in bug-13750.php fail; with this change they all pass.
• With the extension loaded, the test also passes against the pre-PR PHP code, so the C++ side carries the fix on its own.
• make tests passes with and without the turbo extension (23404 tests), and make phpstan reports no errors.
• The turbo smoke test and signature parity pass. Self-analysis output is byte-identical with and without the extension.
• In a two-pair PHP-mode benchmark, the merge-time check showed no slowdown (user CPU 147.5s/148.7s vs 158.8s/151.8s).

Trade-off: a partly informative merged count, like int<0, 2>|int<5, max>, is still kept as is. A later $arr !== [] therefore won't trim it to int<1, 2>|int<5, max>. The earlier count-only narrowing did handle that case.

Not done: bin/side-by-side.php couldn't be run because the file isn't at that path. The PR title and description still describe the old approach ("Narrow remembered count()/sizeof()…") and need updating.

A background watcher I'd started to wait on the test log was stopped when it hit its time limit. It probably never exited because its process check matched itself. It doesn't affect anything: I had already read that log myself, and it showed both test runs and make phpstan passing.

[VincentLanglet]

a partly informative merged count, like int<0, 2>|int<5, max>, is still kept as is. A later $arr !== [] therefore won't trim it to int<1, 2>|int<5, max>. The earlier count-only narrowing did handle that case.

What are the different solution to handle this case ?
Is is better to have a count-only narrowing ? Is there other function/method impacted ?

Can't we fix this by adding an extra inference non-empty-array => count !== 0 ?

[phpstan-bot]

I kept both fixes. The general merge-time pruning stays, and I added back the count-only narrowing to handle the partly informative case. It's pushed as 1d6545c61.

What are the options for the partly informative case?

1. Count-only narrowing in the scope (what I chose). When an array expression is narrowed, specifyExpressionTypeInPlace() intersects any remembered count($expr)/sizeof($expr) with the new type's getArraySize(). That costs four hash lookups per narrowing, and the intersection only runs when such an entry exists. The result is exact, because array types already know their size.
2. Your suggestion, "non-empty-array ⇒ count !== 0", in TypeSpecifier. This gives the same result as option 1, but TypeSpecifier narrows arrays in many places: !== [], === [], truthiness, empty(), isset($arr[0]), array_key_exists(), recursive count(), and more. Each would need the extra rule, and every place that narrows arrays later would have to remember it. specifyExpressionTypeInPlace() is the one place all of them pass through, so the rule lives there instead.
3. A generic version for any call. When $x is narrowed, find every remembered call that takes $x as an argument, resolve it again, and intersect. That means scanning the whole expression table on every narrowing and resolving calls on a scope that is being changed in place. I rejected it as too costly and risky for the gain.

Should it be count-only? Not on its own. The pruning when scopes merge is still needed, because it fixes the fully uninformative case for any call: strlen(), method calls and so on. The count narrowing covers what pruning can't, a merged count that still carries some information. The two work together.

Other functions affected: any remembered call whose result depends on its argument's type has the same partly informative gap. For example, strlen($s) stays int<0, 2>|int<5, max> after a later $s !== ''. There is no API like getArraySize() for strings, so string length has no exact rule like this one. I left it, since that would need a new Type method.

Separate bug I found: after if (count($arr) > 2 && count($arr) < 5) { return; }, count($arr) comes out as int<1, 2>|int<5, max>, which leaves out 0. With nested ifs it is correctly int<0, 2>|int<5, max>. The base commit before this PR has the same bug, so it isn't caused by this PR. I left it out of scope, and the new test uses the nested form to avoid it.

Test: I added partlyInformativeMergedCount to bug-13750.php. After $arr !== [] it expects int<1, 2>|int<5, max>, and 0 in the else branch. Without the change it fails on both assertions; with it, it passes.

Verification:

• make tests and make phpstan pass in PHP mode (23404 tests).
• The full suite also passes with the turbo extension loaded. I built it here this time.
• With the extension loaded, the regression test passes even against PHP code without the count narrowing, so the C++ port carries the fix by itself.
• The turbo smoke test and signature parity both pass.
• One caveat: during the PHP-mode suite run I briefly swapped in the old MutatingScope.php for that turbo check. The run still passed, and the later run with the extension used the final files.

The PR title and description still describe the original count-only approach and should be updated to cover both changes.

[VincentLanglet]

I would be interested about your opinion on this/those fix @staabm

[staabm]

my feeling is that its not worth having a hack for a single function like that.

maybe we should look into why count() has this problem and maybe we need to handle count() itself differently?

[phpstan-bot]

Waiting on the build and test runs.

I removed the count-only narrowing, as you suggested. The PR now has only the general fix in how branches are merged, which works for every function and method call. Pushed as 9f750f50e.

Why count() has this problem. Nothing about it is specific to count(); it comes from how comparisons narrow types:

• if (count($arr) > 2) narrows both sides of the comparison. It narrows $arr, and it also remembers the left side, count($arr), as int<3, max> in the true branch and int<0, 2> in the false branch.
• count() needs that remembered entry because an array<mixed> type can't hold "at least 3 elements". The most $arr can become is non-empty-array.
• When the two branches merge, the remembered entry becomes int<0, max>. That tells us nothing, but it hides what a later narrowing of $arr would imply.
• strlen($s) > 2 followed by $s !== '' goes stale the same way. This is why the fix belongs where branches merge, not in count().

What's left in the PR:

• When branches merge, a remembered function, method or static call is not written into the merged scope if its merged type is exactly what the call works out to on its own. This fixes the issue's reproducer and the strlen() case.
• The C++ mirror in turbo-ext/src/MutatingScope.cpp has the same logic.

What was removed:

• narrowRememberedCountsInPlace() and COUNT_FUNCTION_KEY_NAMES, from both MutatingScope.php and MutatingScope.cpp.
• The partlyInformativeMergedCount test case.

Trade-off: a merged count that still carries some information, such as int<0, 2>|int<5, max>, is no longer trimmed by a later $arr !== []. It stays int<0, 2>|int<5, max> instead of becoming int<1, 2>|int<5, max>. The same was already true for strlen() and every other call. Fixing it properly for all calls would mean re-checking every remembered call each time one of its arguments is narrowed, which I left out.

Verification:

• make tests passed: 23404 tests, both without the turbo extension and with the rebuilt one loaded.
• make phpstan reported no errors.
• The turbo smoke test printed ALL OK, and signature parity passed.
• I didn't re-check that bug-13750.php fails without the remaining fix. The commit I pushed only removes code, and the earlier rounds checked that.

The PR title and description still describe the count-only approach that's now gone. They need rewriting to cover only the merge-time change.

[VincentLanglet]

I feel like the general fix is ok for now, and it avoids keeping in memory useless expression types.

[staabm]

this PR has no CPP components. so lets wait until 2.3.x is green before merging