Skip to content

Commit 27771bf

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: that is how NodeScopeResolver applies it at the call site, writing the out type - or the declared type when there is no @param-out - back to each argument individually. Inside the body the variable holds the packed array of those arguments, so comparing the packed array against the out type reported the array as the wrong type and, in the too-wide rules, claimed the parameter never gets the values it does get. The element type is the side to compare, both in the level-dependent filtering and in the comparison itself. Rebinding the packed variable to something that is no longer an array leaves nothing to compare: the references it held are discarded, so PHP writes nothing back to any caller. Closes phpstan/phpstan#15066 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent dda82d9 commit 27771bf

9 files changed

Lines changed: 281 additions & 1 deletion

‎src/Rules/TooWideTypehints/TooWideParameterOutTypeCheck.php‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
use PHPStan\Node\ReturnStatement;
1010
use PHPStan\Reflection\ExtendedParameterReflection;
1111
use PHPStan\Rules\IdentifierRuleError;
12+
use PHPStan\Rules\VariadicByRefParameterOutType;
1213
use function lcfirst;
1314
use function sprintf;
1415

@@ -94,6 +95,13 @@ private function processSingleParameter(
9495
$variableExpr = new Variable($parameter->getName());
9596
$variableType = $scope->getType($variableExpr);
9697

98+
if ($parameter->isVariadic()) {
99+
$variableType = VariadicByRefParameterOutType::elementType($variableType);
100+
if ($variableType === null) {
101+
return [];
102+
}
103+
}
104+
97105
return $this->tooWideTypeCheck->checkParameterOutType(
98106
$outType,
99107
$variableType,

‎src/Rules/Variables/ParameterOutTypeCheck.php‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
use PHPStan\Rules\IdentifierRuleError;
1212
use PHPStan\Rules\RuleErrorBuilder;
1313
use PHPStan\Rules\RuleLevelHelper;
14+
use PHPStan\Rules\VariadicByRefParameterOutType;
1415
use PHPStan\Type\ErrorType;
1516
use PHPStan\Type\Type;
1617
use PHPStan\Type\VerbosityLevel;
@@ -22,6 +23,9 @@
2223
* The promise is either an explicit `@param-out` or, in its absence, the parameter's own type.
2324
* Which one it is only shows in the error message, so callers report it via $isParamOutType.
2425
*
26+
* For a variadic parameter the promise describes a single argument while the variable holds the
27+
* packed array of them, so the two sides are reconciled through VariadicByRefParameterOutType.
28+
*
2529
* @internal
2630
*/
2731
#[AutowiredService]
@@ -47,17 +51,35 @@ public function check(
4751
bool $isParamOutType,
4852
): array
4953
{
54+
$isVariadic = $parameter->isVariadic();
55+
5056
$typeResult = $this->ruleLevelHelper->findTypeToCheck(
5157
$scope,
5258
$checkedExpr,
5359
'',
54-
static fn (Type $type): bool => $outType->isSuperTypeOf($type)->yes(),
60+
static function (Type $type) use ($outType, $isVariadic): bool {
61+
if ($isVariadic) {
62+
$type = VariadicByRefParameterOutType::elementType($type);
63+
if ($type === null) {
64+
return false;
65+
}
66+
}
67+
68+
return $outType->isSuperTypeOf($type)->yes();
69+
},
5570
);
5671
if ($typeResult->getType() instanceof ErrorType) {
5772
return $typeResult->getUnknownClassErrors();
5873
}
5974

6075
$assignedExprType = $scope->getType($checkedExpr);
76+
if ($isVariadic) {
77+
$assignedExprType = VariadicByRefParameterOutType::elementType($assignedExprType);
78+
if ($assignedExprType === null) {
79+
return [];
80+
}
81+
}
82+
6183
if ($outType->isSuperTypeOf($assignedExprType)->yes()) {
6284
return [];
6385
}

‎src/Rules/VariadicByRefParameterOutType.php‎

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
<?php declare(strict_types = 1);
2+
3+
namespace PHPStan\Rules;
4+
5+
use PHPStan\Type\Type;
6+
7+
/**
8+
* The out type of a variadic by-ref parameter describes a single argument: that is how it is applied
9+
* at the call site, where NodeScopeResolver writes the out type - or the declared type when there is
10+
* no @param-out - back to each argument individually. Inside the body the variable holds the packed
11+
* array of those arguments instead, so the element type is the side to compare against the out type.
12+
*
13+
* @internal
14+
*/
15+
final class VariadicByRefParameterOutType
16+
{
17+
18+
/**
19+
* Returns the type to compare against the out type, or null when there is nothing to compare.
20+
*
21+
* Null means the variable no longer holds an array. Rebinding the packed variable discards the
22+
* references it held, so PHP writes nothing back to any caller and no out value is left to check.
23+
* An array is still compared, because a write through an offset - `$refs[0] = ...`, which does
24+
* reach the caller - leaves the variable as an array too, and the two are indistinguishable here.
25+
*/
26+
public static function elementType(Type $packedType): ?Type
27+
{
28+
if (!$packedType->isArray()->yes()) {
29+
return null;
30+
}
31+
32+
return $packedType->getIterableValueType();
33+
}
34+
35+
}

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

Lines changed: 19 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,22 @@ 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+
// rebinding the packed variable to a non-array is silent, to an array is not - see the fixture
64+
[
65+
'Function Bug15066\\variadicRebindOnlyString() never assigns null to &$refs so it can be removed from the by-ref type.',
66+
62,
67+
'You can narrow the parameter out type with @param-out PHPDoc tag.',
68+
],
69+
]);
70+
}
71+
5372
}

