thecodingmachine / safe8

All PHP functions, rewritten to throw exceptions instead of returning false, now for php8
MIT License
106 stars 7 forks source link

Add pure annotation to pure functions #11

Open rubenrubiob opened 3 years ago

rubenrubiob commented 3 years ago

Hi!

When using sprintf in a class with a @psalm-immutable I get the error:

 ImpureFunctionCall: Cannot call an impure function from a mutation-free context

The thing is that, if I use core sprintf function, I do not get the error, as it is marked as pure. For example, in PHPStorm stubs the function is defined as follows:

/**
 * Return a formatted string
 * @link https://php.net/manual/en/function.sprintf.php
 * @param string $format <p>
 * The format string is composed of zero or more directives:
 * ordinary characters (excluding %) that are
 * copied directly to the result, and conversion
 * specifications, each of which results in fetching its
 * own parameter. This applies to both sprintf
 * and printf.
 * </p>
 * <p>
 * Each conversion specification consists of a percent sign
 * (%), followed by one or more of these
 * elements, in order:
 * An optional sign specifier that forces a sign
 * (- or +) to be used on a number. By default, only the - sign is used
 * on a number if it's negative. This specifier forces positive numbers
 * to have the + sign attached as well, and was added in PHP 4.3.0.</p>
 * @param string|int|float ...$values [optional] <p>
 * </p>
 * @return string a string produced according to the formatting string
 * format.
 */
#[Pure]
function sprintf(string $format, ...$values): string {}

I am opening this issue to ask if it would be valuable to add a #[Pure] annotation to all functions defined in Safe. This is something I could work on. As I do not know which functions are pure, I would base the work on PHPStorm stubs. We should be fine.

Do you think that this is something that may be worth to work on?

Thank you!

Edit: I see that it is implemented in the 7.x branch: https://github.com/thecodingmachine/safe/pull/172 The work could be based on that, too.

simPod commented 3 years ago

We should rather use @psalm-immutable. #[Pure] is not standardized.

rubenrubiob commented 3 years ago

@simPod I suppose you refer to @psalm-pure. In any case, I agree with you, it is better to use that annotation.

I saw that https://github.com/thecodingmachine/safe/pull/172 is not merged (my mistake, I did not see it), but it looks a good approach. The thing that bothers me is how to get the list of pure/impure PHP functions and how to maintain it.

One option would be to copy https://github.com/vimeo/psalm/blob/master/src/Psalm/Internal/Codebase/Functions.php#L385 and adapt it to Safe.

The other option is to check all functions with #[Pure] attribute in https://github.com/JetBrains/phpstorm-stubs to get the list and adapt it to Safe.

In both cases the list should be maintained by hand. I suppose it is not a big deal, as new functions are only added with new versions of PHP, and the list should not grow out of control. But I would rather use something automatic: maybe a separated composer package that contains the list of PHP impure/pure functions. It may be something to work on after it is implemented on Safe.

I think I will have time tomorrow morning to work on a PR for this, taking the list from Psalm, as it looks easier. We could work based on that, if I get to a solution.