Skip to content

Commit

Permalink
Merge pull request #9877 from kkmuffme/sprintf-improve-return-param-t…
Browse files Browse the repository at this point in the history 
…ype-validation

Sprintf improve return param type validation
  • Loading branch information
@orklah
orklah committed Jun 11, 2023
2 parents 5df772b + 1465ed7 commit b6cba5a
Show file tree
Hide file tree
Showing 5 changed files with 235 additions and 22 deletions.
8 changes: 4 additions & 4 deletions dictionaries/CallMap.php
View file
Expand Up @@ -9404,7 +9404,7 @@
'print' => ['int', 'arg'=>'string'],
'print_r' => ['string', 'value'=>'mixed'],
'print_r\'1' => ['true', 'value'=>'mixed', 'return='=>'bool'],
'printf' => ['int', 'format'=>'string', '...values='=>'string|int|float'],
'printf' => ['int<0, max>', 'format'=>'string', '...values='=>'string|int|float'],
'proc_close' => ['int', 'process'=>'resource'],
'proc_get_status' => ['array{command: string, pid: int, running: bool, signaled: bool, stopped: bool, exitcode: int, termsig: int, stopsig: int}', 'process'=>'resource'],
'proc_nice' => ['bool', 'priority'=>'int'],
Expand Down Expand Up @@ -14365,7 +14365,7 @@
'VarnishStat::getSnapshot' => ['array'],
'version_compare' => ['bool', 'version1'=>'string', 'version2'=>'string', 'operator'=>'\'<\'|\'lt\'|\'<=\'|\'le\'|\'>\'|\'gt\'|\'>=\'|\'ge\'|\'==\'|\'=\'|\'eq\'|\'!=\'|\'<>\'|\'ne\''],
'version_compare\'1' => ['int', 'version1'=>'string', 'version2'=>'string'],
'vfprintf' => ['int', 'stream'=>'resource', 'format'=>'string', 'values'=>'array'],
'vfprintf' => ['int<0, max>', 'stream'=>'resource', 'format'=>'string', 'values'=>'array<string|int|float>'],
'virtual' => ['bool', 'uri'=>'string'],
'vpopmail_add_alias_domain' => ['bool', 'domain'=>'string', 'aliasdomain'=>'string'],
'vpopmail_add_alias_domain_ex' => ['bool', 'olddomain'=>'string', 'newdomain'=>'string'],
Expand All @@ -14384,8 +14384,8 @@
'vpopmail_error' => ['string'],
'vpopmail_passwd' => ['bool', 'user'=>'string', 'domain'=>'string', 'password'=>'string', 'apop='=>'bool'],
'vpopmail_set_user_quota' => ['bool', 'user'=>'string', 'domain'=>'string', 'quota'=>'string'],
'vprintf' => ['int', 'format'=>'string', 'values'=>'array'],
'vsprintf' => ['string', 'format'=>'string', 'values'=>'array'],
'vprintf' => ['int<0, max>', 'format'=>'string', 'values'=>'array<string|int|float>'],
'vsprintf' => ['string', 'format'=>'string', 'values'=>'array<string|int|float>'],
'Vtiful\Kernel\Chart::__construct' => ['void', 'handle'=>'resource', 'type'=>'int'],
'Vtiful\Kernel\Chart::axisNameX' => ['Vtiful\Kernel\Chart', 'name'=>'string'],
'Vtiful\Kernel\Chart::axisNameY' => ['Vtiful\Kernel\Chart', 'name'=>'string'],
Expand Down
8 changes: 4 additions & 4 deletions dictionaries/CallMap_historical.php
View file
Expand Up @@ -13555,7 +13555,7 @@
'print' => ['int', 'arg'=>'string'],
'print_r' => ['string', 'value'=>'mixed'],
'print_r\'1' => ['true', 'value'=>'mixed', 'return='=>'bool'],
'printf' => ['int', 'format'=>'string', '...values='=>'string|int|float'],
'printf' => ['int<0, max>', 'format'=>'string', '...values='=>'string|int|float'],
'proc_close' => ['int', 'process'=>'resource'],
'proc_get_status' => ['array{command: string, pid: int, running: bool, signaled: bool, stopped: bool, exitcode: int, termsig: int, stopsig: int}|false', 'process'=>'resource'],
'proc_nice' => ['bool', 'priority'=>'int'],
Expand Down Expand Up @@ -15318,7 +15318,7 @@
'variant_xor' => ['mixed', 'left'=>'mixed', 'right'=>'mixed'],
'version_compare' => ['bool', 'version1'=>'string', 'version2'=>'string', 'operator'=>'\'<\'|\'lt\'|\'<=\'|\'le\'|\'>\'|\'gt\'|\'>=\'|\'ge\'|\'==\'|\'=\'|\'eq\'|\'!=\'|\'<>\'|\'ne\''],
'version_compare\'1' => ['int', 'version1'=>'string', 'version2'=>'string'],
'vfprintf' => ['int', 'stream'=>'resource', 'format'=>'string', 'values'=>'array'],
'vfprintf' => ['int<0, max>', 'stream'=>'resource', 'format'=>'string', 'values'=>'array<string|int|float>'],
'virtual' => ['bool', 'uri'=>'string'],
'vpopmail_add_alias_domain' => ['bool', 'domain'=>'string', 'aliasdomain'=>'string'],
'vpopmail_add_alias_domain_ex' => ['bool', 'olddomain'=>'string', 'newdomain'=>'string'],
Expand All @@ -15337,8 +15337,8 @@
'vpopmail_error' => ['string'],
'vpopmail_passwd' => ['bool', 'user'=>'string', 'domain'=>'string', 'password'=>'string', 'apop='=>'bool'],
'vpopmail_set_user_quota' => ['bool', 'user'=>'string', 'domain'=>'string', 'quota'=>'string'],
'vprintf' => ['int', 'format'=>'string', 'values'=>'array'],
'vsprintf' => ['string', 'format'=>'string', 'values'=>'array'],
'vprintf' => ['int<0, max>', 'format'=>'string', 'values'=>'array<string|int|float>'],
'vsprintf' => ['string', 'format'=>'string', 'values'=>'array<string|int|float>'],
'w32api_deftype' => ['bool', 'typename'=>'string', 'member1_type'=>'string', 'member1_name'=>'string', '...args='=>'string'],
'w32api_init_dtype' => ['resource', 'typename'=>'string', 'value'=>'', '...args='=>''],
'w32api_invoke_function' => ['', 'funcname'=>'string', 'argument'=>'', '...args='=>''],
Expand Down
164 changes: 153 additions & 11 deletions src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php
View file
Expand Up @@ -2,6 +2,11 @@

