diff --git a/src/Rules/TooWideTypehints/TooWideParameterOutTypeCheck.php b/src/Rules/TooWideTypehints/TooWideParameterOutTypeCheck.php index 4fd41dc37b1..25c1a1ff39d 100644 --- a/src/Rules/TooWideTypehints/TooWideParameterOutTypeCheck.php +++ b/src/Rules/TooWideTypehints/TooWideParameterOutTypeCheck.php @@ -9,6 +9,7 @@ use PHPStan\Node\ReturnStatement; use PHPStan\Reflection\ExtendedParameterReflection; use PHPStan\Rules\IdentifierRuleError; +use PHPStan\Rules\VariadicByRefParameterOutType; use function lcfirst; use function sprintf; @@ -94,6 +95,13 @@ private function processSingleParameter( $variableExpr = new Variable($parameter->getName()); $variableType = $scope->getType($variableExpr); + if ($parameter->isVariadic()) { + $variableType = VariadicByRefParameterOutType::elementType($variableType); + if ($variableType === null) { + return []; + } + } + return $this->tooWideTypeCheck->checkParameterOutType( $outType, $variableType, diff --git a/src/Rules/Variables/ParameterOutTypeCheck.php b/src/Rules/Variables/ParameterOutTypeCheck.php index b2ddbf64f02..6d30d3fe196 100644 --- a/src/Rules/Variables/ParameterOutTypeCheck.php +++ b/src/Rules/Variables/ParameterOutTypeCheck.php @@ -11,6 +11,7 @@ use PHPStan\Rules\IdentifierRuleError; use PHPStan\Rules\RuleErrorBuilder; use PHPStan\Rules\RuleLevelHelper; +use PHPStan\Rules\VariadicByRefParameterOutType; use PHPStan\Type\ErrorType; use PHPStan\Type\Type; use PHPStan\Type\VerbosityLevel; @@ -22,6 +23,9 @@ * The promise is either an explicit `@param-out` or, in its absence, the parameter's own type. * Which one it is only shows in the error message, so callers report it via $isParamOutType. * + * For a variadic parameter the promise describes a single argument while the variable holds the + * packed array of them, so the two sides are reconciled through VariadicByRefParameterOutType. + * * @internal */ #[AutowiredService] @@ -47,6 +51,8 @@ public function check( bool $isParamOutType, ): array { + $isVariadic = $parameter->isVariadic(); + $typeResult = $this->ruleLevelHelper->findTypeToCheck( $scope, $checkedExpr, @@ -58,6 +64,13 @@ public function check( } $assignedExprType = $scope->getType($checkedExpr); + if ($isVariadic) { + $assignedExprType = VariadicByRefParameterOutType::elementType($assignedExprType); + if ($assignedExprType === null) { + return []; + } + } + if ($outType->isSuperTypeOf($assignedExprType)->yes()) { return []; } diff --git a/src/Rules/VariadicByRefParameterOutType.php b/src/Rules/VariadicByRefParameterOutType.php new file mode 100644 index 00000000000..7fb0020ee32 --- /dev/null +++ b/src/Rules/VariadicByRefParameterOutType.php @@ -0,0 +1,35 @@ +isArray()->yes()) { + return null; + } + + return $packedType->getIterableValueType(); + } + +} diff --git a/tests/PHPStan/Rules/TooWideTypehints/TooWideFunctionParameterOutTypeRuleTest.php b/tests/PHPStan/Rules/TooWideTypehints/TooWideFunctionParameterOutTypeRuleTest.php index ddc91cbc001..591d77f09ee 100644 --- a/tests/PHPStan/Rules/TooWideTypehints/TooWideFunctionParameterOutTypeRuleTest.php +++ b/tests/PHPStan/Rules/TooWideTypehints/TooWideFunctionParameterOutTypeRuleTest.php @@ -5,6 +5,7 @@ use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule as TRule; use PHPStan\Testing\RuleTestCase; +use PHPUnit\Framework\Attributes\RequiresPhp; /** * @extends RuleTestCase @@ -50,4 +51,22 @@ public function testNestedTooWideType(): void ]); } + #[RequiresPhp('>= 8.0.0')] + public function testBug15066(): void + { + $this->analyse([__DIR__ . '/data/bug-15066.php'], [ + [ + 'Function Bug15066\\variadicNeverNull() never assigns null to &$refs so it can be removed from the by-ref type.', + 24, + 'You can narrow the parameter out type with @param-out PHPDoc tag.', + ], + // rebinding the packed variable to a non-array is silent, to an array is not - see the fixture + [ + 'Function Bug15066\\variadicRebindOnlyString() never assigns null to &$refs so it can be removed from the by-ref type.', + 62, + 'You can narrow the parameter out type with @param-out PHPDoc tag.', + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/TooWideTypehints/TooWideMethodParameterOutTypeRuleTest.php b/tests/PHPStan/Rules/TooWideTypehints/TooWideMethodParameterOutTypeRuleTest.php index d8dc023a57e..56824b30bc1 100644 --- a/tests/PHPStan/Rules/TooWideTypehints/TooWideMethodParameterOutTypeRuleTest.php +++ b/tests/PHPStan/Rules/TooWideTypehints/TooWideMethodParameterOutTypeRuleTest.php @@ -5,6 +5,7 @@ use PHPStan\Rules\Properties\PropertyReflectionFinder; use PHPStan\Rules\Rule as TRule; use PHPStan\Testing\RuleTestCase; +use PHPUnit\Framework\Attributes\RequiresPhp; /** * @extends RuleTestCase @@ -144,4 +145,16 @@ public function testNestedTooWideType(): void ]); } + #[RequiresPhp('>= 8.0.0')] + public function testBug15066(): void + { + $this->analyse([__DIR__ . '/data/bug-15066.php'], [ + [ + 'Method Bug15066\\Foo::variadicNeverNull() never assigns null to &$refs so it can be removed from the by-ref type.', + 45, + 'You can narrow the parameter out type with @param-out PHPDoc tag.', + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/TooWideTypehints/data/bug-15066.php b/tests/PHPStan/Rules/TooWideTypehints/data/bug-15066.php new file mode 100644 index 00000000000..fecc2dbb9af --- /dev/null +++ b/tests/PHPStan/Rules/TooWideTypehints/data/bug-15066.php @@ -0,0 +1,65 @@ += 8.0 + +namespace Bug15066; + +function variadicByRef(string|null &...$refs): void +{ + foreach ($refs as &$ref) { + $ref = $ref === null ? null : trim($ref); + } +} + +function singleByRef(string|null &$ref): void +{ + $ref = $ref === null ? null : trim($ref); +} + +function variadicByIndex(string|null &...$refs): void +{ + foreach ($refs as $key => $value) { + $refs[$key] = $value === null ? null : trim($value); + } +} + +function variadicNeverNull(string|null &...$refs): void +{ + foreach ($refs as $key => $value) { + $refs[$key] = 'foo'; + } +} + +function variadicNeverWritten(string|null &...$refs): void +{ +} + +class Foo +{ + + public function variadicByIndex(string|null &...$refs): void + { + foreach ($refs as $key => $value) { + $refs[$key] = $value === null ? null : trim($value); + } + } + + public function variadicNeverNull(string|null &...$refs): void + { + foreach ($refs as $key => $value) { + $refs[$key] = 'foo'; + } + } + +} + +// Rebinding the packed variable discards the references it held, so PHP writes nothing back and +// there is no out value left to check. An array is still compared, because a write through an +// offset reaches the caller and leaves the variable as an array too. +function variadicRebindNonArray(string|null &...$refs): void +{ + $refs = 42; +} + +function variadicRebindOnlyString(string|null &...$refs): void +{ + $refs = ['ok']; +} diff --git a/tests/PHPStan/Rules/Variables/ParameterOutAssignedTypeRuleTest.php b/tests/PHPStan/Rules/Variables/ParameterOutAssignedTypeRuleTest.php index abba16bcb0e..ed7fb791ddb 100644 --- a/tests/PHPStan/Rules/Variables/ParameterOutAssignedTypeRuleTest.php +++ b/tests/PHPStan/Rules/Variables/ParameterOutAssignedTypeRuleTest.php @@ -117,4 +117,27 @@ public function testCatchVariable(): void ]); } + #[RequiresPhp('>= 8.0.0')] + public function testBug15066(): void + { + $this->analyse([__DIR__ . '/data/bug-15066.php'], [ + [ + 'Parameter &$refs by-ref type of function Bug15066Variables\\variadicWrongType() expects string|null, int|string|null given.', + 22, + 'You can change the parameter out type with @param-out PHPDoc tag.', + ], + [ + 'Parameter &$refs by-ref type of method Bug15066Variables\\Foo::variadicWrongType() expects string|null, int|string|null given.', + 52, + 'You can change the parameter out type with @param-out PHPDoc tag.', + ], + // rebinding the packed variable to a non-array is silent, to an array is not - see the fixture + [ + 'Parameter &$refs by-ref type of function Bug15066Variables\\variadicRebindWrongArray() expects string|null, int given.', + 69, + 'You can change the parameter out type with @param-out PHPDoc tag.', + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Variables/ParameterOutExecutionEndTypeRuleTest.php b/tests/PHPStan/Rules/Variables/ParameterOutExecutionEndTypeRuleTest.php index 311ffea2114..bd3a5258348 100644 --- a/tests/PHPStan/Rules/Variables/ParameterOutExecutionEndTypeRuleTest.php +++ b/tests/PHPStan/Rules/Variables/ParameterOutExecutionEndTypeRuleTest.php @@ -5,6 +5,7 @@ use PHPStan\Rules\Rule; use PHPStan\Rules\RuleLevelHelper; use PHPStan\Testing\RuleTestCase; +use PHPUnit\Framework\Attributes\RequiresPhp; /** * @extends RuleTestCase @@ -74,4 +75,15 @@ public function testBug12330(): void $this->analyse([__DIR__ . '/data/bug-12330.php'], []); } + #[RequiresPhp('>= 8.0.0')] + public function testBug15066(): void + { + $this->analyse([__DIR__ . '/data/bug-15066.php'], [ + [ + 'Parameter &$refs @param-out type of function Bug15066Variables\\variadicParamOutNeverWritten() expects string, string|null given.', + 35, + ], + ]); + } + } diff --git a/tests/PHPStan/Rules/Variables/data/bug-15066.php b/tests/PHPStan/Rules/Variables/data/bug-15066.php new file mode 100644 index 00000000000..9558061f86e --- /dev/null +++ b/tests/PHPStan/Rules/Variables/data/bug-15066.php @@ -0,0 +1,83 @@ += 8.0 + +namespace Bug15066Variables; + +function variadicByRef(string|null &...$refs): void +{ + foreach ($refs as &$ref) { + $ref = $ref === null ? null : trim($ref); + } +} + +function variadicByIndex(string|null &...$refs): void +{ + foreach ($refs as $key => $value) { + $refs[$key] = $value === null ? null : trim($value); + } +} + +function variadicWrongType(string|null &...$refs): void +{ + foreach ($refs as $key => $value) { + $refs[$key] = 42; + } +} + +/** @param-out string|null $refs */ +function variadicParamOut(string|null &...$refs): void +{ + foreach ($refs as $key => $value) { + $refs[$key] = $value === null ? null : trim($value); + } +} + +/** @param-out string $refs */ +function variadicParamOutNeverWritten(string|null &...$refs): void +{ +} + +class Foo +{ + + public function variadicByIndex(string|null &...$refs): void + { + foreach ($refs as $key => $value) { + $refs[$key] = $value === null ? null : trim($value); + } + } + + public function variadicWrongType(string|null &...$refs): void + { + foreach ($refs as $key => $value) { + $refs[$key] = 42; + } + } + +} + +// Rebinding the packed variable discards the references it held, so PHP writes nothing back to any +// caller. Nothing is reported for a non-array, because no out value is left to check. An array is +// still reported: a write through an offset reaches the caller and leaves the variable as an array +// too, so the two cannot be told apart here, and the offset write is the case that matters. +function variadicRebindNonArray(string|null &...$refs): void +{ + $refs = 42; +} + +function variadicRebindWrongArray(string|null &...$refs): void +{ + $refs = [42]; +} + +function variadicRebindOkArray(string|null &...$refs): void +{ + $refs = ['ok']; +} + +// The packed variable may end up only maybe holding an array. There is then no element type to +// speak of, so the comparison is skipped rather than run against a nonexistent one. +function variadicRebindMaybeArray(string|null &...$refs): void +{ + $refs = rand(0, 1) === 1 ? [42] : null; +} +