Sprintf with only one argument without formatting directives leads to false TooFewArguments

[tscni]

https://psalm.dev/r/6c9dd0f8b3

Presumably caused by

psalm/src/Psalm/Internal/Provider/ReturnTypeProvider/SprintfReturnTypeProvider.php

Lines 73 to 86 in 564c6de

	// 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') {
	IssueBuffer::maybeAdd(
	new TooFewArguments(
	'Too few arguments for ' . $event->getFunctionId() . ', expecting at least 2 arguments',
	$event->getCodeLocation(),
	$event->getFunctionId(),
	),
	$statements_source->getSuppressedIssues(),
	);
	return null;
	}

It states

it makes no sense to use sprintf when there is only 1 arg (the format)
as it wouldn't have any placeholders

Which sure, is true, but that's a code style issue and not statically incorrect code.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/6c9dd0f8b3

<?php

echo sprintf('text');

Psalm output (using commit 564c6de):

ERROR: TooFewArguments - 3:6 - Too few arguments for sprintf, expecting at least 2 arguments

[tscni]

I guess this relates to #9987
Based on the discussion there, it seems like switching this case (and similarly for printf) over to RedundantFunctionCall might be the way to go?

[ygottschalk]

+1 change the error to RedundantFunctionCall or remove it at all for above given reason (codestyle != statically incorrect)

@kkmuffme Im interested in your thoughts on this?

[kkmuffme]

Yes, makes sense to move it to RedundantFunctionCall (and change it for printf too).
It's not really a "code style" issue, just as https://psalm.dev/r/ac4e6664e2 isn't one (unless your code style is "I'm being paid per line of code" :D )

In fact, in this case with sprintf it's even more likely this is a bug though, that will not be caught:
https://psalm.dev/r/4f60fa8c62

This is a classic (and common) mistake, that should definitely get reported, but can only get reported when the 1st arg is a literal string - for all other cases like https://psalm.dev/r/730db01a6d it would now only report as redundant function call.

I'll fix change this and also take care of #9987 (comment) (@orklah):

• change printf/sprintf with 1 argument to RedundantFunctionCall if the 1st arg is a literal string WITHOUT placeholder
• change that the printf/sprintf pattern is validated if it's a literal string, even if there's only 1 argument to catch things like https://psalm.dev/r/4f60fa8c62 and report an error TooFewArguments
• keep InvalidArgument for printf/sprintf with 1 argument, when the 1st argument is NOT a literal string, e.g. https://psalm.dev/r/730db01a6d - since validation isn't possible in that case, and it's highly likely this is a mistake. (while it is perfectly fine type wise, what we care about is that the output is fine - I don't want customers to get emails with greeting "Hi %s" instead of "Hi Sam")

Could you all please let me know what you think, then I'll take care of that in the next days

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/ac4e6664e2

<?php

$a = 'text';

if ( is_string( $a ) ) {
 	   
}

Psalm output (using commit 9c814c8):

ERROR: RedundantCondition - 5:6 - Type 'text' for $a is always string

https://psalm.dev/r/4f60fa8c62

<?php

printf('Hi %s');

Psalm output (using commit 9c814c8):

ERROR: TooFewArguments - 3:1 - Too few arguments for printf

https://psalm.dev/r/730db01a6d

<?php
/** @var string $greeting_text */

echo sprintf( $greeting_text );

Psalm output (using commit 9c814c8):

ERROR: TooFewArguments - 4:6 - Too few arguments for sprintf, expecting at least 2 arguments

[tscni]

Sounds great!

I hadn't thought about the non-literal case at all.
Maybe the issue message could hint at the problem a bit more (something about not being able to determine placeholders)?

I wonder how worthwhile it'd be to add a new issue type for this case in general, even if there are 2 arguments or more, because it's never clear how many placeholders there really are. (e.g. a "possibly too few arguments" or "non analyzable function argument" - not sure if a similar type already exists)

[ygottschalk]

IMO we should not push our feelings/views/opinions on others, that is why I think we should let people use printf as their 'default output method'. This would mean to not report issues for using printf with one argument, unless this argument can contain a literal-string containing a placeholder.

All other proposed changes are looking like good improvements!

[kkmuffme]

I think we should let people use printf as their 'default output method'.

We definitely can, it's just a bad practice - it's slower than print, if you accidentally use a placeholder in it it will throw/crash (PHP 8) or not output anything (PHP 7) and makes identifying whether it should be a pattern or just a regular string harder for people who work on that codebase.

Taking it into account though, I'll change printf with 1 arg to only report RedundantFunctionCall instead of InvalidArgument

[ygottschalk]

it's just a bad practice

100% agree!

I'll change printf with 1 arg to only report RedundantFunctionCall

I would either not report an issue (since printf does output, the call is not redundant, just bad practice) or raise some issue with a level of Info.

[tscni]

As a user ideally I'd want to:

• Have a sprintf() call with a single format-less literal (or analyzable variable) argument be reported as RedundantFunctionCall
• Have a printf() call with a single format-less literal (or analyzable variable) argument be reported as "some issue type" (e.g. a SuboptimalFunctionCall) that doesn't necessarily have to be an error by default
• Have a sprintf() / printf() call with a single literal (or analyzable variable) argument with a formatting directive be reported as TooFewArguments
• Have a sprintf() / printf() call with an unanalyzable first argument, no matter the other arguments, be reported as "some issue type" (e.g. UnanalyzableFormatArgument) that doesn't necessarily have to be an error by default.
  Having it in a try-catch block that catches ArgumentCountError should suppress the issue though. (if that's the only error that could be thrown in that case, I haven't checked)

[kkmuffme]

Sorry for the delay here, will fix this today.

@danog your changes to SprintfReturnTypeProvider.php in 79da332 seem wrong - that code is required when running psalm with PHP 7, otherwise these won't get reported. Is there a reason you removed that?

[danog]

Psalm v6 will not support PHP 7 anymore, the minimum supported version will be PHP 8.1.17.

[weirdan]

Psalm v6 will not support PHP 7 anymore, the minimum supported version will be PHP 8.1.17.

To expand on that, we've started work on Psalm 6 (the master branch). The 5.x branch is still open and is the default branch to propose changes (unless these should only appear in Psalm 6).

[kkmuffme]

@weirdan thanks, just realized that there was a 5.x branch now. All clear now why @danog removed that.