Skip to content

Commit

Permalink
Merge pull request #10088 from kkmuffme/sprintf-error-reporting-more-…
Browse files Browse the repository at this point in the history 
…correct-literal

make (s)printf error reporting more correct/literal
  • Loading branch information
@orklah
orklah authored Aug 5, 2023
2 parents 2c39046 + 02385c2 commit 96a61fc
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 24 deletions.
88 changes: 70 additions & 18 deletions src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

use ArgumentCountError;
use Psalm\Issue\InvalidArgument;
use Psalm\Issue\RedundantFunctionCall;
use Psalm\Issue\TooFewArguments;
use Psalm\Issue\TooManyArguments;
use Psalm\IssueBuffer;
Expand All @@ -25,6 +26,7 @@
use function is_string;
use function preg_match;
use function sprintf;
use function strlen;

/**
* @internal
Expand All @@ -47,6 +49,11 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev
$statements_source = $event->getStatementsSource();
$call_args = $event->getCallArgs();

// invalid - will already report an error for the params anyway
if (count($call_args) < 1) {
return null;
}

$has_splat_args = false;
$node_type_provider = $statements_source->getNodeTypeProvider();
foreach ($call_args as $call_arg) {
Expand All @@ -67,17 +74,29 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev
// eventually this could be refined
// to check if it's an array with literal string as first element for further checking
if (count($call_args) === 1 && $has_splat_args === true) {
IssueBuffer::maybeAdd(
new RedundantFunctionCall(
'Using the splat operator is redundant, as v' . $event->getFunctionId()
. ' without splat operator can be used instead of ' . $event->getFunctionId(),
$event->getCodeLocation(),
),
$statements_source->getSuppressedIssues(),
);

return null;
}

// it makes no sense to use sprintf when there is only 1 arg (the format)
// as it wouldn't have any placeholders
if (count($call_args) === 1 && $event->getFunctionId() === 'sprintf') {
// if it's a literal string, we can check it further though!
$first_arg_type = $node_type_provider->getType($call_args[0]->value);
if (count($call_args) === 1
&& ($first_arg_type === null || !$first_arg_type->isSingleStringLiteral())) {
IssueBuffer::maybeAdd(
new TooFewArguments(
'Too few arguments for ' . $event->getFunctionId() . ', expecting at least 2 arguments',
new RedundantFunctionCall(
'Using ' . $event->getFunctionId()
. ' with a single argument is redundant, since there are no placeholder params to be substituted',
$event->getCodeLocation(),
$event->getFunctionId(),
),
$statements_source->getSuppressedIssues(),
);
Expand All @@ -89,7 +108,10 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev
$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') {
// printf only has the format validated above
// don't change the return type
break;
}

Expand All @@ -100,10 +122,9 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev
if ($index === 0 && $type->isSingleStringLiteral()) {
if ($type->getSingleStringLiteral()->value === '') {
IssueBuffer::maybeAdd(
new InvalidArgument(
'Argument 1 of ' . $event->getFunctionId() . ' must not be an empty string',
new RedundantFunctionCall(
'Calling ' . $event->getFunctionId() . ' with an empty first argument does nothing',
$event->getCodeLocation(),
$event->getFunctionId(),
),
$statements_source->getSuppressedIssues(),
);
Expand Down Expand Up @@ -158,17 +179,48 @@ public static function getFunctionReturnType(FunctionReturnTypeProviderEvent $ev
$initial_result = $result;

if ($result === $type->getSingleStringLiteral()->value) {
IssueBuffer::maybeAdd(
new InvalidArgument(
'Argument 1 of ' . $event->getFunctionId()
. ' does not contain any placeholders',
$event->getCodeLocation(),
$event->getFunctionId(),
),
$statements_source->getSuppressedIssues(),
);

return null;
if (count($call_args) > 1) {
// we need to report this here too, since we return early without further validation
// otherwise people who have suspended RedundantFunctionCall errors
// will not get an error for this
IssueBuffer::maybeAdd(
new TooManyArguments(
'Too many arguments for the number of placeholders in '
. $event->getFunctionId(),
$event->getCodeLocation(),
$event->getFunctionId(),
),
$statements_source->getSuppressedIssues(),
);
}

// the same error as above, but we have validated the pattern now
if (count($call_args) === 1) {
IssueBuffer::maybeAdd(
new RedundantFunctionCall(
'Using ' . $event->getFunctionId()
. ' with a single argument is redundant,'
. ' since there are no placeholder params to be substituted',
$event->getCodeLocation(),
),
$statements_source->getSuppressedIssues(),
);
} else {
IssueBuffer::maybeAdd(
new RedundantFunctionCall(
'Argument 1 of ' . $event->getFunctionId()
. ' does not contain any placeholders',
$event->getCodeLocation(),
),
$statements_source->getSuppressedIssues(),
);
}

if ($event->getFunctionId() === 'printf') {
return Type::getInt(false, strlen($type->getSingleStringLiteral()->value));
}

return $type;
}
}
} catch (ValueError $value_error) {
Expand Down
30 changes: 24 additions & 6 deletions tests/ReturnTypeProvider/SprintfTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ public function providerValidCodeParse(): iterable
'$val===' => '\'\'',
],
'ignored_issues' => [
'InvalidArgument',
'RedundantFunctionCall',
],
];

Expand Down Expand Up @@ -221,7 +221,9 @@ public function providerValidCodeParse(): iterable
'assertions' => [
'$val===' => 'string',
],
'ignored_issues' => [],
'ignored_issues' => [
'RedundantFunctionCall',
],
'php_version' => '8.0',
];

Expand Down Expand Up @@ -251,11 +253,17 @@ public function providerValidCodeParse(): iterable
public function providerInvalidCodeParse(): iterable
{
return [
'sprintfOnlyFormat' => [
'sprintfOnlyFormatWithoutPlaceholders' => [
'code' => '<?php
$x = sprintf("hello");
',
'error_message' => 'TooFewArguments',
'error_message' => 'RedundantFunctionCall',
],
'printfOnlyFormatWithoutPlaceholders' => [
'code' => '<?php
$x = sprintf("hello");
',
'error_message' => 'RedundantFunctionCall',
],
'sprintfTooFewArguments' => [
'code' => '<?php
Expand Down Expand Up @@ -297,20 +305,30 @@ public function providerInvalidCodeParse(): iterable
'code' => '<?php
$x = sprintf("", "abc");
',
'error_message' => 'InvalidArgument',
'error_message' => 'RedundantFunctionCall',
],
'sprintfFormatWithoutPlaceholders' => [
'code' => '<?php
$x = sprintf("hello", "abc");
',
'error_message' => 'InvalidArgument',
'error_message' => 'TooManyArguments',
'ignored_issues' => [
'RedundantFunctionCall',
],
],
'sprintfPaddedComplexEmptyStringFormat' => [
'code' => '<?php
$x = sprintf("%1$+0.0s", "abc");
',
'error_message' => 'InvalidArgument',
],
'printfVariableFormat' => [
'code' => '<?php
/** @var string $bar */
printf($bar);
',
'error_message' => 'RedundantFunctionCall',
],
];
}
}

0 comments on commit 96a61fc

Please sign in to comment.