Compare the element type of a variadic by-ref parameter against its out type

[SanderMuller]

Rebased onto #6238, so this is now only the functional change: one commit, and the two Variables rules are untouched because the comparison they share already lives in ParameterOutTypeCheck.

The out type of a variadic by-ref parameter describes a single argument. That is how it is applied at the call site: NodeScopeResolver writes getOutType(), or the declared type when there is no @param-out, back to each argument individually. tests/PHPStan/Analyser/data/param-out.php already asserts it, for noParamOutVariadic(string &...$s) called with two arguments.

Inside the body, though, the variable holds the packed array of those arguments. Four rules compare the two directly, so they were reading a packed array as if it were one argument - through the two checks they share:

• TooWideParameterOutTypeCheck, behind TooWideFunctionParameterOutTypeRule and TooWideMethodParameterOutTypeRule
• ParameterOutTypeCheck, behind ParameterOutAssignedTypeRule and ParameterOutExecutionEndTypeRule

Nothing could satisfy that comparison, so every variadic by-ref parameter with a union type was reported, whatever the body did with it:

function variadicByRef(string|null &...$refs): void
{
	foreach ($refs as &$ref) {
		$ref = $ref === null ? null : trim($ref);
	}
}

Function variadicByRef() never assigns null to &$refs so it can be removed from the by-ref type.
Function variadicByRef() never assigns string to &$refs so it can be removed from the by-ref type.

The same function without the ... is silent, which is what localises it. An empty body was reported too, so the body never mattered.

Neither spelling of @param-out was a way out. @param-out string|null hits the same mismatch, and @param-out array<int, string|null> fails on the key type, because the packed array is array<int<0, max>|string, ...> and no hand written array<int, ...> accepts that.

The change

Compare the element type when the parameter is variadic.

The RuleLevelHelper::findTypeToCheck() callback deliberately does not get the same treatment. It only decides which members of the observed type are kept, and these checks use that result for nothing but its ErrorType test, which the callback cannot influence - filtering everything out falls back to the unfiltered type. An earlier revision unpacked the element type in the callback too; it changed no output at levels 3, 5, 7, 8 or 9 on nullable, union, mixed and object-typed variadics, so it is gone.

Genuinely too wide variadics are still reported, and errors that used to name the packed array now name the element. On tests/PHPStan/Analyser/data/param-out.php the same 14 errors are reported before and after, four of them changing from expects int, array<int|string, mixed> given to expects int, mixed given.

One behaviour change worth flagging

Rebinding the variable itself to something that is not an array, as in $refs = 42;, is now silent where it previously produced three errors on that function. Rebinding to an array of the wrong element type, $refs = [42];, is still reported, with the message naming the element.

That asymmetry is deliberate but it is not a difference in what PHP does. Rebinding the packed variable discards the references it held, so nothing is written back to any caller in either case - checked against PHP, where none of $refs = 42, $refs = [], $refs = [42] or even $refs = $refs updates any argument, while $refs[0] = 42 and a by-ref foreach do. The reason arrays stay reported is that a write through an offset also leaves the variable holding an array, so by type alone the two are indistinguishable here, and the offset write is the case worth reporting. Bailing out on a non-array is the part that is unambiguous.

Verification

• A regression test per rule: TooWideFunctionParameterOutTypeRuleTest, TooWideMethodParameterOutTypeRuleTest, ParameterOutAssignedTypeRuleTest, ParameterOutExecutionEndTypeRuleTest. Each fails without the change, each with the symptom from the issue, and each fixture also carries a true positive so the tests cannot be satisfied by skipping variadics.
• Both fixtures also carry the three rebinding shapes, so the behaviour described above is pinned rather than only documented: the non-array rebinding stays silent and the array one stays reported.
• The element-type unpacking lives in one place, VariadicByRefParameterOutType::elementType(), and with Extract the shared parameter-out type comparison into ParameterOutTypeCheck #6238 in it is applied in two checks rather than at each of the five comparison sites it started at.
• Full suite green, self analysis clean, phpcs clean on the touched files.
• Analysing vendor/symfony, vendor/nikic and vendor/react gives an identical set of findings before and after. That is a no regression signal only, since these rules produce no by-ref findings on that code at all. param-out.php above is the run that actually exercises the change.

