Skip to content

Commit

Permalink
Merge pull request #9943 from kkmuffme/sprintf-php7-false-positive
Browse files Browse the repository at this point in the history 
fix PHP 7 sprintf too many arguments false positive
  • Loading branch information
@orklah
orklah authored Jun 24, 2023
2 parents b9a7c37 + d4dcee3 commit a0a9c27
Show file tree
Hide file tree
Showing 3 changed files with 126 additions and 60 deletions.
121 changes: 67 additions & 54 deletions src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
use function array_fill;
use function array_pop;
use function count;
use function is_string;
use function preg_match;
use function sprintf;

Expand Down Expand Up @@ -61,7 +62,24 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev
return null;
}

$has_splat_args = false;
$node_type_provider = $statements_source->getNodeTypeProvider();
foreach ($call_args as $call_arg) {
$type = $node_type_provider->getType($call_arg->value);
if ($type === null) {
continue;
}

// if it's an array, used with splat operator
// we cannot validate it reliably below and report false positive errors
if ($type->isArray()) {
$has_splat_args = true;
break;
}
}

// PHP 7 handling for formats that do not contain anything but placeholders
$is_falsable = true;
foreach ($call_args as $index => $call_arg) {
$type = $node_type_provider->getType($call_arg->value);
if ($type === null && $index === 0 && $event->getFunctionId() === 'printf') {
Expand Down Expand Up @@ -114,21 +132,17 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev
'/%(?:\d+\$)?[-+]?(?:\d+|\*)(?:\.(?:\d+|\*))?[bcdouxXeEfFgGhHs]/',
$type->getSingleStringLiteral()->value,
) === 1) {
if ($event->getFunctionId() === 'printf') {
return null;
}

// the core stubs are wrong for these too, since these might be empty strings
// e.g. sprintf(\'%0.*s\', 0, "abc")
return Type::getString();
return null;
}

$args_count = count($call_args) - 1;
$dummy = array_fill(0, $args_count, '');
// assume a random, high number for tests
$provided_placeholders_count = $has_splat_args === true ? 100 : count($call_args) - 1;
$dummy = array_fill(0, $provided_placeholders_count, '');

