diff --git a/conf/bleedingEdge.neon b/conf/bleedingEdge.neon index 22487e357c..c6c8c7fbec 100644 --- a/conf/bleedingEdge.neon +++ b/conf/bleedingEdge.neon @@ -6,3 +6,4 @@ parameters: stricterFunctionMap: true reportPreciseLineForUnusedFunctionParameter: true internalTag: true + checkPrintfParameterTypes: true diff --git a/conf/config.level5.neon b/conf/config.level5.neon index fd3835fbf1..ff67944231 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% rules: - PHPStan\Rules\DateTimeInstantiationRule @@ -42,3 +44,5 @@ services: - phpstan.rules.rule - class: PHPStan\Rules\Functions\ParameterCastableToNumberRule + - + class: PHPStan\Rules\Functions\PrintfParameterTypeRule diff --git a/conf/config.neon b/conf/config.neon index 7a4a43a4de..6eaa881ade 100644 --- a/conf/config.neon +++ b/conf/config.neon @@ -27,6 +27,7 @@ parameters: stricterFunctionMap: false reportPreciseLineForUnusedFunctionParameter: false internalTag: false + checkPrintfParameterTypes: false fileExtensions: - php checkAdvancedIsset: false diff --git a/conf/parametersSchema.neon b/conf/parametersSchema.neon index d18df776e3..43ca887191 100644 --- a/conf/parametersSchema.neon +++ b/conf/parametersSchema.neon @@ -33,6 +33,7 @@ parametersSchema: stricterFunctionMap: bool() reportPreciseLineForUnusedFunctionParameter: bool() internalTag: bool() + checkPrintfParameterTypes: bool() ]) fileExtensions: listOf(string()) checkAdvancedIsset: bool() diff --git a/src/Rules/Functions/PrintfHelper.php b/src/Rules/Functions/PrintfHelper.php index a5d4571f76..ba196c3a1c 100644 --- a/src/Rules/Functions/PrintfHelper.php +++ b/src/Rules/Functions/PrintfHelper.php @@ -4,31 +4,104 @@ use Nette\Utils\Strings; use PHPStan\Php\PhpVersion; +use PHPStan\Type\ErrorType; +use PHPStan\Type\IntegerType; +use PHPStan\Type\Type; use function array_filter; +use function array_flip; +use function array_keys; +use function array_map; +use function array_reduce; use function count; +use function in_array; use function max; +use function sort; use function sprintf; use function strlen; +use function usort; use const PREG_SET_ORDER; +/** @phpstan-type AcceptingTypeString 'strict-int'|'int'|'float'|'string'|'mixed' */ 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); + } + + /** @return array position => [type name, matches callback] */ + public function getPrintfPlaceholderAcceptingTypes(string $format): array + { + $placeholders = $this->parsePlaceholders(self::PRINTF_SPECIFIER_PATTERN, $format); + $result = []; + // Type on the left can go to the type on the right, but not vice versa. + $typeSequenceMap = array_flip(['int', 'float', 'string', 'mixed']); + + foreach ($placeholders as $position => $types) { + sort($types); + $typeNames = array_map( + static fn (string $t) => $t === 'strict-int' + ? 'int' + : $t, + $types, + ); + $typeName = array_reduce( + $typeNames, + static fn (string $carry, string $type) => $typeSequenceMap[$carry] < $typeSequenceMap[$type] + ? $carry + : $type, + 'mixed', + ); + $result[$position] = [ + $typeName, + static function (Type $t) use ($types): bool { + foreach ($types as $acceptingType) { + switch ($acceptingType) { + case 'strict-int': + $subresult = (new IntegerType())->accepts($t, true)->yes(); + break; + case 'int': + $subresult = ! $t->toInteger() instanceof ErrorType; + break; + case 'float': + $subresult = ! $t->toFloat() instanceof ErrorType; + break; + // The function signature already limits the parameters to stringable types, so there's + // no point in checking string again here. + case 'string': + case 'mixed': + default: + $subresult = true; + break; + } + + if (!$subresult) { + return false; + } + } + + return true; + }, + ]; + } + + return $result; } 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> position => type */ + private function parsePlaceholders(string $specifiersPattern, string $format): array { $addSpecifier = ''; if ($this->phpVersion->supportsHhPrintfSpecifier()) { @@ -42,34 +115,72 @@ 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 = []; + $positionalPlaceholders = []; + $idx = 0; - $maxPositionedNumber = 0; - $maxOrdinaryNumber = 0; foreach ($placeholders as $placeholder) { if (isset($placeholder['width']) && $placeholder['width'] !== '') { - $maxOrdinaryNumber++; + $result[$idx++] = ['strict-int' => 1]; } if (isset($placeholder['precision']) && $placeholder['precision'] !== '') { - $maxOrdinaryNumber++; + $result[$idx++] = ['strict-int' => 1]; } if (isset($placeholder['position']) && $placeholder['position'] !== '') { - $maxPositionedNumber = max((int) $placeholder['position'], $maxPositionedNumber); - } else { - $maxOrdinaryNumber++; + // It may reference future position, so we have to process them later. + $positionalPlaceholders[] = $placeholder; + continue; } + + $result[$idx++][$this->getAcceptingTypeBySpecifier($placeholder['specifier'] ?? '')] = 1; + } + + usort( + $positionalPlaceholders, + static fn (array $a, array $b) => (int) $a['position'] <=> (int) $b['position'], + ); + + foreach ($positionalPlaceholders as $placeholder) { + $idx = $placeholder['position'] - 1; + $result[$idx][$this->getAcceptingTypeBySpecifier($placeholder['specifier'] ?? '')] = 1; + } + + return array_map(static fn (array $a) => array_keys($a), $result); + } + + /** @phpstan-return 'string'|'int'|'float'|'mixed' */ + private function getAcceptingTypeBySpecifier(string $specifier): string + { + if ($specifier === 's') { + return 'string'; } - return max($maxPositionedNumber, $maxOrdinaryNumber); + 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 '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..0821afb019 --- /dev/null +++ b/src/Rules/Functions/PrintfParameterTypeRule.php @@ -0,0 +1,145 @@ + + */ +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(); + $acceptingTypes = $this->printfHelper->getPrintfPlaceholderAcceptingTypes($format); + $errors = []; + $typeAllowedByCallToFunctionParametersRule = TypeCombinator::union( + new StringAlwaysAcceptingObjectWithToStringType(), + new IntegerType(), + new FloatType(), + new BooleanType(), + new NullType(), + ); + + for ($i = $formatArgumentPosition + 1, $j = 0; $i < $argsCount; $i++, $j++) { + // Some arguments may be skipped entirely. + if (! array_key_exists($j, $acceptingTypes)) { + continue; + } + + [$acceptingName, $acceptingCb] = $acceptingTypes[$j]; + $argType = $this->ruleLevelHelper->findTypeToCheck( + $scope, + $args[$i]->value, + '', + $acceptingCb, + )->getType(); + + if ($argType instanceof ErrorType || $acceptingCb($argType)) { + continue; + } + + // This is already reported by CallToFunctionParametersRule + if ( + !$this->ruleLevelHelper->accepts( + $typeAllowedByCallToFunctionParametersRule, + $argType, + $scope->isDeclareStrictTypes(), + )->result + ) { + continue; + } + + $errors[] = RuleErrorBuilder::message( + sprintf( + 'Placeholder #%d of function %s expects %s, %s given', + $j + 1, + $name, + $acceptingName, + $argType->describe(VerbosityLevel::typeOnly()), + ), + )->identifier('argument.type')->build(); + } + + return $errors; + } + +} diff --git a/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php b/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php new file mode 100644 index 0000000000..7fc45e4385 --- /dev/null +++ b/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php @@ -0,0 +1,102 @@ + + */ +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'], [ + [ + 'Placeholder #1 of function printf expects int, PrintfParamTypes\\FooStringable given', + 15, + ], + [ + 'Placeholder #1 of function printf expects int, int|PrintfParamTypes\\FooStringable given', + 16, + ], + [ + 'Placeholder #1 of function printf expects float, PrintfParamTypes\\FooStringable given', + 17, + ], + [ + 'Placeholder #1 of function sprintf expects int, PrintfParamTypes\\FooStringable given', + 18, + ], + [ + 'Placeholder #1 of function fprintf expects float, PrintfParamTypes\\FooStringable given', + 19, + ], + [ + 'Placeholder #1 of function printf expects int, string given', + 20, + ], + [ + 'Placeholder #1 of function printf expects int, float given', + 21, + ], + [ + 'Placeholder #1 of function printf expects int, SimpleXMLElement given', + 22, + ], + [ + 'Placeholder #1 of function printf expects int, null given', + 23, + ], + [ + 'Placeholder #1 of function printf expects int, true given', + 24, + ], + [ + 'Placeholder #1 of function printf expects int, string given', + 25, + ], + [ + 'Placeholder #1 of function printf expects int, string given', + 26, + ], + [ + 'Placeholder #1 of function printf expects float, PrintfParamTypes\\FooStringable given', + 27, + ], + [ + 'Placeholder #1 of function printf expects float, PrintfParamTypes\\FooStringable given', + 28, + ], + [ + 'Placeholder #3 of function printf expects float, PrintfParamTypes\\FooStringable given', + 29, + ], + ]); + } + +} 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..6c1c2142af --- /dev/null +++ b/tests/PHPStan/Rules/Functions/data/printf-param-types.php @@ -0,0 +1,66 @@ +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()); + +// 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