namespace Psalm\Internal\Provider\ReturnTypeProvider;

use ArgumentCountError;
use Psalm\Issue\InvalidArgument;
use Psalm\Issue\TooFewArguments;
use Psalm\Issue\TooManyArguments;
use Psalm\IssueBuffer;
use Psalm\Plugin\EventHandler\Event\FunctionReturnTypeProviderEvent;
use Psalm\Plugin\EventHandler\FunctionReturnTypeProviderInterface;
use Psalm\Type;
Expand All @@ -12,8 +17,10 @@
use Psalm\Type\Atomic\TNonEmptyString;
use Psalm\Type\Atomic\TNumeric;
use Psalm\Type\Union;
use ValueError;

use function array_fill;
use function array_pop;
use function count;
use function sprintf;

Expand All @@ -28,40 +35,175 @@ class SprintfReturnTypeProvider implements FunctionReturnTypeProviderInterface
public static function getFunctionIds(): array
{
return [
'printf',
'sprintf',
];
}

public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $event): Union
public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $event): ?Union
{
$statements_source = $event->getStatementsSource();
$node_type_provider = $statements_source->getNodeTypeProvider();

$call_args = $event->getCallArgs();

// it makes no sense to use sprintf/printf when there is only 1 arg (the format)
// as it wouldn't have any placeholders
if (count($call_args) === 1) {
IssueBuffer::maybeAdd(
new TooFewArguments(
'Too few arguments for ' . $event->getFunctionId() . ', expecting at least 2 arguments',
$event->getCodeLocation(),
$event->getFunctionId(),
),
$statements_source->getSuppressedIssues(),
);

return null;
}

$node_type_provider = $statements_source->getNodeTypeProvider();
foreach ($call_args as $index => $call_arg) {
$type = $node_type_provider->getType($call_arg->value);
if ($type === null && $event->getFunctionId() === 'printf') {
break;
}

if ($type === null) {
continue;
}

if ($index === 0 && $type->isSingleStringLiteral()) {
// use empty string dummies to check if the format itself produces a non-empty return value
// faster than validating the pattern and checking all args separately
$dummy = array_fill(0, count($call_args) - 1, '');
if (sprintf($type->getSingleStringLiteral()->value, ...$dummy) !== '') {
$args_count = count($call_args) - 1;
$dummy = array_fill(0, $args_count, '');

// check if we have enough/too many arguments and a valid format
$initial_result = null;
while (count($dummy) > -1) {
try {
// before PHP 8, an uncatchable Warning is thrown if too few arguments are passed
// which is ignored and handled below instead
$result = @sprintf($type->getSingleStringLiteral()->value, ...$dummy);
if ($initial_result === null) {
$initial_result = $result;
}
} catch (ValueError $value_error) {
// PHP 8
// the format is invalid
IssueBuffer::maybeAdd(
new InvalidArgument(
'Argument 1 of ' . $event->getFunctionId() . ' is invalid - '
. $value_error->getMessage(),
$event->getCodeLocation(),
$event->getFunctionId(),
),
$statements_source->getSuppressedIssues(),
);

break 2;
} catch (ArgumentCountError $error) {
// PHP 8
if (count($dummy) >= $args_count) {
IssueBuffer::maybeAdd(
new TooFewArguments(
'Too few arguments for ' . $event->getFunctionId(),
$event->getCodeLocation(),
$event->getFunctionId(),
),
$statements_source->getSuppressedIssues(),
);

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;
}

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

break;
}

/**
* 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();
}

/**
* PHP 7
*
* @psalm-suppress DocblockTypeContradiction
*/
if ($result === false && count($dummy) + 1 !== $args_count) {
IssueBuffer::maybeAdd(
new TooManyArguments(
'Too many arguments for the number of placeholders in ' . $event->getFunctionId(),
$event->getCodeLocation(),
$event->getFunctionId(),
),
$statements_source->getSuppressedIssues(),
);

break;
}

// for PHP 7, since it doesn't throw above
// abort if it's empty, since we checked everything
if (array_pop($dummy) === null) {
break;
}
}

if ($event->getFunctionId() === 'printf') {
// printf only has the format validated above
// don't change the return type
return null;
}

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

if ($index === 0 && $event->getFunctionId() === 'printf') {
// printf only has the format validated above
// don't change the return type
break;
}

if ($index === 0) {
continue;
}

// 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 we would need to analyze the format arg to check that
// can be done eventually to also implement https://github.com/vimeo/psalm/issues/9818
// and https://github.com/vimeo/psalm/issues/9817
// 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 @@ -91,6 +233,6 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev
return Type::getNonEmptyString();
}

return Type::getString();
return null;
}
}
12 changes: 9 additions & 3 deletions stubs/CoreGenericFunctions.phpstub
View file
Expand Up @@ -1285,10 +1285,11 @@ function preg_quote(string $str, ?string $delimiter = null) : string {}
*
* @psalm-flow ($format, $values) -> return
*/
function sprintf(string $format, ...$values) : string {}
function sprintf(string $format, ...$values) {}

/**
* @psalm-pure
* @param array<string|int|float> $values
* @return string|false
* @psalm-ignore-falsable-return
*
Expand All @@ -1308,20 +1309,25 @@ function wordwrap(string $string, int $width = 75, string $break = "\n", bool $c
* @psalm-pure
*
* @param string|int|float $values
* @return int<0, max>
*
* @psalm-taint-specialize
* @psalm-flow ($format, $values) -> return
* @psalm-taint-sink html $format
* @psalm-taint-sink html $values
*/
function printf(string $format, ...$values) : string {}
function printf(string $format, ...$values) {}

/**
* @param array<string|int|float> $values
* @return int<0, max>
*
* @psalm-pure
* @psalm-taint-specialize
* @psalm-taint-sink html $format
* @psalm-taint-sink html $values
*/
function vprintf(string $format, array $values) : int {}
function vprintf(string $format, array $values) {}

/**
* @psalm-pure
Expand Down
65 changes: 65 additions & 0 deletions tests/ReturnTypeProvider/SprintfTest.php
View file
Expand Up @@ -3,10 +3,12 @@
namespace Psalm\Tests\ReturnTypeProvider;

use Psalm\Tests\TestCase;
use Psalm\Tests\Traits\InvalidCodeAnalysisTestTrait;
use Psalm\Tests\Traits\ValidCodeAnalysisTestTrait;

class SprintfTest extends TestCase
{
use InvalidCodeAnalysisTestTrait;
use ValidCodeAnalysisTestTrait;

public function providerValidCodeParse(): iterable
Expand Down Expand Up @@ -120,5 +122,68 @@ public function providerValidCodeParse(): iterable
'$val===' => 'string',
],
];

yield 'printfSimple' => [
'code' => '<?php
$val = printf("%s", "hello");
',
'assertions' => [
'$val===' => 'int<0, max>',
],
];
}

public function providerInvalidCodeParse(): iterable
{
return [
'sprintfOnlyFormat' => [
'code' => '<?php
$x = sprintf("hello");
',
'error_message' => 'TooFewArguments',
],
'sprintfTooFewArguments' => [
'code' => '<?php
$x = sprintf("%s hello %d", "a");
',
'error_message' => 'TooFewArguments',
],
'sprintfTooManyArguments' => [
'code' => '<?php
$x = sprintf("%s hello", "a", "b");
',
'error_message' => 'TooManyArguments',
],
'sprintfInvalidFormat' => [
'code' => '<?php
$x = sprintf(\'"%" hello\', "a");
',
'error_message' => 'InvalidArgument',
],
'printfOnlyFormat' => [
'code' => '<?php
printf("hello");
',
'error_message' => 'TooFewArguments',
],
'printfTooFewArguments' => [
'code' => '<?php
printf("%s hello %d", "a");
',
'error_message' => 'TooFewArguments',
],
'printfTooManyArguments' => [
'code' => '<?php
printf("%s hello", "a", "b");
',
'error_message' => 'TooManyArguments',
],
'printfInvalidFormat' => [
'code' => '<?php
printf(\'"%" hello\', "a");
',
'error_message' => 'InvalidArgument',
],
];
}
}

0 comments on commit b6cba5a

Please sign in to comment.