Annotation to allow mutating property in mutation-free context

[AndrolGenhald]

Sometimes it's desirable to have a property be modified in a mutation-free context when that modification wouldn't change the mutation-free status of the function: https://psalm.dev/r/ae13d40aee. It would be good to have an annotation for this, but we should also make sure the documentation warns users that using this annotation prevents safety checks, and it's the user's responsibility to ensure that the mutations on the property don't affect the observable behavior of the class.

See also doctrine/collections#322 where this came up with ReadableCollection::contains().

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/ae13d40aee

<?php

class Foobar
{
    /**
     * Shouldn't affect observable behavior besides call time and memory usage.
     * //@psalm-???
     * @var array<int, int>
     */
    private array $bazCache = [];
    
    /** @psalm-mutation-free */
    public function baz(int $i): int
    {
        if (!isset($this->bazCache[$i])) {
            // This should actually be fine since it doesn't affect observable behavior
            $this->bazCache[$i] = $this->longComputation($i);
        }
        
        return $this->bazCache[$i];
    }
    
    /** @psalm-mutation-free */
    private function longComputation(int $i): int
    {
        return $i * 42;
    }
}

Psalm output (using commit afe85fa):

ERROR: ImpurePropertyAssignment - 17:13 - Cannot assign to a property from a mutation-free context

[greg0ire]

I just noticed there is @psalm-external-mutation-free, does this match the feature request, or is that something else?

[AndrolGenhald]

That's probably why I thought we already had something for this, but that's actually used to annotate methods that are allowed to modify properties on $this but nothing else.

[greg0ire]

I believe that would be appropriate for contains(), but using it does not seem to help:

ERROR: ImpureMethodCall - lib/Doctrine/Common/Collections/AbstractLazyCollection.php:70:35 - Cannot call a possibly-mutating method Doctrine\Common\Collections\Collection::contains from a mutation-free context (see https://psalm.dev/203)
        return $this->collection->contains($element);


ERROR: InvalidTemplateParam - lib/Doctrine/Common/Collections/ReadableCollection.php:24:21 - Template param T of Doctrine\Common\Collections\ReadableCollection is marked covariant and cannot be used here (see https://psalm.dev/183)
     * @psalm-param T $element


------------------------------
2 errors found
------------------------------
3 other issues found.
You can display them with --show-info=true
------------------------------

UPD: I understand the first issue, since Collection is not the interface that is annotated with @psalm-external-mutation-free second update: actually, contains() is inherited annotated with @psalm-external-mutation-free regardless of where it comes from, why would it be "possibly mutating"?

[AndrolGenhald]

Right, because since it's allowed to modify properties on $this it might cause the template type to change. This is fine, this is not. Tbh I'm not really sure what the use case of @psalm-external-mutation-free is, I've never used it or seen it used.

[psalm-github-bot[bot]]

I found these snippets:

https://psalm.dev/r/f3cfa359f0

<?php

/** @template-covariant T */
class Wrapper
{
    /** @param T $val */
    public function __construct(private $val) {}
    
    /**
     * @psalm-mutation-free
     * @param T $other
     */
    public function equals($other): bool
    {
        return $this->val == $other;
    }
}

Psalm output (using commit afe85fa):

No issues!

https://psalm.dev/r/1ff219ae6d

<?php

/** @template-covariant T */
class Wrapper
{
    /** @param T $val */
    public function __construct(private $val) {}
    
    /**
     * @psalm-external-mutation-free
     * @param T $other
     */
    public function equals($other): bool
    {
        // What if this happens?
        // $this->val = $other;
        return $this->val == $other;
    }
}

Psalm output (using commit afe85fa):

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

[greg0ire]

Ok so we do indeed need something more precise than @psalm-external-mutation-free, where there might be an internal mutation as long as it does not affect the template item type.

[danog]

Precisely what I need for the checked field of Atomic in my immutable refactoring :)