From c085116a54ec420cb7e00cf8661d1bb03662f7c0 Mon Sep 17 00:00:00 2001 From: schlndh Date: Thu, 1 May 2025 15:34:51 +0200 Subject: [PATCH 1/9] Bleeding edge - check printf parameter types --- conf/bleedingEdge.neon | 1 + conf/config.level5.neon | 4 + conf/config.neon | 1 + conf/parametersSchema.neon | 1 + src/Rules/Functions/PrintfHelper.php | 145 +++++++++++++++++- .../Functions/PrintfParameterTypeRule.php | 145 ++++++++++++++++++ .../Functions/PrintfParameterTypeRuleTest.php | 90 +++++++++++ .../Functions/data/printf-param-types.php | 62 ++++++++ 8 files changed, 447 insertions(+), 2 deletions(-) create mode 100644 src/Rules/Functions/PrintfParameterTypeRule.php create mode 100644 tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php create mode 100644 tests/PHPStan/Rules/Functions/data/printf-param-types.php 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..02d2533c01 100644 --- a/src/Rules/Functions/PrintfHelper.php +++ b/src/Rules/Functions/PrintfHelper.php @@ -5,29 +5,170 @@ use Nette\Utils\Strings; use PHPStan\DependencyInjection\AutowiredService; 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 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' */ #[AutowiredService] 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 = []; + // int can go into float, string and mixed as well. + // float can't go into int, but it can go to string/mixed. + // string can go into mixed, but not into int/float. + // mixed can only go into mixed. + $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) { + $subresult = match ($acceptingType) { + 'strict-int' => (new IntegerType())->accepts($t, true)->yes(), + // This allows float, constant non-numeric string, ... + 'int' => ! $t->toInteger() instanceof ErrorType, + 'float' => ! $t->toFloat() instanceof ErrorType, + // The function signature already limits the parameters to stringable types, so there's + // no point in checking it again here. + 'string', 'mixed' => true, + }; + + 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); + } + + /** @phpstan-return array> position => type */ + private function parsePlaceholders(string $specifiersPattern, string $format): array + { + $addSpecifier = ''; + if ($this->phpVersion->supportsHhPrintfSpecifier()) { + $addSpecifier .= 'hH'; + } + + $specifiers = sprintf($specifiersPattern, $addSpecifier); + + $pattern = '~(?%*)%(?:(?\d+)\$)?[-+]?(?:[ 0]|(?:\'[^%]))?(?\*)?-?\d*(?:\.(?:\d+|(?\*))?)?' . $specifiers . '~'; + + $matches = Strings::matchAll($format, $pattern, PREG_SET_ORDER); + + if (count($matches) === 0) { + return []; + } + + $placeholders = array_filter($matches, static fn (array $match): bool => strlen($match['before']) % 2 === 0); + + $result = []; + $positionToIdxMap = []; + $positionalPlaceholders = []; + $idx = $position = 0; + + foreach ($placeholders as $placeholder) { + if (isset($placeholder['width']) && $placeholder['width'] !== '') { + $result[$idx] = ['strict-int' => 1]; + $positionToIdxMap[$position++] = $idx++; + } + + if (isset($placeholder['precision']) && $placeholder['precision'] !== '') { + $result[$idx] = ['strict-int' => 1]; + $positionToIdxMap[$position++] = $idx++; + } + + if (isset($placeholder['position']) && $placeholder['position'] !== '') { + // It may reference future position, so we have to process them later. + $positionalPlaceholders[] = $placeholder; + continue; + } + + $position++; + $positionToIdxMap[$position] = $idx; + $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 = $positionToIdxMap[$placeholder['position']] ?? null; + + if ($idx === null) { + continue; + } + + $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 + { + return match ($specifier) { + 's' => 'string', + 'd', 'u', 'c', 'o', 'x', 'X', 'b' => 'int', + 'e', 'E', 'f', 'F', 'g', 'G', 'h', 'H' => 'float', + default => 'mixed', + }; } private function getPlaceholdersCount(string $specifiersPattern, string $format): int 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..0509e223ce --- /dev/null +++ b/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php @@ -0,0 +1,90 @@ + + */ +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, + ], + ]); + } + +} 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..3041f662b5 --- /dev/null +++ b/tests/PHPStan/Rules/Functions/data/printf-param-types.php @@ -0,0 +1,62 @@ +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', []); + +// 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 From 7dc38646ed852587699fd336d6eeaef37c52353c Mon Sep 17 00:00:00 2001 From: schlndh Date: Thu, 1 May 2025 15:46:57 +0200 Subject: [PATCH 2/9] fix missing errors --- src/Rules/Functions/PrintfHelper.php | 18 ++++-------------- .../Functions/PrintfParameterTypeRuleTest.php | 12 ++++++++++++ 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/src/Rules/Functions/PrintfHelper.php b/src/Rules/Functions/PrintfHelper.php index 02d2533c01..a8a0f08758 100644 --- a/src/Rules/Functions/PrintfHelper.php +++ b/src/Rules/Functions/PrintfHelper.php @@ -116,19 +116,16 @@ private function parsePlaceholders(string $specifiersPattern, string $format): a $placeholders = array_filter($matches, static fn (array $match): bool => strlen($match['before']) % 2 === 0); $result = []; - $positionToIdxMap = []; $positionalPlaceholders = []; - $idx = $position = 0; + $idx = 0; foreach ($placeholders as $placeholder) { if (isset($placeholder['width']) && $placeholder['width'] !== '') { - $result[$idx] = ['strict-int' => 1]; - $positionToIdxMap[$position++] = $idx++; + $result[$idx++] = ['strict-int' => 1]; } if (isset($placeholder['precision']) && $placeholder['precision'] !== '') { - $result[$idx] = ['strict-int' => 1]; - $positionToIdxMap[$position++] = $idx++; + $result[$idx++] = ['strict-int' => 1]; } if (isset($placeholder['position']) && $placeholder['position'] !== '') { @@ -137,8 +134,6 @@ private function parsePlaceholders(string $specifiersPattern, string $format): a continue; } - $position++; - $positionToIdxMap[$position] = $idx; $result[$idx++][$this->getAcceptingTypeBySpecifier($placeholder['specifier'] ?? '')] = 1; } @@ -148,12 +143,7 @@ private function parsePlaceholders(string $specifiersPattern, string $format): a ); foreach ($positionalPlaceholders as $placeholder) { - $idx = $positionToIdxMap[$placeholder['position']] ?? null; - - if ($idx === null) { - continue; - } - + $idx = $placeholder['position'] - 1; $result[$idx][$this->getAcceptingTypeBySpecifier($placeholder['specifier'] ?? '')] = 1; } diff --git a/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php b/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php index 0509e223ce..7fc45e4385 100644 --- a/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php +++ b/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php @@ -84,6 +84,18 @@ public function test(): void '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, + ], ]); } From e348973b42705c2e0cf943ca65aa6814ba0ea008 Mon Sep 17 00:00:00 2001 From: schlndh Date: Fri, 2 May 2025 08:10:44 +0200 Subject: [PATCH 3/9] simplify PrintfHelper --- src/Rules/Functions/PrintfHelper.php | 44 ++++------------------------ 1 file changed, 5 insertions(+), 39 deletions(-) diff --git a/src/Rules/Functions/PrintfHelper.php b/src/Rules/Functions/PrintfHelper.php index a8a0f08758..79852c7897 100644 --- a/src/Rules/Functions/PrintfHelper.php +++ b/src/Rules/Functions/PrintfHelper.php @@ -163,46 +163,12 @@ private function getAcceptingTypeBySpecifier(string $specifier): string private function getPlaceholdersCount(string $specifiersPattern, string $format): int { - $addSpecifier = ''; - if ($this->phpVersion->supportsHhPrintfSpecifier()) { - $addSpecifier .= 'hH'; - } - - $specifiers = sprintf($specifiersPattern, $addSpecifier); - - $pattern = '~(?%*)%(?:(?\d+)\$)?[-+]?(?:[ 0]|(?:\'[^%]))?(?\*)?-?\d*(?:\.(?:\d+|(?\*))?)?' . $specifiers . '~'; - - $matches = Strings::matchAll($format, $pattern, PREG_SET_ORDER); - - if (count($matches) === 0) { - return 0; - } - - $placeholders = array_filter($matches, static fn (array $match): bool => strlen($match['before']) % 2 === 0); - - if (count($placeholders) === 0) { - return 0; - } - - $maxPositionedNumber = 0; - $maxOrdinaryNumber = 0; - foreach ($placeholders as $placeholder) { - if (isset($placeholder['width']) && $placeholder['width'] !== '') { - $maxOrdinaryNumber++; - } - - if (isset($placeholder['precision']) && $placeholder['precision'] !== '') { - $maxOrdinaryNumber++; - } - - if (isset($placeholder['position']) && $placeholder['position'] !== '') { - $maxPositionedNumber = max((int) $placeholder['position'], $maxPositionedNumber); - } else { - $maxOrdinaryNumber++; - } - } + $paramIndices = array_keys($this->parsePlaceholders($specifiersPattern, $format)); - return max($maxPositionedNumber, $maxOrdinaryNumber); + return $paramIndices === [] + ? 0 + // The indices start from 0 + : max($paramIndices) + 1; } } From 2a81c1d715a3356f8410d8d596dfc1e0b064f39d Mon Sep 17 00:00:00 2001 From: schlndh Date: Fri, 2 May 2025 08:21:40 +0200 Subject: [PATCH 4/9] clean up --- src/Rules/Functions/PrintfHelper.php | 8 ++------ tests/PHPStan/Rules/Functions/data/printf-param-types.php | 4 ++++ 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/Rules/Functions/PrintfHelper.php b/src/Rules/Functions/PrintfHelper.php index 79852c7897..707f6015f0 100644 --- a/src/Rules/Functions/PrintfHelper.php +++ b/src/Rules/Functions/PrintfHelper.php @@ -42,10 +42,7 @@ public function getPrintfPlaceholderAcceptingTypes(string $format): array { $placeholders = $this->parsePlaceholders(self::PRINTF_SPECIFIER_PATTERN, $format); $result = []; - // int can go into float, string and mixed as well. - // float can't go into int, but it can go to string/mixed. - // string can go into mixed, but not into int/float. - // mixed can only go into mixed. + // 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) { @@ -69,11 +66,10 @@ static function (Type $t) use ($types): bool { foreach ($types as $acceptingType) { $subresult = match ($acceptingType) { 'strict-int' => (new IntegerType())->accepts($t, true)->yes(), - // This allows float, constant non-numeric string, ... 'int' => ! $t->toInteger() instanceof ErrorType, 'float' => ! $t->toFloat() instanceof ErrorType, // The function signature already limits the parameters to stringable types, so there's - // no point in checking it again here. + // no point in checking string again here. 'string', 'mixed' => true, }; diff --git a/tests/PHPStan/Rules/Functions/data/printf-param-types.php b/tests/PHPStan/Rules/Functions/data/printf-param-types.php index 3041f662b5..6c1c2142af 100644 --- a/tests/PHPStan/Rules/Functions/data/printf-param-types.php +++ b/tests/PHPStan/Rules/Functions/data/printf-param-types.php @@ -49,6 +49,10 @@ public function __toString(): string 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()); From 057206974d1afd1130c2a3e3a658b15dd873d676 Mon Sep 17 00:00:00 2001 From: schlndh Date: Fri, 2 May 2025 08:47:38 +0200 Subject: [PATCH 5/9] replace match with switch --- src/Rules/Functions/PrintfHelper.php | 42 ++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 12 deletions(-) diff --git a/src/Rules/Functions/PrintfHelper.php b/src/Rules/Functions/PrintfHelper.php index 707f6015f0..3d679f1193 100644 --- a/src/Rules/Functions/PrintfHelper.php +++ b/src/Rules/Functions/PrintfHelper.php @@ -14,6 +14,7 @@ use function array_map; use function array_reduce; use function count; +use function in_array; use function max; use function sort; use function sprintf; @@ -64,14 +65,24 @@ public function getPrintfPlaceholderAcceptingTypes(string $format): array $typeName, static function (Type $t) use ($types): bool { foreach ($types as $acceptingType) { - $subresult = match ($acceptingType) { - 'strict-int' => (new IntegerType())->accepts($t, true)->yes(), - 'int' => ! $t->toInteger() instanceof ErrorType, - 'float' => ! $t->toFloat() instanceof ErrorType, + 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. - 'string', 'mixed' => true, - }; + case 'string': + case 'mixed': + default: + $subresult = true; + break; + } if (!$subresult) { return false; @@ -149,12 +160,19 @@ private function parsePlaceholders(string $specifiersPattern, string $format): a /** @phpstan-return 'string'|'int'|'float'|'mixed' */ private function getAcceptingTypeBySpecifier(string $specifier): string { - return match ($specifier) { - 's' => 'string', - 'd', 'u', 'c', 'o', 'x', 'X', 'b' => 'int', - 'e', 'E', 'f', 'F', 'g', 'G', 'h', 'H' => 'float', - default => 'mixed', - }; + 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 'mixed'; } private function getPlaceholdersCount(string $specifiersPattern, string $format): int From dd2ac67cf32e2d59491001eeae5cf9417314b21d Mon Sep 17 00:00:00 2001 From: schlndh Date: Sun, 31 Aug 2025 14:25:54 +0200 Subject: [PATCH 6/9] fix printf type rule error messages --- src/Rules/Functions/PrintfHelper.php | 120 +++++------------- .../Functions/PrintfParameterTypeRule.php | 82 ++++++------ src/Rules/Functions/PrintfPlaceholder.php | 40 ++++++ .../Functions/PrintfParameterTypeRuleTest.php | 42 +++--- .../Functions/data/printf-param-types.php | 2 + 5 files changed, 150 insertions(+), 136 deletions(-) create mode 100644 src/Rules/Functions/PrintfPlaceholder.php diff --git a/src/Rules/Functions/PrintfHelper.php b/src/Rules/Functions/PrintfHelper.php index 3d679f1193..2afa6228e9 100644 --- a/src/Rules/Functions/PrintfHelper.php +++ b/src/Rules/Functions/PrintfHelper.php @@ -5,21 +5,13 @@ use Nette\Utils\Strings; use PHPStan\DependencyInjection\AutowiredService; 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' */ @@ -38,63 +30,10 @@ public function getPrintfPlaceholdersCount(string $format): int return $this->getPlaceholdersCount(self::PRINTF_SPECIFIER_PATTERN, $format); } - /** @return array position => [type name, matches callback] */ - public function getPrintfPlaceholderAcceptingTypes(string $format): array + /** @phpstan-return array> parameter index => placeholders */ + public function getPrintfPlaceholders(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; + return $this->parsePlaceholders(self::PRINTF_SPECIFIER_PATTERN, $format); } public function getScanfPlaceholdersCount(string $format): int @@ -102,7 +41,7 @@ public function getScanfPlaceholdersCount(string $format): int return $this->getPlaceholdersCount('(?[cdDeEfinosuxX%s]|\[[^\]]+\])', $format); } - /** @phpstan-return array> position => type */ + /** @phpstan-return array> parameter index => placeholders */ private function parsePlaceholders(string $specifiersPattern, string $format): array { $addSpecifier = ''; @@ -123,38 +62,49 @@ private function parsePlaceholders(string $specifiersPattern, string $format): a $placeholders = array_filter($matches, static fn (array $match): bool => strlen($match['before']) % 2 === 0); $result = []; - $positionalPlaceholders = []; - $idx = 0; + $parsedPlaceholders = []; + $parameterIdx = 0; + $placeholderNumber = 0; foreach ($placeholders as $placeholder) { + $placeholderNumber++; + $showValueSuffix = false; + if (isset($placeholder['width']) && $placeholder['width'] !== '') { - $result[$idx++] = ['strict-int' => 1]; + $parsedPlaceholders[] = new PrintfPlaceholder( + sprintf('"%s" (width)', $placeholder[0]), + $parameterIdx++, + $placeholderNumber, + 'strict-int', + ); + $showValueSuffix = true; } if (isset($placeholder['precision']) && $placeholder['precision'] !== '') { - $result[$idx++] = ['strict-int' => 1]; - } - - if (isset($placeholder['position']) && $placeholder['position'] !== '') { - // It may reference future position, so we have to process them later. - $positionalPlaceholders[] = $placeholder; - continue; + $parsedPlaceholders[] = new PrintfPlaceholder( + sprintf('"%s" (precision)', $placeholder[0]), + $parameterIdx++, + $placeholderNumber, + 'strict-int', + ); + $showValueSuffix = true; } - $result[$idx++][$this->getAcceptingTypeBySpecifier($placeholder['specifier'] ?? '')] = 1; + $parsedPlaceholders[] = new PrintfPlaceholder( + sprintf('"%s"', $placeholder[0]) . ($showValueSuffix ? ' (value)' : ''), + isset($placeholder['position']) && $placeholder['position'] !== '' + ? $placeholder['position'] - 1 + : $parameterIdx++, + $placeholderNumber, + $this->getAcceptingTypeBySpecifier($placeholder['specifier'] ?? ''), + ); } - 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; + foreach ($parsedPlaceholders as $placeholder) { + $result[$placeholder->parameterIndex][] = $placeholder; } - return array_map(static fn (array $a) => array_keys($a), $result); + return $result; } /** @phpstan-return 'string'|'int'|'float'|'mixed' */ diff --git a/src/Rules/Functions/PrintfParameterTypeRule.php b/src/Rules/Functions/PrintfParameterTypeRule.php index 0821afb019..67ff11855c 100644 --- a/src/Rules/Functions/PrintfParameterTypeRule.php +++ b/src/Rules/Functions/PrintfParameterTypeRule.php @@ -14,6 +14,7 @@ use PHPStan\Type\IntegerType; use PHPStan\Type\NullType; use PHPStan\Type\StringAlwaysAcceptingObjectWithToStringType; +use PHPStan\Type\Type; use PHPStan\Type\TypeCombinator; use PHPStan\Type\VerbosityLevel; use function array_key_exists; @@ -89,7 +90,7 @@ public function processNode(Node $node, Scope $scope): array $formatString = $formatArgTypeStrings[0]; $format = $formatString->getValue(); - $acceptingTypes = $this->printfHelper->getPrintfPlaceholderAcceptingTypes($format); + $placeholderMap = $this->printfHelper->getPrintfPlaceholders($format); $errors = []; $typeAllowedByCallToFunctionParametersRule = TypeCombinator::union( new StringAlwaysAcceptingObjectWithToStringType(), @@ -98,45 +99,54 @@ public function processNode(Node $node, Scope $scope): array 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. - if (! array_key_exists($j, $acceptingTypes)) { - continue; + 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->acceptedType], + $placeholder->placeholderNumber, + $placeholder->label, + $argType->describe(VerbosityLevel::typeOnly()), + ), + )->identifier('argument.type')->build(); } - - [$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/src/Rules/Functions/PrintfPlaceholder.php b/src/Rules/Functions/PrintfPlaceholder.php new file mode 100644 index 0000000000..24fe4fdd5b --- /dev/null +++ b/src/Rules/Functions/PrintfPlaceholder.php @@ -0,0 +1,40 @@ +acceptedType) { + 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': + default: + return true; + } + } + +} diff --git a/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php b/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php index 7fc45e4385..1259a4cd53 100644 --- a/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php +++ b/tests/PHPStan/Rules/Functions/PrintfParameterTypeRuleTest.php @@ -37,65 +37,77 @@ public function test(): void { $this->analyse([__DIR__ . '/data/printf-param-types.php'], [ [ - 'Placeholder #1 of function printf expects int, PrintfParamTypes\\FooStringable given', + 'Parameter #2 of function printf is expected to be castable to int by placeholder #1 ("%d"), PrintfParamTypes\\FooStringable given.', 15, ], [ - 'Placeholder #1 of function printf expects int, int|PrintfParamTypes\\FooStringable given', + 'Parameter #2 of function printf is expected to be castable to int by placeholder #1 ("%d"), int|PrintfParamTypes\\FooStringable given.', 16, ], [ - 'Placeholder #1 of function printf expects float, PrintfParamTypes\\FooStringable given', + 'Parameter #2 of function printf is expected to be castable to float by placeholder #1 ("%f"), PrintfParamTypes\\FooStringable given.', 17, ], [ - 'Placeholder #1 of function sprintf expects int, PrintfParamTypes\\FooStringable given', + 'Parameter #2 of function sprintf is expected to be castable to int by placeholder #1 ("%d"), PrintfParamTypes\\FooStringable given.', 18, ], [ - 'Placeholder #1 of function fprintf expects float, PrintfParamTypes\\FooStringable given', + 'Parameter #3 of function fprintf is expected to be castable to float by placeholder #1 ("%f"), PrintfParamTypes\\FooStringable given.', 19, ], [ - 'Placeholder #1 of function printf expects int, string given', + 'Parameter #2 of function printf is expected to be int by placeholder #1 ("%*s" (width)), string given.', 20, ], [ - 'Placeholder #1 of function printf expects int, float given', + 'Parameter #2 of function printf is expected to be int by placeholder #1 ("%*s" (width)), float given.', 21, ], [ - 'Placeholder #1 of function printf expects int, SimpleXMLElement given', + 'Parameter #2 of function printf is expected to be int by placeholder #1 ("%*s" (width)), SimpleXMLElement given.', 22, ], [ - 'Placeholder #1 of function printf expects int, null given', + 'Parameter #2 of function printf is expected to be int by placeholder #1 ("%*s" (width)), null given.', 23, ], [ - 'Placeholder #1 of function printf expects int, true given', + 'Parameter #2 of function printf is expected to be int by placeholder #1 ("%*s" (width)), true given.', 24, ], [ - 'Placeholder #1 of function printf expects int, string given', + 'Parameter #2 of function printf is expected to be int by placeholder #1 ("%.*s" (precision)), string given.', 25, ], [ - 'Placeholder #1 of function printf expects int, string given', + 'Parameter #2 of function printf is expected to be int by placeholder #2 ("%3$.*s" (precision)), string given.', 26, ], [ - 'Placeholder #1 of function printf expects float, PrintfParamTypes\\FooStringable given', + 'Parameter #2 of function printf is expected to be castable to float by placeholder #1 ("%1$-\'X10.2f"), PrintfParamTypes\\FooStringable given.', 27, ], [ - 'Placeholder #1 of function printf expects float, PrintfParamTypes\\FooStringable given', + 'Parameter #2 of function printf is expected to be castable to float by placeholder #2 ("%1$*.*f" (value)), PrintfParamTypes\\FooStringable given.', 28, ], [ - 'Placeholder #3 of function printf expects float, PrintfParamTypes\\FooStringable given', + '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 index 6c1c2142af..b2c5748627 100644 --- a/tests/PHPStan/Rules/Functions/data/printf-param-types.php +++ b/tests/PHPStan/Rules/Functions/data/printf-param-types.php @@ -27,6 +27,8 @@ public function __toString(): string 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); From 6c3114f674caa23068fc0e94c7189d831973040f Mon Sep 17 00:00:00 2001 From: schlndh Date: Sun, 31 Aug 2025 14:36:02 +0200 Subject: [PATCH 7/9] remove unused phpdoc type --- src/Rules/Functions/PrintfHelper.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/Rules/Functions/PrintfHelper.php b/src/Rules/Functions/PrintfHelper.php index 2afa6228e9..73cb4af220 100644 --- a/src/Rules/Functions/PrintfHelper.php +++ b/src/Rules/Functions/PrintfHelper.php @@ -14,7 +14,6 @@ use function strlen; use const PREG_SET_ORDER; -/** @phpstan-type AcceptingTypeString 'strict-int'|'int'|'float'|'string'|'mixed' */ #[AutowiredService] final class PrintfHelper { From 5d8709e98532f4ef1ab13d012103a4cc359bec40 Mon Sep 17 00:00:00 2001 From: schlndh Date: Fri, 5 Sep 2025 13:00:56 +0200 Subject: [PATCH 8/9] fix CR issues --- src/Rules/Functions/PrintfParameterTypeRule.php | 2 +- src/Rules/Functions/PrintfPlaceholder.php | 7 +++---- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/src/Rules/Functions/PrintfParameterTypeRule.php b/src/Rules/Functions/PrintfParameterTypeRule.php index 67ff11855c..8e2997c67f 100644 --- a/src/Rules/Functions/PrintfParameterTypeRule.php +++ b/src/Rules/Functions/PrintfParameterTypeRule.php @@ -140,7 +140,7 @@ public function processNode(Node $node, Scope $scope): array 'Parameter #%d of function %s is expected to be %s by placeholder #%d (%s), %s given.', $i + 1, $name, - $allowedTypeNameMap[$placeholder->acceptedType], + $allowedTypeNameMap[$placeholder->acceptingType], $placeholder->placeholderNumber, $placeholder->label, $argType->describe(VerbosityLevel::typeOnly()), diff --git a/src/Rules/Functions/PrintfPlaceholder.php b/src/Rules/Functions/PrintfPlaceholder.php index 24fe4fdd5b..0bac1097e3 100644 --- a/src/Rules/Functions/PrintfPlaceholder.php +++ b/src/Rules/Functions/PrintfPlaceholder.php @@ -9,19 +9,19 @@ final class PrintfPlaceholder { - /** @phpstan-param 'strict-int'|'int'|'float'|'string'|'mixed' $acceptedType */ + /** @phpstan-param 'strict-int'|'int'|'float'|'string'|'mixed' $acceptingType */ public function __construct( public readonly string $label, public readonly int $parameterIndex, public readonly int $placeholderNumber, - public readonly string $acceptedType, + public readonly string $acceptingType, ) { } public function doesArgumentTypeMatchPlaceholder(Type $argumentType): bool { - switch ($this->acceptedType) { + switch ($this->acceptingType) { case 'strict-int': return (new IntegerType())->accepts($argumentType, true)->yes(); case 'int': @@ -32,7 +32,6 @@ public function doesArgumentTypeMatchPlaceholder(Type $argumentType): bool // no point in checking string again here. case 'string': case 'mixed': - default: return true; } } From 89641e5bfb739106ce54a9ece493cd86463c82eb Mon Sep 17 00:00:00 2001 From: schlndh Date: Fri, 5 Sep 2025 13:26:42 +0200 Subject: [PATCH 9/9] fix static analysis job with PHP 7.4 --- src/Rules/Functions/PrintfPlaceholder.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Rules/Functions/PrintfPlaceholder.php b/src/Rules/Functions/PrintfPlaceholder.php index 0bac1097e3..8e2c1336fa 100644 --- a/src/Rules/Functions/PrintfPlaceholder.php +++ b/src/Rules/Functions/PrintfPlaceholder.php @@ -2,6 +2,7 @@ namespace PHPStan\Rules\Functions; +use PHPStan\ShouldNotHappenException; use PHPStan\Type\ErrorType; use PHPStan\Type\IntegerType; use PHPStan\Type\Type; @@ -33,6 +34,10 @@ public function doesArgumentTypeMatchPlaceholder(Type $argumentType): bool 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); } }