Skip to content

Commit 704ce58

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 704ce58

9 files changed

Lines changed: 224 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: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use PHPStan\Rules\Properties\PropertyReflectionFinder;
66
use PHPStan\Rules\Rule as TRule;
77
use PHPStan\Testing\RuleTestCase;
8+
use PHPUnit\Framework\Attributes\RequiresPhp;
89

910
/**
1011
* @extends RuleTestCase<TooWideFunctionParameterOutTypeRule>
@@ -50,4 +51,16 @@ public function testNestedTooWideType(): void
5051
]);
5152
}
5253

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

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

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use PHPStan\Rules\Properties\PropertyReflectionFinder;
66
use PHPStan\Rules\Rule as TRule;
77
use PHPStan\Testing\RuleTestCase;
8+
use PHPUnit\Framework\Attributes\RequiresPhp;
89

910
/**
1011
* @extends RuleTestCase<TooWideMethodParameterOutTypeRule>
@@ -144,4 +145,16 @@ public function testNestedTooWideType(): void
144145
]);
145146
}
146147

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

‎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: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -115,4 +115,21 @@ public function testCatchVariable(): void
115115
]);
116116
}
117117

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

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
use PHPStan\Rules\Rule;
66
use PHPStan\Rules\RuleLevelHelper;
77
use PHPStan\Testing\RuleTestCase;
8+
use PHPUnit\Framework\Attributes\RequiresPhp;
89

910
/**
1011
* @extends RuleTestCase<ParameterOutExecutionEndTypeRule>
@@ -72,4 +73,15 @@ public function testBug12330(): void
7273
$this->analyse([__DIR__ . '/data/bug-12330.php'], []);
7374
}
7475

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

‎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)