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

Calling Closure/callable leads to ImpureFunctionCall #8116

Open
klimick opened this issue Jun 15, 2022 · 10 comments
Open

Calling Closure/callable leads to ImpureFunctionCall #8116

klimick opened this issue Jun 15, 2022 · 10 comments

Comments

@klimick
Copy link
Contributor

klimick commented Jun 15, 2022

https://psalm.dev/r/392de9f519

Why I can't call callable/Closure in psalm-mutation-free/psalm-immutable context?
These annotations should garantie that current class instance doesn't mutate. Isn't?

(Go to use pure-Closure/pure-callable is not good idea)

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/392de9f519
<?php

/**
 * @template A
 * @implements Collection<A>
 */
final class ArrayList implements Collection
{
    /**
     * @param list<A> $items
     */
    public function __construct(private array $items)
    {
    }

    /**
     * @template B
     *
     * @param callable(A): B $ab
     * @return ArrayList<B>
     *
     * @psalm-mutation-free
     */
    public function map(callable $ab): ArrayList
    {
        $agg = [];

        foreach ($this->items as $item) {
            $agg[] = $ab($item);
        }

        return new ArrayList($agg);
    }
}

/**
 * @template-covariant A
 */
interface Collection
{
    /**
     * @template B
     *
     * @param callable(A): B $ab
     * @return Collection<B>
     *
     * @psalm-mutation-free
     */
    public function map(callable $ab): Collection;
}
Psalm output (using commit 10ea05a):

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

@orklah
Copy link
Collaborator

orklah commented Jun 15, 2022

Psalm can't check that the callable won't make you method mutate, so it has to be sure the callable itself is pure. I'm not sure I understood what you meant

@klimick
Copy link
Contributor Author

klimick commented Jun 15, 2022

I'm not sure I understood what you meant

I don't understand how to make, for example ArrayList usable in the real production code.

Imagine KVStore. It fetch by id something from db. It's impure.

/**
 * @template T
 */
final class KVStore
{
    /**
     * @return T
     */
    public function get(string $id): mixed
    {
        throw new RuntimeException('???');
    }
}

Next imagine the client side.
If client side will you KVStore it will be impure too.

final class FetchObjectByIds
{
    /**
     * @param KVStore<array{id: string, value: string}> $cache
     */
    public function __construct(private KVStore $cache) {}

    /**
     * @param ArrayList<string> $ids
     * @return ArrayList<array{id: string, value: string}>
     */
    public function getByIds(ArrayList $ids): ArrayList
    {
        return $ids->map(fn($id) => $this->cache->get($id));
    }
}

So fn($id) => $this->cache->get($id) is impure-callable and it cannot be passed to ArrayList::map.
https://psalm.dev/r/282a8ab4b0

Where is the usefulness from psalm-mutation-free/psalm-immutable?

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/282a8ab4b0
<?php

final class FetchObjectByIds
{
    /**
     * @param KVStore<array{id: string, value: string}> $cache
     */
    public function __construct(private KVStore $cache) {}

    /**
     * @param ArrayList<string> $ids
     * @return ArrayList<array{id: string, value: string}>
     */
    public function getByIds(ArrayList $ids): ArrayList
    {
        return $ids->map(fn($id) => $this->cache->get($id));
    }
}

/**
 * @template T
 */
final class KVStore
{
    /**
     * @return T
     */
    public function get(string $id): mixed
    {
        throw new RuntimeException('???');
    }
}

/**
 * @template A
 * @implements Collection<A>
 */
final class ArrayList implements Collection
{
    /**
     * @param list<A> $items
     */
    public function __construct(private array $items)
    {
    }

    /**
     * @template B
     *
     * @param pure-callable(A): B $ab
     * @return ArrayList<B>
     *
     * @psalm-mutation-free
     */
    public function map(callable $ab): ArrayList
    {
        $agg = [];

        foreach ($this->items as $item) {
            $agg[] = $ab($item);
        }

        return new ArrayList($agg);
    }
}

/**
 * @template-covariant A
 */
interface Collection
{
    /**
     * @template B
     *
     * @param pure-callable(A): B $ab
     * @return Collection<B>
     *
     * @psalm-mutation-free
     */
    public function map(callable $ab): Collection;
}
Psalm output (using commit 10ea05a):

ERROR: InvalidArgument - 16:26 - Argument 1 of ArrayList::map expects pure-callable(string):mixed, impure-Closure(string):array{id: string, value: string} provided

@AndrolGenhald
Copy link
Collaborator

@psalm-mutation-free is supposed to guarantee that the method's return value depends only on the object`s properties and the arguments. Using an impure callable breaks that guarantee (since 3v4l seems broken atm).

@klimick
Copy link
Contributor Author

klimick commented Jun 16, 2022

@psalm-mutation-free is supposed to guarantee that the method's return value depends only on the object`s properties and the arguments. Using an impure callable breaks that guarantee (since 3v4l seems broken atm).

Then let's say it differently. How I can do useful covariant collection?