Performance: one isVariadic() check per by-ref parameter in level 3 rules, so nothing on a hot path.

One CI note: Mutation Testing flags an escaped IsSuperTypeOfCalleeAndArgumentMutator on the findTypeToCheck() callback line, and I do not think it is killable. ParameterOutTypeCheck uses the result of findTypeToCheck() only as instanceof ErrorType, and the callback cannot influence that - it is consulted only to strip null (!checkNullables) or to filter union members (!checkUnionTypes), and neither can turn the result into an ErrorType. Swapping the operands leaves the whole suite green (21329 tests) and produces byte-identical output on a probe of nullable, union, benevolent-union and object out types at levels 3, 5, 7 and 8. The same escape applies to the callback as it stood before this PR; it only surfaced now because the line is part of a diff. Happy to follow up if you would rather these rules compared the filtered type instead - that would make the callback meaningful, but it changes what is reported below level 8.

Closes phpstan/phpstan#15066

[staabm]

repeating this pattern in so many places make me feel that there might be a better place in e.g. NodeScopeResolver or MutatingScope to fix this in a more generic fashion

[SanderMuller]

Fair, and it pointed at real duplication - just not where I expected. Folded in 446245d.

ParameterOutAssignedTypeRule and ParameterOutExecutionEndTypeRule were asking the same question of a different expression: the same findTypeToCheck() predicate, the same comparison, the same message, the same identifier - the execution-end rule is exactly the @param-out case of the other. That duplication predates this PR, and it is why the variadic handling needed repeating: two rules times the predicate and the comparison. There is now a ParameterOutTypeCheck, next to the TooWideParameterOutTypeCheck that already serves the other pair the same way. Neither rule mentions variadics any more, and both lose their RuleLevelHelper dependency. The two rules together shrink by ~90 lines.

I did look at NodeScopeResolver and MutatingScope first, and I do not think either can carry it:

• NodeScopeResolver is already on the correct side. At the call site it writes getOutType() back to each argument individually (processVirtualAssign per arg, reusing array_last($writebackParameters) for the variadic tail), which is the per-argument meaning the tag has. Nothing there is wrong to fix.
• MutatingScope cannot be it either: inside the body the variable really does hold the packed array, and getType($refs) returning anything else would be a lie that every other rule would then have to undo.

So the two meanings are both correct and the mismatch only exists where they are compared, which is the rules. The bridge has to live there.

The one alternative that would have removed the transformation entirely is to compare packed-against-packed - build array<int, T> from the out type once instead of unpacking the observed type. I did not do that because the error message then names the packed array, which is the form the issue was reported over (expects int, array<int|string, mixed> given); unpacking keeps the message on the element, expects string|null, int given. Happy to swap if you would rather have the simpler comparison and accept the message.

Full suite green, self analysis clean, phpcs clean, and both rules' complete test classes pass untouched apart from constructing the check - so the extraction is behaviour preserving rather than just looking like it.

[staabm]

nice. please create a separate refactoring PR which introduces ParameterOutTypeCheck without any functional changes.

[SanderMuller]

Done: #6238. It extracts the check from 2.2.x as it stands, so it carries no variadic handling - both rule tests keep their expectations, the only diff under tests/ is the new ParameterOutTypeCheck(...) wrapper. Full suite green (21325 tests), self-analysis clean.

Once that is merged I will rebase this PR on top of it, so this one is left with just the variadic element-type comparison.

On your point above about NodeScopeResolver/MutatingScope being the better place: I dug into that in this reply - NodeScopeResolver already writes the out type per argument, and MutatingScope cannot represent the packed variable as anything other than the array it is, so the two sides only meet in the rules. With #6238 the reconciliation lives in two checks instead of the five sites it started at.

[SanderMuller]

