Skip to content

Commit d2e4efd

Browse files
SanderMullerclaude
SanderMuller
and
claude
committed
Compare the element type of a variadic by-ref parameter against its out type
The out type of a variadic by-ref parameter describes a single argument: at the call site NodeScopeResolver writes `getOutType()` (or the declared type) back to each argument individually, which `tests/PHPStan/Analyser/data/param-out.php` already asserts for `noParamOutVariadic(string &...$s)`. Inside the body, though, the variable holds the packed array of those arguments, and the three rules that compare the two read that packed array as if it were one argument. Nothing could satisfy the comparison, so every variadic by-ref parameter with a union type was reported, including one with an empty body: function variadicByRef(string|null &...$refs): void { foreach ($refs as &$ref) { $ref = $ref === null ? null : trim($ref); } } Function variadicByRef() never assigns null to &$refs [...] Function variadicByRef() never assigns string to &$refs [...] Neither `@param-out string|null` nor `@param-out array<int, string|null>` worked around it, the first because it hit the same mismatch and the second because the packed array's key type is wider than any `array<int, ...>` written by hand. Compare the element type for a variadic instead. In the two rules that also feed the observed type through `RuleLevelHelper::findTypeToCheck()`, the callback gets the same treatment, so the level-dependent filtering does not keep comparing the packed array. The rules keep reporting a genuinely too-wide variadic, and errors on 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 which change from "expects int, array<int|string, mixed> given" to "expects int, mixed given". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 6c642f1 commit d2e4efd

9 files changed

Lines changed: 217 additions & 2 deletions

‎src/Rules/TooWideTypehints/TooWideParameterOutTypeCheck.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -94,6 +94,17 @@ private function processSingleParameter(
9494
$variableExpr = new Variable($parameter->getName());
9595
$variableType = $scope->getType($variableExpr);
9696

97+
if ($parameter->isVariadic()) {
98+
// The out type of a variadic by-ref parameter describes a single argument - that is what
99+
// gets written back to each of them at the call site - while the variable inside the body
100+
// holds the packed array of them. So the element type is the side to compare.
101+
if (!$variableType->isArray()->yes()) {
102+
return [];
103+
}
104+
105+
$variableType = $variableType->getIterableValueType();
106+
}
107+
97108
return $this->tooWideTypeCheck->checkParameterOutType(
98109
$outType,
99110
$variableType,

‎src/Rules/Variables/ParameterOutAssignedTypeRule.php‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -78,18 +78,42 @@ public function processNode(Node $node, Scope $scope): array
7878

7979
$outType = TypeUtils::resolveLateResolvableTypes($outType);
8080

81+
// The out type of a variadic by-ref parameter describes a single argument - that is what gets
82+
// written back to each of them at the call site - while the variable inside the body holds the
83+
// packed array of them. So the element type is the side to compare, both here and in the
84+
// level-dependent filtering below.
85+
$isVariadic = $foundParameter->isVariadic();
86+
8187
$typeResult = $this->ruleLevelHelper->findTypeToCheck(
8288
$scope,
8389
$node->getAssignedExpr(),
8490
'',
85-
static fn (Type $type): bool => $outType->isSuperTypeOf($type)->yes(),
91+
static function (Type $type) use ($outType, $isVariadic): bool {
92+
if ($isVariadic) {
93+
if (!$type->isArray()->yes()) {
94+
return false;
95+
}
96+
97+
$type = $type->getIterableValueType();
98+
}
99+
100+
return $outType->isSuperTypeOf($type)->yes();
101+
},
86102
);
87103
$type = $typeResult->getType();
88104
if ($type instanceof ErrorType) {
89105
return $typeResult->getUnknownClassErrors();
90106
}
91107

92108
$assignedExprType = $scope->getType($node->getAssignedExpr());
109+
if ($isVariadic) {
110+
if (!$assignedExprType->isArray()->yes()) {
111+
return [];
112+
}
113+
114+
$assignedExprType = $assignedExprType->getIterableValueType();
115+
}
116+
93117
if ($outType->isSuperTypeOf($assignedExprType)->yes()) {
94118
return [];
95119
}

‎src/Rules/Variables/ParameterOutExecutionEndTypeRule.php‎

Lines changed: 25 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,19 +94,43 @@ private function processSingleParameter(
9494

9595
$outType = TypeUtils::resolveLateResolvableTypes($outType);
9696

97+
// The out type of a variadic by-ref parameter describes a single argument - that is what gets
98+
// written back to each of them at the call site - while the variable inside the body holds the
99+
// packed array of them. So the element type is the side to compare, both here and in the
100+
// level-dependent filtering below.
101+
$isVariadic = $parameter->isVariadic();
102+
97103
$variableExpr = new Node\Expr\Variable($parameter->getName());
98104
$typeResult = $this->ruleLevelHelper->findTypeToCheck(
99105
$scope,
100106
$variableExpr,
101107
'',
102-
static fn (Type $type): bool => $outType->isSuperTypeOf($type)->yes(),
108+
static function (Type $type) use ($outType, $isVariadic): bool {
109+
if ($isVariadic) {
110+
if (!$type->isArray()->yes()) {
111+
return false;
112+
}
113+
114+
$type = $type->getIterableValueType();
115+
}
116+
117+
return $outType->isSuperTypeOf($type)->yes();
118+
},
103119
);
104120
$type = $typeResult->getType();
105121
if ($type instanceof ErrorType) {
106122
return $typeResult->getUnknownClassErrors();
107123
}
108124

109125
$assignedExprType = $scope->getType($variableExpr);
126+
if ($isVariadic) {
127+
if (!$assignedExprType->isArray()->yes()) {
128+
return [];
129+
}
130+
131+
$assignedExprType = $assignedExprType->getIterableValueType();
132+
}
133+
110134
if ($outType->isSuperTypeOf($assignedExprType)->yes()) {
111135
return [];
112136
}

‎tests/PHPStan/Rules/TooWideTypehints/TooWideFunctionParameterOutTypeRuleTest.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,4 +50,15 @@ public function testNestedTooWideType(): void
5050
]);
5151
}
5252

