diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index 3939aa9bff..8cccad6c7e 100644 --- a/conf/bleedingEdge.neon +++ b/conf/bleedingEdge.neon @@ -6,6 +6,7 @@ parameters: skipCheckGenericClasses!: [] stricterFunctionMap: true reportPreciseLineForUnusedFunctionParameter: true + checkPrintfParameterTypes: true internalTag: true newStaticInAbstractClassStaticMethod: true checkExtensionsForComparisonOperators: true diff --git a/conf/config.level5.neon b/conf/config.level5.neon index 4d9bbeedf4..4dfbc72598 100644 --- a/conf/config.level5.neon +++ b/conf/config.level5.neon @@ -8,6 +8,8 @@ parameters: conditionalTags: PHPStan\Rules\Functions\ParameterCastableToNumberRule: phpstan.rules.rule: %featureToggles.checkParameterCastableToNumberFunctions% + PHPStan\Rules\Functions\PrintfParameterTypeRule: + phpstan.rules.rule: %featureToggles.checkPrintfParameterTypes% autowiredAttributeServices: # registers rules with #[RegisteredRule] attribute @@ -16,3 +18,5 @@ autowiredAttributeServices: services: - class: PHPStan\Rules\Functions\ParameterCastableToNumberRule + - + class: PHPStan\Rules\Functions\PrintfParameterTypeRule diff --git a/conf/config.neon b/conf/config.neon index f922f49d60..9d7c9e061b 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -30,6 +30,7 @@ parameters: - DOMNamedNodeMap stricterFunctionMap: false reportPreciseLineForUnusedFunctionParameter: false + checkPrintfParameterTypes: false internalTag: false newStaticInAbstractClassStaticMethod: false checkExtensionsForComparisonOperators: false diff --git a/conf/parametersSchema.neon b/conf/parametersSchema.neon index 9670a557bd..73e7f56b0f 100644 --- a/conf/parametersSchema.neon +++ b/conf/parametersSchema.neon @@ -33,6 +33,7 @@ parametersSchema: skipCheckGenericClasses: listOf(string()), stricterFunctionMap: bool() reportPreciseLineForUnusedFunctionParameter: bool() + checkPrintfParameterTypes: bool() internalTag: bool() newStaticInAbstractClassStaticMethod: bool() checkExtensionsForComparisonOperators: bool() diff --git a/src/Rules/Functions/PrintfHelper.php b/src/Rules/Functions/PrintfHelper.php index 7f68f17fa4..73cb4af220 100644 --- a/src/Rules/Functions/PrintfHelper.php +++ b/src/Rules/Functions/PrintfHelper.php @@ -6,7 +6,9 @@ use PHPStan\DependencyInjection\AutowiredService; use PHPStan\Php\PhpVersion; use function array_filter; +use function array_keys; use function count; +use function in_array; use function max; use function sprintf; use function strlen; @@ -16,21 +18,30 @@ final class PrintfHelper { + private const PRINTF_SPECIFIER_PATTERN = '(?[bs%s]|l?[cdeEgfFGouxX])'; + public function __construct(private PhpVersion $phpVersion) { } public function getPrintfPlaceholdersCount(string $format): int { - return $this->getPlaceholdersCount('(?:[bs%s]|l?[cdeEgfFGouxX])', $format); + return $this->getPlaceholdersCount(self::PRINTF_SPECIFIER_PATTERN, $format); + } + + /** @phpstan-return array> parameter index => placeholders */ + public function getPrintfPlaceholders(string $format): array + { + return $this->parsePlaceholders(self::PRINTF_SPECIFIER_PATTERN, $format); } public function getScanfPlaceholdersCount(string $format): int { - return $this->getPlaceholdersCount('(?:[cdDeEfinosuxX%s]|\[[^\]]+\])', $format); + return $this->getPlaceholdersCount('(?[cdDeEfinosuxX%s]|\[[^\]]+\])', $format); } - private function getPlaceholdersCount(string $specifiersPattern, string $format): int + /** @phpstan-return array> parameter index => placeholders */ + private function parsePlaceholders(string $specifiersPattern, string $format): array { $addSpecifier = ''; if ($this->phpVersion->supportsHhPrintfSpecifier()) { @@ -44,34 +55,83 @@ private function getPlaceholdersCount(string $specifiersPattern, string $format) $matches = Strings::matchAll($format, $pattern, PREG_SET_ORDER); if (count($matches) === 0) { - return 0; + return []; } $placeholders = array_filter($matches, static fn (array $match): bool => strlen($match['before']) % 2 === 0); - if (count($placeholders) === 0) { - return 0; - } + $result = []; + $parsedPlaceholders = []; + $parameterIdx = 0; + $placeholderNumber = 0; - $maxPositionedNumber = 0; - $maxOrdinaryNumber = 0; foreach ($placeholders as $placeholder) { + $placeholderNumber++; + $showValueSuffix = false; + if (isset($placeholder['width']) && $placeholder['width'] !== '') { - $maxOrdinaryNumber++; + $parsedPlaceholders[] = new PrintfPlaceholder( + sprintf('"%s" (width)', $placeholder[0]), + $parameterIdx++, + $placeholderNumber, + 'strict-int', + ); + $showValueSuffix = true; } if (isset($placeholder['precision']) && $placeholder['precision'] !== '') { - $maxOrdinaryNumber++; + $parsedPlaceholders[] = new PrintfPlaceholder( + sprintf('"%s" (precision)', $placeholder[0]), + $parameterIdx++, + $placeholderNumber, + 'strict-int', + ); + $showValueSuffix = true; } - if (isset($placeholder['position']) && $placeholder['position'] !== '') { - $maxPositionedNumber = max((int) $placeholder['position'], $maxPositionedNumber); - } else { - $maxOrdinaryNumber++; - } + $parsedPlaceholders[] = new PrintfPlaceholder( + sprintf('"%s"', $placeholder[0]) . ($showValueSuffix ? ' (value)' : ''), + isset($placeholder['position']) && $placeholder['position'] !== '' + ? $placeholder['position'] - 1 + : $parameterIdx++, + $placeholderNumber, + $this->getAcceptingTypeBySpecifier($placeholder['specifier'] ?? ''), + ); + } + + foreach ($parsedPlaceholders as $placeholder) { + $result[$placeholder->parameterIndex][] = $placeholder; + } + + return $result; + } + + /** @phpstan-return 'string'|'int'|'float'|'mixed' */ + private function getAcceptingTypeBySpecifier(string $specifier): string + { + if ($specifier === 's') { + return 'string'; + } + + if (in_array($specifier, ['d', 'u', 'c', 'o', 'x', 'X', 'b'], true)) { + return 'int'; + } + + if (in_array($specifier, ['e', 'E', 'f', 'F', 'g', 'G', 'h', 'H'], true)) { + return 'float'; } - return max($maxPositionedNumber, $maxOrdinaryNumber); + return 'mixed'; + } + + private function getPlaceholdersCount(string $specifiersPattern, string $format): int + { + $paramIndices = array_keys($this->parsePlaceholders($specifiersPattern, $format)); + + return $paramIndices === [] + ? 0 + // The indices start from 0 + : max($paramIndices) + 1; } } diff --git a/src/Rules/Functions/PrintfParameterTypeRule.php b/src/Rules/Functions/PrintfParameterTypeRule.php new file mode 100644 index 0000000000..8e2997c67f --- /dev/null +++ b/src/Rules/Functions/PrintfParameterTypeRule.php @@ -0,0 +1,155 @@ + + */ +final class PrintfParameterTypeRule implements Rule +{ + + private const FORMAT_ARGUMENT_POSITIONS = [ + 'printf' => 0, + 'sprintf' => 0, + 'fprintf' => 1, + ]; + private const MINIMUM_NUMBER_OF_ARGUMENTS = [ + 'printf' => 1, + 'sprintf' => 1, + 'fprintf' => 2, + ]; + + public function __construct( + private PrintfHelper $printfHelper, + private ReflectionProvider $reflectionProvider, + private RuleLevelHelper $ruleLevelHelper, + ) + { + } + + public function getNodeType(): string + { + return Node\Expr\FuncCall::class; + } + + public function processNode(Node $node, Scope $scope): array + { + if (!($node->name instanceof Node\Name)) { + return []; + } + + if (!$this->reflectionProvider->hasFunction($node->name, $scope)) { + return []; + } + + $functionReflection = $this->reflectionProvider->getFunction($node->name, $scope); + $name = $functionReflection->getName(); + if (!array_key_exists($name, self::FORMAT_ARGUMENT_POSITIONS)) { + return []; + } + + $formatArgumentPosition = self::FORMAT_ARGUMENT_POSITIONS[$name]; + + $args = $node->getArgs(); + foreach ($args as $arg) { + if ($arg->unpack) { + return []; + } + } + $argsCount = count($args); + if ($argsCount < self::MINIMUM_NUMBER_OF_ARGUMENTS[$name]) { + return []; // caught by CallToFunctionParametersRule + } + + $formatArgType = $scope->getType($args[$formatArgumentPosition]->value); + $formatArgTypeStrings = $formatArgType->getConstantStrings(); + + // Let's start simple for now. + if (count($formatArgTypeStrings) !== 1) { + return []; + } + + $formatString = $formatArgTypeStrings[0]; + $format = $formatString->getValue(); + $placeholderMap = $this->printfHelper->getPrintfPlaceholders($format); + $errors = []; + $typeAllowedByCallToFunctionParametersRule = TypeCombinator::union( + new StringAlwaysAcceptingObjectWithToStringType(), + new IntegerType(), + new FloatType(), + new BooleanType(), + new NullType(), + ); + // Type on the left can go to the type on the right, but not vice versa. + $allowedTypeNameMap = [ + 'strict-int' => 'int', + 'int' => 'castable to int', + 'float' => 'castable to float', + // These are here just for completeness. They won't be used because, these types are already enforced by + // CallToFunctionParametersRule. + 'string' => 'castable to string', + 'mixed' => 'castable to string', + ]; + + for ($i = $formatArgumentPosition + 1, $j = 0; $i < $argsCount; $i++, $j++) { + // Some arguments may be skipped entirely. + foreach ($placeholderMap[$j] ?? [] as $placeholder) { + $argType = $this->ruleLevelHelper->findTypeToCheck( + $scope, + $args[$i]->value, + '', + static fn (Type $t) => $placeholder->doesArgumentTypeMatchPlaceholder($t), + )->getType(); + + if ($argType instanceof ErrorType || $placeholder->doesArgumentTypeMatchPlaceholder($argType)) { + continue; + } + + // This is already reported by CallToFunctionParametersRule + if ( + !$this->ruleLevelHelper->accepts( + $typeAllowedByCallToFunctionParametersRule, + $argType, + $scope->isDeclareStrictTypes(), + )->result + ) { + continue; + } + + $errors[] = RuleErrorBuilder::message( + sprintf( + 'Parameter #%d of function %s is expected to be %s by placeholder #%d (%s), %s given.', + $i + 1, + $name, + $allowedTypeNameMap[$placeholder->acceptingType], + $placeholder->placeholderNumber, + $placeholder->label, + $argType->describe(VerbosityLevel::typeOnly()), + ), + )->identifier('argument.type')->build(); + } + } + + return $errors; + } + +} diff --git a/src/Rules/Functions/PrintfPlaceholder.php b/src/Rules/Functions/PrintfPlaceholder.php new file mode 100644 index 0000000000..8e2c1336fa --- /dev/null +++ b/src/Rules/Functions/PrintfPlaceholder.php @@ -0,0 +1,44 @@ +acceptingType) { + case 'strict-int': + return (new IntegerType())->accepts($argumentType, true)->yes(); + case 'int': + return ! $argumentType->toInteger() instanceof ErrorType; + case 'float': + return ! $argumentType->toFloat() instanceof ErrorType; + // The function signature already limits the parameters to stringable types, so there's + // no point in checking string again here. + case 'string': + case 'mixed': + return true; + // Without this PHPStan with PHP 7.4 reports "...should return bool but return statement is missing." + // Presumably, because promoted properties are turned into regular properties and the phpdoc isn't applied to the property. + default: + throw new ShouldNotHappenException('Unexpected type ' . $this->acceptingType); + } + } + +} diff --git a/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php b/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php new file mode 100644 index 0000000000..1259a4cd53 --- /dev/null +++ b/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php @@ -0,0 +1,114 @@ + + */ +class PrintfParameterTypeRuleTest extends RuleTestCase +{ + + protected function getRule(): Rule + { + $reflectionProvider = $this->createReflectionProvider(); + return new PrintfParameterTypeRule( + new PrintfHelper(new PhpVersion(PHP_VERSION_ID)), + $reflectionProvider, + new RuleLevelHelper( + $reflectionProvider, + true, + false, + true, + true, + true, + true, + false, + ), + ); + } + + public function test(): void + { + $this->analyse([__DIR__ . '/data/printf-param-types.php'], [ + [ + 'Parameter #2 of function printf is expected to be castable to int by placeholder #1 ("%d"), PrintfParamTypes\\FooStringable given.', + 15, + ], + [ + 'Parameter #2 of function printf is expected to be castable to int by placeholder #1 ("%d"), int|PrintfParamTypes\\FooStringable given.', + 16, + ], + [ + 'Parameter #2 of function printf is expected to be castable to float by placeholder #1 ("%f"), PrintfParamTypes\\FooStringable given.', + 17, + ], + [ + 'Parameter #2 of function sprintf is expected to be castable to int by placeholder #1 ("%d"), PrintfParamTypes\\FooStringable given.', + 18, + ], + [ + 'Parameter #3 of function fprintf is expected to be castable to float by placeholder #1 ("%f"), PrintfParamTypes\\FooStringable given.', + 19, + ], + [ + 'Parameter #2 of function printf is expected to be int by placeholder #1 ("%*s" (width)), string given.', + 20, + ], + [ + 'Parameter #2 of function printf is expected to be int by placeholder #1 ("%*s" (width)), float given.', + 21, + ], + [ + 'Parameter #2 of function printf is expected to be int by placeholder #1 ("%*s" (width)), SimpleXMLElement given.', + 22, + ], + [ + 'Parameter #2 of function printf is expected to be int by placeholder #1 ("%*s" (width)), null given.', + 23, + ], + [ + 'Parameter #2 of function printf is expected to be int by placeholder #1 ("%*s" (width)), true given.', + 24, + ], + [ + 'Parameter #2 of function printf is expected to be int by placeholder #1 ("%.*s" (precision)), string given.', + 25, + ], + [ + 'Parameter #2 of function printf is expected to be int by placeholder #2 ("%3$.*s" (precision)), string given.', + 26, + ], + [ + 'Parameter #2 of function printf is expected to be castable to float by placeholder #1 ("%1$-\'X10.2f"), PrintfParamTypes\\FooStringable given.', + 27, + ], + [ + 'Parameter #2 of function printf is expected to be castable to float by placeholder #2 ("%1$*.*f" (value)), PrintfParamTypes\\FooStringable given.', + 28, + ], + [ + 'Parameter #4 of function printf is expected to be castable to float by placeholder #1 ("%3$f"), PrintfParamTypes\\FooStringable given.', + 29, + ], + [ + 'Parameter #2 of function printf is expected to be castable to float by placeholder #1 ("%1$f"), PrintfParamTypes\\FooStringable given.', + 30, + ], + [ + 'Parameter #2 of function printf is expected to be castable to int by placeholder #2 ("%1$d"), PrintfParamTypes\\FooStringable given.', + 30, + ], + [ + 'Parameter #2 of function printf is expected to be int by placeholder #1 ("%1$*d" (width)), float given.', + 31, + ], + ]); + } + +} diff --git a/tests/PHPStan/Rules/Functions/data/printf-param-types.php b/tests/PHPStan/Rules/Functions/data/printf-param-types.php new file mode 100644 index 0000000000..b2c5748627 --- /dev/null +++ b/tests/PHPStan/Rules/Functions/data/printf-param-types.php @@ -0,0 +1,68 @@ +7'), 'a'); +printf('%*s', null, 'a'); +printf('%*s', true, 'a'); +printf('%.*s', '5', 'a'); +printf('%2$s %3$.*s', '1', 5, 'a'); // * is the first ordinary placeholder, so it matches '1' +printf('%1$-\'X10.2f', new FooStringable()); +printf('%s %1$*.*f', new FooStringable(), 5, 2); +printf('%3$f', 1, 2, new FooStringable()); +printf('%1$f %1$d', new FooStringable()); +printf('%1$*d', 5.5); + +// Strict error +printf('%d', 1.23); +printf('%d', rand() ? 1.23 : 1); +printf('%d', 'a'); +printf('%d', '1.23'); +printf('%d', null); +printf('%d', true); +printf('%d', new \SimpleXMLElement('aaa')); + +printf('%f', 'a'); +printf('%f', null); +printf('%f', true); +printf('%f', new \SimpleXMLElement('aaa')); + +printf('%s', null); +printf('%s', true); + +// Error, but already reported by CallToFunctionParametersRule +printf('%d', new \stdClass()); +printf('%s', []); + +// Error, but already reported by PrintfParametersRule +printf('%s'); +printf('%s', 1, 2); + +// OK +printf('%s', 'a'); +printf('%s', new FooStringable()); +printf('%d', 1); +printf('%f', 1); +printf('%f', 1.1); +printf('%*s', 5, 'a'); +printf('%2$*s', 5, 'a'); +printf('%s %2$*s', 'a', 5, 'a'); +printf('%1$-+\'X10.2f', 5); +printf('%1$*.*f %s %2$d', 5, 6, new FooStringable()); // 5.000000 foo 6