Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix PHP 7 sprintf too many arguments false positive #9943

Merged
merged 10 commits into from
Jun 24, 2023
Merged

fix PHP 7 sprintf too many arguments false positive #9943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
151f101
fix PHP 7 sprintf too many arguments false positive
kkmuffme Jun 21, 2023
f2a9e09
reorganize code for PHP 7 to also check for invalid argument and redu…
kkmuffme Jun 21, 2023
fad1768
Fix false positive for array unpacking
kkmuffme Jun 21, 2023
52ca871
fix try/catch variable must be initialized
kkmuffme Jun 21, 2023
9599c24
code style
kkmuffme Jun 21, 2023
28b9e8d
run format checks for splat too
kkmuffme Jun 21, 2023
7f0217d
fix incorrect default stubs
kkmuffme Jun 24, 2023
c5fee53
consistently ignore falsable return, remove unused suppress with new …
kkmuffme Jun 24, 2023
0535c6b
PHP 7 format with only placeholders isn't falsable if valid
kkmuffme Jun 24, 2023
d4dcee3
code style
kkmuffme Jun 24, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
121 changes: 67 additions & 54 deletions src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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