@psalm-immutable throws an ImpurePropertyAssignment

[remorhaz]

Psalm documentation defines @psalm-immutable as a way "to annotate a class where every property is treated by consumers as @psalm-readonly and every instance method is treated as @psalm-mutation-free." According to that, marking the class as immutable should be a strict equivalent of marking all it's properties as read-only and all it's methods as mutation-free; but that doesn't work.

https://psalm.dev/r/a9c53e21c9

In given example classes A and B are identical: they both accept an Exception instance (just arbitrary non-immutable object) in constructor, assign it to a property and provide a getter for that property. But A class gets an ImpurePropertyAssignment, which is wierd because there's no any @psalm-pure annotation.

It seems that either the documentation is incorrect/incomplete or @psalm-immutable doesn't work as expected.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/a9c53e21c9

<?php

/**
 * @psalm-immutable
 */
class A
{
    
    private $x;
    
    public function __construct(Exception $x)
    {
        $this->x = $x;
    }
    
    public function getX(): Exception
    {
        return $this->x;
    }
}

class B
{
    
    /**
     * @psalm-readonly
     */
    private $x;
    
    public function __construct(Exception $x)
    {
        $this->x = $x;
    }
    
    /**
     * @psalm-mutation-free
     */
    public function getX(): Exception
    {
        return $this->x;
    }
}

Psalm output (using commit a4d6a84):

ERROR: ImpurePropertyAssignment - 13:9 - Cannot store a reference to an externally-mutable object inside an immutable object – consider using __clone

[muglug]

This is what that rule is designed to prevent: https://psalm.dev/r/55507b5a32 – existing Psalm rules say that calling a method on any object is fine in a pure context if that method is mutation-free.

Alternatively you can choose to clone the object, fixing the issue: https://psalm.dev/r/e0dca603d6

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/55507b5a32

<?php

/**
 * @psalm-immutable
 */
class A
{
    private $x;
    
    public function __construct(MutableWithMutationFreeMethod $x)
    {
        /** @psalm-suppress ImpurePropertyAssignment */
        $this->x = $x;
    }
    
    public function getX(): MutableWithMutationFreeMethod
    {
        return $this->x;
    }
    
    public function getXS(): string
    {
        return $this->x->s;
    }
}

class MutableWithMutationFreeMethod {
    public $s;
    
    public function __construct(string $s) {
        $this->s = $s;
    }
    
    /** @psalm-mutation-free */
    public function getS() : string {
        return $this->s;
    }
}

$m = new MutableWithMutationFreeMethod("hello");
$a = new A($m);
echo $a->getXS();
$m->s = "goodbye";
echo $a->getXS();

Psalm output (using commit a4d6a84):

No issues!

https://psalm.dev/r/e0dca603d6

<?php

/**
 * @psalm-immutable
 */
class A
{
    private MutableWithMutationFreeMethod $x;
    
    public function __construct(MutableWithMutationFreeMethod $x)
    {
        $this->x = clone $x;
    }
    
    public function getX(): MutableWithMutationFreeMethod
    {
        return $this->x;
    }
    
    public function getXS(): string
    {
        return $this->x->s;
    }
}

class MutableWithMutationFreeMethod {
    public $s;
    
    public function __construct(string $s) {
        $this->s = $s;
    }
    
    /** @psalm-mutation-free */
    public function getS() : string {
        return $this->s;
    }
}

$m = new MutableWithMutationFreeMethod("hello");
$a = new A($m);
echo $a->getXS();
$m->s = "goodbye";
echo $a->getXS();

Psalm output (using commit a4d6a84):

No issues!

[muglug]

actually I think you have a point – I'm trying to come up with a situation that this is really ever a problem, and I can't

[muglug]

fixed in 8505ca2

[weirdan]

So this (https://psalm.dev/r/d76dbd22f3, https://3v4l.org/901Ce) is a properly immutable class, right? If so, I think this needs to be highlighted in the docs.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/d76dbd22f3

<?php
class StringBuffer {
    private string $s = '';
    public function add(string $s): void {
        $this->s .= $s;
    }
    public function get(): string {
        return $this->s;
    }
}

/** @psalm-immutable */
class A {  
    private StringBuffer $b;
    
    public function __construct(StringBuffer $b) {
        $this->b = $b;
    }
    
    public function getBufferContents(): string {
        return $this->b->get();
    }
}

$s = new StringBuffer;
$s->add("aaa");

$a = new A($s);
echo $a->getBufferContents() . PHP_EOL;

$s->add("zzz");

echo $a->getBufferContents() . PHP_EOL;

Psalm output (using commit 66251d8):

No issues!

[remorhaz]

I agree with your decision, but my point was rather on adjusting the definition of "immutable object". Documentation, in fact, states that immutable object is an object with read-only properties and mutation-free methods; but in fact there were additional limitations (at least before your fix). If there are more additional limitations left, it would be useful to describe them in documentation.

[snapshotpl]

@muglug are you sure with that? Immutable object mean object can change, there is not any side effect. This is how I it understand and use in my apps. However maybe there is many other kind of immutable. Anyway, previously implementation with #2805 was very useful for my apps. IMO we should revert this change and change definition of @psalm-immutable or introduce new one, like @psalm-strong-immutable with previously behavior.

I have found interesting article about immutability here https://www.yegor256.com/2016/09/07/gradients-of-immutability.html

[muglug]

@snapshotpl the basic value of immutable objects, in my mind anyway, is the idea that they can be constructed and also used inside pure functions without violating that function's purity. That's their only value, in my mind – a promise that nowhere will they do anything that mutates state.