‎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: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,65 @@
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+
}
53+
54+
// Rebinding the packed variable discards the references it held, so PHP writes nothing back and
55+
// there is no out value left to check. An array is still compared, because a write through an
56+
// offset reaches the caller and leaves the variable as an array too.
57+
function variadicRebindNonArray(string|null &...$refs): void
58+
{
59+
$refs = 42;
60+
}
61+
62+
function variadicRebindOnlyString(string|null &...$refs): void
63+
{
64+
$refs = ['ok'];
65+
}

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

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,4 +117,27 @@ public function testCatchVariable(): void
117117
]);
118118
}
119119

120+
#[RequiresPhp('>= 8.0.0')]
121+
public function testBug15066(): void
122+
{
123+
$this->analyse([__DIR__ . '/data/bug-15066.php'], [
124+
[
125+
'Parameter &$refs by-ref type of function Bug15066Variables\\variadicWrongType() expects string|null, int|string|null given.',
126+
22,
127+
'You can change the parameter out type with @param-out PHPDoc tag.',
128+
],
129+
[
130+
'Parameter &$refs by-ref type of method Bug15066Variables\\Foo::variadicWrongType() expects string|null, int|string|null given.',
131+
52,
132+
'You can change the parameter out type with @param-out PHPDoc tag.',
133+
],
134+
// rebinding the packed variable to a non-array is silent, to an array is not - see the fixture
135+
[
136+
'Parameter &$refs by-ref type of function Bug15066Variables\\variadicRebindWrongArray() expects string|null, int given.',
137+
69,
138+
'You can change the parameter out type with @param-out PHPDoc tag.',
139+
],
140+
]);
141+
}
142+
120143
}

‎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>
@@ -74,4 +75,15 @@ public function testBug12330(): void
7475
$this->analyse([__DIR__ . '/data/bug-12330.php'], []);
7576
}
7677

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

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

Lines changed: 83 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,83 @@
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+
}
57+
58+
// Rebinding the packed variable discards the references it held, so PHP writes nothing back to any
59+
// caller. Nothing is reported for a non-array, because no out value is left to check. An array is
60+
// still reported: a write through an offset reaches the caller and leaves the variable as an array
61+
// too, so the two cannot be told apart here, and the offset write is the case that matters.
62+
function variadicRebindNonArray(string|null &...$refs): void
63+
{
64+
$refs = 42;
65+
}
66+
67+
function variadicRebindWrongArray(string|null &...$refs): void
68+
{
69+
$refs = [42];
70+
}
71+
72+
function variadicRebindOkArray(string|null &...$refs): void
73+
{
74+
$refs = ['ok'];
75+
}
76+
77+
// The packed variable may end up only maybe holding an array. There is then no element type to
78+
// speak of, so the comparison is skipped rather than run against a nonexistent one.
79+
function variadicRebindMaybeArray(string|null &...$refs): void
80+
{
81+
$refs = rand(0, 1) === 1 ? [42] : null;
82+
}
83+

0 commit comments

Comments
 (0)