// check if we have enough/too many arguments and a valid format
$initial_result = null;
while (count($dummy) > -1) {
$result = null;
try {
// before PHP 8, an uncatchable Warning is thrown if too few arguments are passed
// which is ignored and handled below instead
Expand Down Expand Up @@ -166,7 +180,7 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev
break 2;
} catch (ArgumentCountError $error) {
// PHP 8
if (count($dummy) >= $args_count) {
if (count($dummy) === $provided_placeholders_count) {
IssueBuffer::maybeAdd(
new TooFewArguments(
'Too few arguments for ' . $event->getFunctionId(),
Expand All @@ -178,49 +192,43 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev

break 2;
}
}

// we are in the next iteration, so we have 1 placeholder less here
// otherwise we would have reported an error above already
if (count($dummy) + 1 === $args_count) {
break;
if ($result === false && count($dummy) === $provided_placeholders_count) {
// could be invalid format or too few arguments
// we cannot distinguish this in PHP 7 without additional checks
$max_dummy = array_fill(0, 100, '');
$result = @sprintf($type->getSingleStringLiteral()->value, ...$max_dummy);
if ($result === false) {
// the format is invalid
IssueBuffer::maybeAdd(
new InvalidArgument(
'Argument 1 of ' . $event->getFunctionId() . ' is invalid',
$event->getCodeLocation(),
$event->getFunctionId(),
),
$statements_source->getSuppressedIssues(),
);
} else {
IssueBuffer::maybeAdd(
new TooFewArguments(
'Too few arguments for ' . $event->getFunctionId(),
$event->getCodeLocation(),
$event->getFunctionId(),
),
$statements_source->getSuppressedIssues(),
);
}

IssueBuffer::maybeAdd(
new TooManyArguments(
'Too many arguments for the number of placeholders in ' . $event->getFunctionId(),
$event->getCodeLocation(),
$event->getFunctionId(),
),
$statements_source->getSuppressedIssues(),
);

break;
return Type::getFalse();
}

/**
* PHP 7
*
* @psalm-suppress DocblockTypeContradiction
*/
if ($result === false && count($dummy) >= $args_count) {
IssueBuffer::maybeAdd(
new TooFewArguments(
'Too few arguments for ' . $event->getFunctionId(),
$event->getCodeLocation(),
$event->getFunctionId(),
),
$statements_source->getSuppressedIssues(),
);

return Type::getFalse();
// we can only validate the format and arg 1 when using splat
if ($has_splat_args === true) {
break;
}

/**
* PHP 7
*
* @psalm-suppress DocblockTypeContradiction
*/
if ($result === false && count($dummy) + 1 !== $args_count) {
if (is_string($result) && count($dummy) + 1 <= $provided_placeholders_count) {
IssueBuffer::maybeAdd(
new TooManyArguments(
'Too many arguments for the number of placeholders in ' . $event->getFunctionId(),
Expand All @@ -233,7 +241,10 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev
break;
}

// for PHP 7, since it doesn't throw above
if (!is_string($result)) {
break;
}

// abort if it's empty, since we checked everything
if (array_pop($dummy) === null) {
break;
Expand All @@ -246,20 +257,19 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev
return null;
}

/**
* PHP 7 can have false here
*
* @psalm-suppress RedundantConditionGivenDocblockType
*/
if ($initial_result !== null && $initial_result !== false && $initial_result !== '') {
return Type::getNonEmptyString();
}

// if we didn't have a valid result
// if we didn't have any valid result
// the pattern is invalid or not yet supported by the return type provider
if ($initial_result === null || $initial_result === false) {
return null;
}

// the initial result is an empty string
// which means the format is valid and it depends on the args, whether it is non-empty-string or not
$is_falsable = false;
}

if ($index === 0 && $event->getFunctionId() === 'printf') {
Expand All @@ -274,7 +284,6 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev

// if the function has more arguments than the pattern has placeholders, this could be a false positive
// if the param is not used in the pattern
// however this is already reported above and returned, so this cannot happen
if ($type->isNonEmptyString() || $type->isInt() || $type->isFloat()) {
return Type::getNonEmptyString();
}
Expand Down Expand Up @@ -304,6 +313,10 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev
return Type::getNonEmptyString();
}

if ($is_falsable === false) {
return Type::getString();
}

return null;
}
}
13 changes: 7 additions & 6 deletions stubs/CoreGenericFunctions.phpstub
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1279,9 +1279,8 @@ function preg_quote(string $str, ?string $delimiter = null) : string {}
* @psalm-pure
*
* @param string|int|float $values
* @return ($format is non-empty-string
* ? ($values is non-empty-string|int|float ? non-empty-string : string)
* : string)
* @return (PHP_MAJOR_VERSION is 8 ? string : string|false)
* @psalm-ignore-falsable-return
*
* @psalm-flow ($format, $values) -> return
*/
Expand All @@ -1290,7 +1289,7 @@ function sprintf(string $format, ...$values) {}
/**
* @psalm-pure
* @param array<string|int|float> $values
* @return string|false
* @return (PHP_MAJOR_VERSION is 8 ? string : string|false)
* @psalm-ignore-falsable-return
*
* @psalm-flow ($format, $values) -> return
Expand All @@ -1309,7 +1308,8 @@ function wordwrap(string $string, int $width = 75, string $break = "\n", bool $c
* @psalm-pure
*
* @param string|int|float $values
* @return int<0, max>
* @return (PHP_MAJOR_VERSION is 8 ? int<0, max> : int<0, max>|false)
* @psalm-ignore-falsable-return
*
* @psalm-taint-specialize
* @psalm-flow ($format, $values) -> return
Expand All @@ -1320,7 +1320,8 @@ function printf(string $format, ...$values) {}

/**
* @param array<string|int|float> $values
* @return int<0, max>
* @return (PHP_MAJOR_VERSION is 8 ? int<0, max> : int<0, max>|false)
* @psalm-ignore-falsable-return
*
* @psalm-pure
* @psalm-taint-specialize
Expand Down
52 changes: 52 additions & 0 deletions tests/ReturnTypeProvider/SprintfTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,8 @@ public function providerValidCodeParse(): iterable
'assertions' => [
'$val===' => 'int<0, max>',
],
'ignored_issues' => [],
'php_version' => '8.0',
];

yield 'sprintfEmptyStringFormat' => [
Expand Down Expand Up @@ -163,6 +165,8 @@ public function providerValidCodeParse(): iterable
'assertions' => [
'$val===' => 'string',
],
'ignored_issues' => [],
'php_version' => '8.0',
];

yield 'sprintfComplexPlaceholderNotYetSupported2' => [
Expand All @@ -172,6 +176,8 @@ public function providerValidCodeParse(): iterable
'assertions' => [
'$val===' => 'string',
],
'ignored_issues' => [],
'php_version' => '8.0',
];

yield 'sprintfComplexPlaceholderNotYetSupported3' => [
Expand All @@ -181,6 +187,52 @@ public function providerValidCodeParse(): iterable
'assertions' => [
'$val===' => 'string',
],
'ignored_issues' => [],
'php_version' => '8.0',
];

yield 'sprintfSplatUnpackingArray' => [
'code' => '<?php
$a = ["a", "b", "c"];
$val = sprintf("%s%s%s", ...$a);
',
'assertions' => [
'$val===' => 'string',
],
'ignored_issues' => [],
'php_version' => '8.0',
];

yield 'sprintfSplatUnpackingArrayNonEmpty' => [
'code' => '<?php
$a = ["a", "b", "c"];
$val = sprintf("%s %s %s", ...$a);
',
'assertions' => [
'$val===' => 'non-empty-string',
],
];

yield 'sprintfMultiplePlaceholdersNoErrorsIssue9941PHP7' => [
'code' => '<?php
$val = sprintf("Handling product %d => %d (%d)", 123, 456, 789);
',
'assertions' => [
'$val===' => 'non-empty-string',
],
'ignored_issues' => [],
'php_version' => '7.4',
];

yield 'sprintfMultiplePlaceholdersNoErrorsIssue9941PHP8' => [
'code' => '<?php
$val = sprintf("Handling product %d => %d (%d)", 123, 456, 789);
',
'assertions' => [
'$val===' => 'non-empty-string',
],
'ignored_issues' => [],
'php_version' => '8.0',
];
}

Expand Down

0 comments on commit a0a9c27

Please sign in to comment.