Add psalm-flow for string functions from sscanf to wordwrap

[LukasReschke]

This should conclude all string functions from https://www.php.net/manual/en/book.strings.php

Continuation of #4576

Ref #3636

TODO

• strtr return provider
• Fix tests
• Cleanup commits + remove accidentially commited files

[LukasReschke]

Any pointers why the following test is failing? I defined the second argument as mixed as per php.net (https://www.php.net/manual/en/function.strstr.php). But the test seems to intentionally test wrong behaviour?

psalm/tests/FunctionCallTest.php

Lines 504 to 515 in 494ec40

	'strtrWithPossiblyFalseFirstArg' => [
	'<?php
	/**
	* @param false|string $str
	* @param array<string, string> $replace_pairs
	* @return string
	*/
	function strtr_wrapper($str, array $replace_pairs) {
	/** @psalm-suppress PossiblyFalseArgument */
	return strtr($str, $replace_pairs);
	}',
	],

[weirdan]

But the test seems to intentionally test wrong behaviour?

The test was added along with the fix for #915, and was probably related to how Psalm selects the overloaded signature (strtr() has two) from the callmap.

defined the second argument as mixed as per php.net

That's too wide. It should be either string or array, depending on the number of parameters (string for 3 arg signature and array for 2).

[LukasReschke]

That's too wide. It should be either string or array, depending on the number of parameters (string for 3 arg signature and array for 2).

Oof. Seems the PHP documentation omits that behaviour for arrays straight out. Even though it works fine as per https://3v4l.org/7PJG7 :-/

I'm going to fix this, thanks! Also the CallMap was new to me, good to know. :)

I was wrong. I looked at the wrong docs. https://www.php.net/manual/en/function.strtr.php and https://www.php.net/manual/en/function.strstr.php were confusing me :)

[staabm]

Side-note: could someone elaborate why/when psalm-flow should be used?

Its not documented.. is it a psalm internal-only phpdoc?

[orklah]

Side-note: could someone elaborate why/when psalm-flow should be used?

Its not documented.. is it a psalm internal-only phpdoc?

See #4587 (comment)

[LukasReschke]

This should be ready to review now. For strtr I added a provider (StrTrReturnTypeProvider) that will propagate the taint flow for all parameters.

[LukasReschke]

The error being thrown is from https://github.com/sebastianbergmann/phpunit/blob/58f69d18fd1e61b54c18cf9d06b762507be55562/src/TextUI/DefaultResultPrinter.php#L149-L154:

        if (!in_array($colors, self::AVAILABLE_COLORS, true)) {
            throw InvalidArgumentException::create(
                3,
                vsprintf('value from "%s", "%s" or "%s"', self::AVAILABLE_COLORS)
            );
        }

As per php.net:

https://www.php.net/manual/en/function.vsprintf.php

vsprintf ( string $format , array $args ) : string|false

Return array values as a formatted string according to format, or FALSE on failure.

And the code doesn't check for false. - Should this be accepted behaviour? For sprintf it seems we just ignore the fact that it could return false. I can also do the same for vsprintf.

psalm/stubs/CoreGenericFunctions.phpstub

Lines 755 to 762 in bf873b2

	/**
	* @psalm-pure
	*
	* @param string|int|float $args
	*
	* @psalm-flow ($format, $args) -> return
	*/
	function sprintf(string $format, ...$args) : string {}

[muglug]

Thanks!

Possibly-false returns in standard library functions are annoying. In > 99.9% of their common usage those functions don’t return false, so Psalm has an annotation @psalm-ignore-falsable-return that triggers special behaviour in a bunch of places.

[orklah]

I have a few differents false positives that popped in my work codebase that seems related to this PR.

I'll need to verify them and post issues but if someone is faster than me:

• stristr return type seems to not contain false anymore
• substr_replace 4th param should be optional and it became mandatory
• I have new errors related to the offset of the array returned by str_split. Not sure if it's a false positive or if the type was narrowed => Checked, the return type was list<string>|false and is now array<string>