Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Psalm static analysis fails on MapperBuilder::registerTransformer #550

Open
Yui-Ezic opened this issue Aug 3, 2024 · 1 comment
Open

Comments

@Yui-Ezic
Copy link

Yui-Ezic commented Aug 3, 2024

Psalm static analysis fails on MapperBuilder::registerTransformer, because transformer should be of type class-string|pure-callable, but sometimes transformers isn't pure.

Examples

Example 1: format dates

(new \CuyZ\Valinor\MapperBuilder())
    ->registerTransformer(
        fn (\DateTimeInterface $date) => $date->format('Y/m/d')
    );

Psalm output

ERROR: InvalidArgument - xxx/transformer.php:20:9 - Argument 1 of CuyZ\Valinor\MapperBuilder::registerTransformer 
expects class-string|pure-callable, but impure-Closure(DateTimeInterface):string provided 
(see https://psalm.dev/004)
        fn (\DateTimeInterface $date) => $date->format('Y/m/d')

Example 2: chained

(new \CuyZ\Valinor\MapperBuilder())
    ->registerTransformer(
        fn (string $value, callable $next) => strtoupper($next())
    );
ERROR: InvalidArgument - xxx/transformer.php:20:9 - Argument 1 of CuyZ\Valinor\MapperBuilder::registerTransformer 
expects class-string|pure-callable, but impure-Closure(string, callable):string provided 
(see https://psalm.dev/004)
        fn (string $value, callable $next) => strtoupper($next())

If you mark $next parameter as pure-callable, then the psalm analysis will be successful, but there will be errors in runtime

(new \CuyZ\Valinor\MapperBuilder())
    ->registerTransformer(
        /** @param pure-callable $next */
        fn (string $value, callable $next) => strtoupper($next())
    )
    ->normalizer(\CuyZ\Valinor\Normalizer\Format::array())
    ->normalize('Hello world');

Php output

PHP Fatal error:  Uncaught CuyZ\Valinor\Normalizer\Exception\TransformerHasInvalidCallableParameter: Transformer's
 second parameter must be a callable, `pure-callable` given for `Closure (lines 11 to 12 of /app/xxx/transformer.php)`. in
/app/vendor/cuyz/valinor/src/Normalizer/Transformer/ValueTransformersHandler.php:122

Example 3: invokable class

This is not pure transformer, cause it depends on $separator value from constructor, but i think it's still should be valid transformer

final readonly class ImplodeLists
{
    public function __construct(private string $separator) {}

    /**
     * @param list<string> $value
     */
    public function __invoke(array $value, callable $next): string
    {
        return implode($this->separator, $next());
    }
}

final readonly class StringList
{
    public function __construct(
        /** @var list<string> */
        public array $list
    ) {}
}

(new \CuyZ\Valinor\MapperBuilder())
    ->registerTransformer(new ImplodeLists(', '))
    ->normalizer(\CuyZ\Valinor\Normalizer\Format::array())
    ->normalize(
        new StringList(
            list: ['foo', '$bar']
        )
    );

Psalm output

ERROR: InvalidArgument - xxx/transformer.php:32:27 - Argument 1 of CuyZ\Valinor\MapperBuilder::registerTransformer 
expects class-string|pure-callable, but XXX\ImplodeLists provided (see https://psalm.dev/004)
   ->registerTransformer(new ImplodeLists(', '))
@SamMousa
Copy link

A library should probably not be type hinting a pure-callable on a closure anyway. What does the library care if my transformer has state or side effects?

I could have a transformer that logs, or increments a number each time it sees a specific value; it is not relevant.

That said, I believe the error is not just about purity.

Parameter #1 $transformer of method CuyZ\Valinor\MapperBuilder::registerTransformer() expects (pure-callable(): mixed)|class-string, Closure(string, callable): string given.

I get this when using the following code, from the source code documentation:

->registerTransformer(fn (string $value, callable $next) => strtoupper($next()))

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants