diff --git a/Makefile b/Makefile index 02c6d35..3941124 100644 --- a/Makefile +++ b/Makefile @@ -8,6 +8,8 @@ tests: .PHONY: lint lint: php vendor/bin/parallel-lint --colors -e php,php3,php4,php5,phtml,phpt,fixed \ + --exclude tests/Rules/StrictCalls/data/strict-function-calls-positional-before-named.php \ + --exclude tests/Rules/StrictCalls/data/strict-function-calls-positional-before-named.php.fixed \ src tests .PHONY: cs-install diff --git a/src/Rules/StrictCalls/StrictFunctionCallsRule.php b/src/Rules/StrictCalls/StrictFunctionCallsRule.php index 9af4389..c8b0fbc 100644 --- a/src/Rules/StrictCalls/StrictFunctionCallsRule.php +++ b/src/Rules/StrictCalls/StrictFunctionCallsRule.php @@ -3,15 +3,24 @@ namespace PHPStan\Rules\StrictCalls; use PhpParser\Node; +use PhpParser\Node\Arg; +use PhpParser\Node\Expr\ConstFetch; use PhpParser\Node\Expr\FuncCall; +use PhpParser\Node\Identifier; use PhpParser\Node\Name; +use PhpParser\Node\Scalar; use PHPStan\Analyser\ArgumentsNormalizer; use PHPStan\Analyser\Scope; +use PHPStan\Reflection\ParametersAcceptor; use PHPStan\Reflection\ParametersAcceptorSelector; use PHPStan\Reflection\ReflectionProvider; +use PHPStan\Rules\IdentifierRuleError; use PHPStan\Rules\Rule; use PHPStan\Rules\RuleErrorBuilder; +use PHPStan\Type\Type; use function array_key_exists; +use function array_search; +use function in_array; use function sprintf; use function strtolower; @@ -47,6 +56,8 @@ public function processNode(Node $node, Scope $scope): array return []; } + $originalNode = $node; + if (!$this->reflectionProvider->hasFunction($node->name, $scope)) { return []; } @@ -69,26 +80,165 @@ public function processNode(Node $node, Scope $scope): array $argumentPosition = $this->functionArguments[$functionName]; if (!array_key_exists($argumentPosition, $node->getArgs())) { return [ - RuleErrorBuilder::message(sprintf( - 'Call to function %s() requires parameter #%d to be set.', + $this->buildError( + sprintf( + 'Call to function %s() requires parameter #%d to be set.', + $functionName, + $argumentPosition + 1, + ), $functionName, - $argumentPosition + 1, - ))->identifier('function.strict')->build(), + $originalNode, + $node, + $scope, + $parametersAcceptor, + $argumentPosition, + ), ]; } $argumentType = $scope->getType($node->getArgs()[$argumentPosition]->value); if (!$argumentType->isTrue()->yes()) { return [ - RuleErrorBuilder::message(sprintf( - 'Call to function %s() requires parameter #%d to be true.', + $this->buildError( + sprintf( + 'Call to function %s() requires parameter #%d to be true.', + $functionName, + $argumentPosition + 1, + ), $functionName, - $argumentPosition + 1, - ))->identifier('function.strict')->build(), + $originalNode, + $node, + $scope, + $parametersAcceptor, + $argumentPosition, + ), ]; } return []; } + private function buildError( + string $message, + string $functionName, + FuncCall $originalNode, + FuncCall $normalizedNode, + Scope $scope, + ParametersAcceptor $parametersAcceptor, + int $strictArgumentPosition + ): IdentifierRuleError + { + $errorBuilder = RuleErrorBuilder::message($message)->identifier('function.strict'); + if ( + ($functionName === 'in_array' || $functionName === 'array_search' || $functionName === 'array_keys') + && $this->hasSafeStrictComparisonTypes($functionName, $normalizedNode, $scope) + ) { + $namedArgumentPositions = []; + foreach ($parametersAcceptor->getParameters() as $position => $parameter) { + $namedArgumentPositions[$parameter->getName()] = $position; + } + + $errorBuilder->fixNode($originalNode, static function (FuncCall $call) use ($namedArgumentPositions, $strictArgumentPosition): FuncCall { + $strictArgument = null; + $hasNamedArguments = false; + $usedPositions = []; + foreach ($call->getArgs() as $arg) { + if ($arg->unpack) { + // unpack is not fixable + return $call; + } + + if ($arg->name !== null) { + $hasNamedArguments = true; + $argumentName = $arg->name->toString(); + if (!array_key_exists($argumentName, $namedArgumentPositions)) { + return $call; + } + + $argumentPosition = $namedArgumentPositions[$argumentName]; + $usedPositions[$argumentPosition] = true; + if ($argumentPosition === $strictArgumentPosition) { + $strictArgument = $arg; + } + continue; + } + + $argumentPosition = 0; + while (array_key_exists($argumentPosition, $usedPositions)) { + $argumentPosition++; + } + $usedPositions[$argumentPosition] = true; + if ($argumentPosition !== $strictArgumentPosition) { + continue; + } + + $strictArgument = $arg; + } + + $true = new ConstFetch(new Name('true')); + if ($strictArgument !== null) { + if ( + $strictArgument->value instanceof ConstFetch + ) { + if (!in_array($strictArgument->value->name->toLowerString(), ['true', 'false', 'null'], true)) { + return $call; + } + } elseif ( + !$strictArgument->value instanceof Scalar\Int_ + && !$strictArgument->value instanceof Scalar\String_ + ) { + return $call; + } + + $strictArgument->value = $true; + return $call; + } + + $targetArgumentName = array_search($strictArgumentPosition, $namedArgumentPositions, true); + + $call->args[] = new Arg( + $true, + false, + false, + [], + $hasNamedArguments && $targetArgumentName !== false ? new Identifier($targetArgumentName) : null, + ); + + return $call; + }); + } + + return $errorBuilder->build(); + } + + private function hasSafeStrictComparisonTypes(string $functionName, FuncCall $normalizedNode, Scope $scope): bool + { + $args = $normalizedNode->getArgs(); + if (!isset($args[0], $args[1])) { + return false; + } + + $needleArgument = $functionName === 'array_keys' ? $args[1] : $args[0]; + $haystackArgument = $functionName === 'array_keys' ? $args[0] : $args[1]; + $needleType = $scope->getNativeType($needleArgument->value); + $haystackValueType = $scope->getNativeType($haystackArgument->value)->getIterableValueType(); + + return $this->areSameStrictScalarTypes($needleType, $haystackValueType); + } + + private function areSameStrictScalarTypes(Type $needleType, Type $haystackValueType): bool + { + return ( + $needleType->isBoolean()->yes() + && $haystackValueType->isBoolean()->yes() + ) || ( + $needleType->isInteger()->yes() + && $haystackValueType->isInteger()->yes() + ) || ( + $needleType->isString()->yes() + && $haystackValueType->isString()->yes() + && $haystackValueType->isNumericString()->no() + ); + } + } diff --git a/tests/Rules/StrictCalls/StrictFunctionCallsRuleTest.php b/tests/Rules/StrictCalls/StrictFunctionCallsRuleTest.php index 00e9009..a2af661 100644 --- a/tests/Rules/StrictCalls/StrictFunctionCallsRuleTest.php +++ b/tests/Rules/StrictCalls/StrictFunctionCallsRuleTest.php @@ -75,6 +75,34 @@ public function testRule(): void ]); } + public function testFix(): void + { + $this->fix( + __DIR__ . '/data/strict-function-calls-fixable.php', + __DIR__ . '/data/strict-function-calls-fixable.php.fixed', + ); + } + + public function testFixNamedArguments(): void + { + if (PHP_VERSION_ID < 80000) { + self::markTestSkipped('Test requires PHP 8.0.'); + } + + $this->fix( + __DIR__ . '/data/strict-function-calls-named-arguments.php', + __DIR__ . '/data/strict-function-calls-named-arguments.php.fixed', + ); + } + + public function testFixPositionalBeforeNamed(): void + { + $this->fix( + __DIR__ . '/data/strict-function-calls-positional-before-named.php', + __DIR__ . '/data/strict-function-calls-positional-before-named.php.fixed', + ); + } + public function testBug231(): void { if (PHP_VERSION_ID < 80100) { diff --git a/tests/Rules/StrictCalls/data/strict-function-calls-fixable.php b/tests/Rules/StrictCalls/data/strict-function-calls-fixable.php new file mode 100644 index 0000000..70bf747 --- /dev/null +++ b/tests/Rules/StrictCalls/data/strict-function-calls-fixable.php @@ -0,0 +1,97 @@ +strict}"); + diff --git a/tests/Rules/StrictCalls/data/strict-function-calls-fixable.php.fixed b/tests/Rules/StrictCalls/data/strict-function-calls-fixable.php.fixed new file mode 100644 index 0000000..0202c5e --- /dev/null +++ b/tests/Rules/StrictCalls/data/strict-function-calls-fixable.php.fixed @@ -0,0 +1,97 @@ +strict}"); + diff --git a/tests/Rules/StrictCalls/data/strict-function-calls-named-arguments.php b/tests/Rules/StrictCalls/data/strict-function-calls-named-arguments.php new file mode 100644 index 0000000..e6938f7 --- /dev/null +++ b/tests/Rules/StrictCalls/data/strict-function-calls-named-arguments.php @@ -0,0 +1,40 @@ += 8.2 + +namespace StrictCallsNamedArguments; + +use function file_put_contents; +use function in_array; + +/** + * @param int<5, 10> $phpdocIntRange + */ +function doFoo(int $i, int $i2, $phpdocIntRange): void { + in_array(needle: $i, haystack: [1, 2]); + in_array(needle: $i, haystack: [1, 2], strict: false); + in_array(strict: false, needle: $i, haystack: [1, 2]); + in_array($i, haystack: [1, 2]); + + if ($i > 5 && $i < 10) { + in_array($i, haystack: [1, 2]); + } + if ($i > 0) { + in_array($i, haystack: [1, 2]); + } + if ($i2 > 0) { + in_array($i, haystack: [1, $i2]); + in_array($i, [1, $i2], returnsFalseWithSideEffect()); + } + + in_array($i, haystack: [1, $phpdocIntRange]); + in_array($phpdocIntRange, haystack: [1, 2]); +} + +function doBar(int|string $union): void { + in_array(needle: $union, haystack: [1, 2]); +} + +function returnsFalseWithSideEffect(): false { + // some side-effect + file_put_contents('somefile.txt', 'some content'); + return false; +} diff --git a/tests/Rules/StrictCalls/data/strict-function-calls-named-arguments.php.fixed b/tests/Rules/StrictCalls/data/strict-function-calls-named-arguments.php.fixed new file mode 100644 index 0000000..54a0cad --- /dev/null +++ b/tests/Rules/StrictCalls/data/strict-function-calls-named-arguments.php.fixed @@ -0,0 +1,40 @@ += 8.2 + +namespace StrictCallsNamedArguments; + +use function file_put_contents; +use function in_array; + +/** + * @param int<5, 10> $phpdocIntRange + */ +function doFoo(int $i, int $i2, $phpdocIntRange): void { + in_array(needle: $i, haystack: [1, 2], strict: true); + in_array(needle: $i, haystack: [1, 2], strict: true); + in_array(strict: true, needle: $i, haystack: [1, 2]); + in_array($i, haystack: [1, 2], strict: true); + + if ($i > 5 && $i < 10) { + in_array($i, haystack: [1, 2], strict: true); + } + if ($i > 0) { + in_array($i, haystack: [1, 2], strict: true); + } + if ($i2 > 0) { + in_array($i, haystack: [1, $i2], strict: true); + in_array($i, [1, $i2], returnsFalseWithSideEffect()); + } + + in_array($i, haystack: [1, $phpdocIntRange]); + in_array($phpdocIntRange, haystack: [1, 2]); +} + +function doBar(int|string $union): void { + in_array(needle: $union, haystack: [1, 2]); +} + +function returnsFalseWithSideEffect(): false { + // some side-effect + file_put_contents('somefile.txt', 'some content'); + return false; +} diff --git a/tests/Rules/StrictCalls/data/strict-function-calls-positional-before-named.php b/tests/Rules/StrictCalls/data/strict-function-calls-positional-before-named.php new file mode 100644 index 0000000..fc704c4 --- /dev/null +++ b/tests/Rules/StrictCalls/data/strict-function-calls-positional-before-named.php @@ -0,0 +1,11 @@ += 8.2 + +namespace StrictCallsNamedArgumentsFollowedByPositional; + +use function in_array; + +function doFoo(int $i): void +{ + // named argument followed by positional ones + in_array(needle: $i, haystack: [1, 2], false); +} diff --git a/tests/Rules/StrictCalls/data/strict-function-calls-positional-before-named.php.fixed b/tests/Rules/StrictCalls/data/strict-function-calls-positional-before-named.php.fixed new file mode 100644 index 0000000..69fd467 --- /dev/null +++ b/tests/Rules/StrictCalls/data/strict-function-calls-positional-before-named.php.fixed @@ -0,0 +1,11 @@ += 8.2 + +namespace StrictCallsNamedArgumentsFollowedByPositional; + +use function in_array; + +function doFoo(int $i): void +{ + // named argument followed by positional ones + in_array(needle: $i, haystack: [1, 2], true); +}