Psalm prevent me to put covariant template to contravariant position (https://psalm.dev/r/76c25e34c3).
Of course it reasonable and soundness.
To do something like this we must prove that method doesn't change internal class state (@psalm-mutation-free) or whole class is immutable (@psalm-immutable). (https://psalm.dev/r/244dea724a, https://psalm.dev/r/b47232b299)

But @psalm-mutation-free/@psalm-immutable make class totally pure.
Not only immutable, but side effect free.
Side effect free means that I can't do forEach method on collection usable: https://psalm.dev/r/627b50d716
We achieve bonus issue UnusedMethodCall at every call forEach as side effect.

How I can do covariant collection? I should suppress any Closure/callable inside map/filter/etc method? How to deal with forEach?

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/76c25e34c3
<?php

/**
 * @template-covariant A
 */
interface Collection
{
    /**
     * @template B
     *
     * @param callable(A): B $ab
     * @return Collection<B>
     */
    public function map(callable $ab): Collection;
}
Psalm output (using commit 10ea05a):

ERROR: InvalidTemplateParam - 11:15 - Template param A of Collection is marked covariant and cannot be used here
https://psalm.dev/r/244dea724a
<?php

/**
 * @template-covariant A
 */
interface Collection
{
    /**
     * @template B
     *
     * @param callable(A): B $ab
     * @return Collection<B>
     *
     * @psalm-mutation-free
     */
    public function map(callable $ab): Collection;
}
Psalm output (using commit 10ea05a):

No issues!
https://psalm.dev/r/b47232b299
<?php

/**
 * @template-covariant A
 * @psalm-immutable
 */
interface Collection
{
    /**
     * @template B
     *
     * @param callable(A): B $ab
     * @return Collection<B>
     */
    public function map(callable $ab): Collection;
}
Psalm output (using commit 10ea05a):

No issues!
https://psalm.dev/r/627b50d716
<?php

/**
 * @template-covariant A
 */
interface Collection
{
    /**
     * @template B
     *
     * @param callable(A): B $ab
     * @return Collection<A>
     *
     * @psalm-mutation-free
     */
    public function forEach(callable $ab): Collection;
}

/** @var Collection<int> $collection */;
$collection->forEach(function($i) {
    print_r($i);
});
Psalm output (using commit 10ea05a):

ERROR: UnusedMethodCall - 20:14 - The call to Collection::forEach is not used

@klimick
Copy link
Contributor Author

klimick commented Jun 16, 2022

Why this code don't break any guaranties? https://3v4l.org/9qcZ1
I want the same behavior but in the object manner $l1->array_map($c);

@AndrolGenhald
Copy link
Collaborator

Ah, that makes sense now. I think this is an XY problem and the real issue here is with the covariance/contravariance rules.

Why this code don't break any guaranties?

Because we know that array_map doesn't modify the original array, but in your example if you remove @psalm-mutation-free Psalm doesn't like it because it thinks the callable might allow you to modify the collection illegally. If the template was used as an argument directly instead of as a callable param it causes this issue (3v4l), but since the covariant template is used as a callable param I think it should be fine and that's the actual bug here.

I'll think about it a bit more and add this as a testcase, hopefully it's not too difficult to fix. For now I think you just have to @psalm-suppress the InvalidTemplateParam.

@psalm-github-bot
Copy link

I found these snippets:

https://psalm.dev/r/b04e379623
<?php

/**
 * @template-covariant A
 * @implements Collection<A>
 * @implements IteratorAggregate<int, A>
 */
final class ArrayList implements Collection, IteratorAggregate
{
    /**
     * @param list<A> $items
     */
    public function __construct(private array $items)
    {
    }

    /**
     * @param A $a
     */
    public function push($a): void
    {
        $this->items[] = $a;
    }
    
    public function getIterator(): Traversable
    {
        return new ArrayObject($this->items);
    }
}

/**
 * @template-covariant A
 */
interface Collection
{
    /**
     * @param A $a
     */
    public function push($a): void;
}

class Animal {}
class Dog extends Animal
{
    public function bark(): void
    {
        echo "Woof";
    }
}
class Cat extends Animal
{
    public function meow(): void
    {
        echo "Meow";
    }
}

$dogs = new ArrayList([new Dog()]);

/** @param ArrayList<Animal> $animals */
function pushCat(ArrayList $animals): void
{
    $animals->push(new Cat());
}

pushCat($dogs);

foreach ($dogs as $dog) {
    $dog->bark();
}
Psalm output (using commit 10ea05a):

ERROR: InvalidTemplateParam - 37:15 - Template param A of Collection is marked covariant and cannot be used here

ERROR: InvalidTemplateParam - 18:15 - Template param A of ArrayList is marked covariant and cannot be used here
https://psalm.dev/r/76c25e34c3
<?php

/**
 * @template-covariant A
 */
interface Collection
{
    /**
     * @template B
     *
     * @param callable(A): B $ab
     * @return Collection<B>
     */
    public function map(callable $ab): Collection;
}
Psalm output (using commit 10ea05a):

ERROR: InvalidTemplateParam - 11:15 - Template param A of Collection is marked covariant and cannot be used here

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

No branches or pull requests

3 participants