53+
public function testBug15066(): void
54+
{
55+
$this->analyse([__DIR__ . '/data/bug-15066.php'], [
56+
[
57+
'Function Bug15066\\variadicNeverNull() never assigns null to &$refs so it can be removed from the by-ref type.',
58+
24,
59+
'You can narrow the parameter out type with @param-out PHPDoc tag.',
60+
],
61+
]);
62+
}
63+
5364
}

‎tests/PHPStan/Rules/TooWideTypehints/TooWideMethodParameterOutTypeRuleTest.php‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -144,4 +144,15 @@ public function testNestedTooWideType(): void
144144
]);
145145
}
146146

147+
public function testBug15066(): void
148+
{
149+
$this->analyse([__DIR__ . '/data/bug-15066.php'], [
150+
[
151+
'Method Bug15066\\Foo::variadicNeverNull() never assigns null to &$refs so it can be removed from the by-ref type.',
152+
45,
153+
'You can narrow the parameter out type with @param-out PHPDoc tag.',
154+
],
155+
]);
156+
}
157+
147158
}

‎tests/PHPStan/Rules/TooWideTypehints/data/bug-15066.php‎

Lines changed: 52 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,52 @@
1+
<?php declare(strict_types = 1); // lint >= 8.0
2+
3+
namespace Bug15066;
4+
5+
function variadicByRef(string|null &...$refs): void
6+
{
7+
foreach ($refs as &$ref) {
8+
$ref = $ref === null ? null : trim($ref);
9+
}
10+
}
11+
12+
function singleByRef(string|null &$ref): void
13+
{
14+
$ref = $ref === null ? null : trim($ref);
15+
}
16+
17+
function variadicByIndex(string|null &...$refs): void
18+
{
19+
foreach ($refs as $key => $value) {
20+
$refs[$key] = $value === null ? null : trim($value);
21+
}
22+
}
23+
24+
function variadicNeverNull(string|null &...$refs): void
25+
{
26+
foreach ($refs as $key => $value) {
27+
$refs[$key] = 'foo';
28+
}
29+
}
30+
31+
function variadicNeverWritten(string|null &...$refs): void
32+
{
33+
}
34+
35+
class Foo
36+
{
37+
38+
public function variadicByIndex(string|null &...$refs): void
39+
{
40+
foreach ($refs as $key => $value) {
41+
$refs[$key] = $value === null ? null : trim($value);
42+
}
43+
}
44+
45+
public function variadicNeverNull(string|null &...$refs): void
46+
{
47+
foreach ($refs as $key => $value) {
48+
$refs[$key] = 'foo';
49+
}
50+
}
51+
52+
}

‎tests/PHPStan/Rules/Variables/ParameterOutAssignedTypeRuleTest.php‎

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,4 +115,20 @@ public function testCatchVariable(): void
115115
]);
116116
}
117117

118+
public function testBug15066(): void
119+
{
120+
$this->analyse([__DIR__ . '/data/bug-15066.php'], [
121+
[
122+
'Parameter &$refs by-ref type of function Bug15066Variables\\variadicWrongType() expects string|null, int|string|null given.',
123+
22,
124+
'You can change the parameter out type with @param-out PHPDoc tag.',
125+
],
126+
[
127+
'Parameter &$refs by-ref type of method Bug15066Variables\\Foo::variadicWrongType() expects string|null, int|string|null given.',
128+
52,
129+
'You can change the parameter out type with @param-out PHPDoc tag.',
130+
],
131+
]);
132+
}
133+
118134
}

‎tests/PHPStan/Rules/Variables/ParameterOutExecutionEndTypeRuleTest.php‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -72,4 +72,14 @@ public function testBug12330(): void
7272
$this->analyse([__DIR__ . '/data/bug-12330.php'], []);
7373
}
7474

75+
public function testBug15066(): void
76+
{
77+
$this->analyse([__DIR__ . '/data/bug-15066.php'], [
78+
[
79+
'Parameter &$refs @param-out type of function Bug15066Variables\\variadicParamOutNeverWritten() expects string, string|null given.',
80+
35,
81+
],
82+
]);
83+
}
84+
7585
}

‎tests/PHPStan/Rules/Variables/data/bug-15066.php‎

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
<?php declare(strict_types = 1); // lint >= 8.0
2+
3+
namespace Bug15066Variables;
4+
5+
function variadicByRef(string|null &...$refs): void
6+
{
7+
foreach ($refs as &$ref) {
8+
$ref = $ref === null ? null : trim($ref);
9+
}
10+
}
11+
12+
function variadicByIndex(string|null &...$refs): void
13+
{
14+
foreach ($refs as $key => $value) {
15+
$refs[$key] = $value === null ? null : trim($value);
16+
}
17+
}
18+
19+
function variadicWrongType(string|null &...$refs): void
20+
{
21+
foreach ($refs as $key => $value) {
22+
$refs[$key] = 42;
23+
}
24+
}
25+
26+
/** @param-out string|null $refs */
27+
function variadicParamOut(string|null &...$refs): void
28+
{
29+
foreach ($refs as $key => $value) {
30+
$refs[$key] = $value === null ? null : trim($value);
31+
}
32+
}
33+
34+
/** @param-out string $refs */
35+
function variadicParamOutNeverWritten(string|null &...$refs): void
36+
{
37+
}
38+
39+
class Foo
40+
{
41+
42+
public function variadicByIndex(string|null &...$refs): void
43+
{
44+
foreach ($refs as $key => $value) {
45+
$refs[$key] = $value === null ? null : trim($value);
46+
}
47+
}
48+
49+
public function variadicWrongType(string|null &...$refs): void
50+
{
51+
foreach ($refs as $key => $value) {
52+
$refs[$key] = 42;
53+
}
54+
}
55+
56+
}

0 commit comments

Comments
 (0)