Rebased onto #6238 now that it is in — this PR is down to one commit, +281/-1, and the two Variables rules are untouched: the comparison they share already lives in ParameterOutTypeCheck, so the variadic reconciliation goes in there and in TooWideParameterOutTypeCheck.

All four regression tests still fail without the change (verified by reverting just the two checks) and pass with it. Full suite 21329 tests, self-analysis clean, phpcs clean.

One thing from #6238's CI that carries over here: Mutation Testing reports an escaped IsSuperTypeOfCalleeAndArgumentMutator on the findTypeToCheck() callback line. I looked into it and I believe it is an equivalent mutant rather than a coverage gap — ParameterOutTypeCheck consumes the result only as instanceof ErrorType, and the callback is consulted only to strip null (!checkNullables) or filter union members (!checkUnionTypes), neither of which can produce an ErrorType. With the operands swapped the whole suite stays green and a probe of nullable, union, benevolent-union and object out types gives byte-identical output at levels 3, 5, 7 and 8. It escaped before this work too; it only shows up now that the line is part of a diff.

If you would rather make it meaningful, the honest fix is for these rules to compare $typeResult->getType() instead of $scope->getType($checkedExpr) — but that changes what gets reported below level 8, so I would not fold it in here.

[staabm]

Does the newly added behavior match psalm?

[SanderMuller]

The model it is built on matches Psalm exactly. The reporting it adds has no Psalm counterpart, because Psalm does not check variadic by-ref bodies at all.

Measured with Psalm 6.16.1 at errorLevel="1" on PHP 8.5, types read with @psalm-trace:

probe	Psalm	this PR
call site, @param-out int on mixed &...$refs, two args	$a: int, $b: int	same - NodeScopeResolver already wrote it per argument
call site, @param-out array<int, string> on a variadic	$a: array<int, string> - per argument, whatever the shape	same
call site, no @param-out, string|null &...$refs	each argument null|string, and each argument validated separately (Argument 1 ..., Argument 2 ...)	same
inside the body	$refs: array<array-key, mixed> - the packed array	packed array

So "the out type describes a single argument, the variable inside the body holds the packed array" is Psalm's reading too, in both spellings and regardless of the out type's shape. That is the part this PR depends on.

On the body side Psalm has the check but not for variadics. The minimal pair, same violation twice:

function declaredNonVariadic(string &$ref): void { $ref = 42; }       // ReferenceConstraintViolation
function declaredVariadicElement(string &...$refs): void { $refs[0] = 42; }  // nothing
function declaredVariadicRebind(string &...$refs): void { $refs = 42; }      // nothing

@param-out string with $refs = [42] or $refs[0] = 42 is silent as well. So Psalm is not deciding that these are fine, it is not looking - which means it cannot confirm or contradict the two choices that are actually new here: comparing the element type, and staying silent when the packed variable is rebound to a non-array. On both, PHPStan ends up strictly stricter than Psalm.

Psalm has no analogue of the too-wide by-ref rule either - the closest thing is UnusedParam, which is a different question.

Harness is four fixtures plus a psalm.xml; happy to paste it if you want to re-run it.

[staabm]

why do we need this part? reverting lines 60-69 does not make a test fail

[SanderMuller]

You are right, it is dead code - removed in 6766421.

The callback only decides which members findTypeToCheck() keeps, and this check uses that result for nothing except instanceof ErrorType, which the callback cannot produce: when the filter drops every member, the union branch falls through and returns the unfiltered type. So unpacking the element type in there could never change an outcome.

Checked before deleting rather than just trusting the test suite, since the callback is only consulted when checkNullables or checkUnionTypes are off and those are off below levels 8 and 7: a fixture with nullable, union, mixed and object-typed variadic by-ref parameters (plus @param-out and declared-type variants) gives byte-identical output with and without it at levels 3, 5, 7, 8 and 9.

Same reason the IsSuperTypeOfCalleeAndArgumentMutator escapes on that line, which I had flagged earlier in this PR - I should have followed my own observation to the conclusion you just drew. I also fixed the PR description, which claimed the opposite.

Suite 21329, self-analysis and phpcs clean; the four regression tests still fail without the fix.