Implicit immutable object mutation is not detected

[vudaltsov]

https://psalm.dev/r/bdd3b6d567

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/bdd3b6d567

<?php

/**
 * @psalm-immutable
 */
final class A
{
    public stdClass $b;
    
	public function __construct()
    {
        $this->b = new stdClass();
    }
}

$a = new A();
$a->b->c = 2; // mutates $a!

Psalm output (using commit d72b384):

No issues!

[weirdan]

$a->b->c = 2; // mutates $a!

Does it?

[weirdan]

I mean, Psalm allows immutable objects to reference mutable structures: https://psalm.dev/r/7992cc4b39, so why would stdClass be any different? In other words, immutability is shallow and does not extend to values of properties. One popular example of that are immutable message objects in PSR-7 that compose mutable body streams.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/7992cc4b39

<?php

class B {
    public int $c = 1;
}

/**
 * @psalm-immutable
 */
final class A
{
    public B $b;
    
	public function __construct()
    {
        $this->b = new B;
    }
}

$a = new A();
$a->b->c = 2; // mutates $a!

Psalm output (using commit d72b384):

No issues!

[vudaltsov]

I completely agree that immutable object should be allowed to contain mutable structures. The problem is not in stdClass, it's just an example.

The problem is that when you pass that mutable structure somewhere else and mutate it, the immutable object changes and you cannot rely on it anymore, although the instantiating code considered it to be unchangeable.

Recently we found a really painful bug. We didn't clone an immutable command's mutable property in the middleware and the handler was getting a different version of the command.

Maybe we could have @psalm-immutable-strict or @psalm-immutable-recursive to mark an object for which Psalm forbids any implicit external mutations?

Also, consider this example: https://psalm.dev/r/563c78d9ec. Isn't it kind of counterintuitive that line 26 is ok, while line 21 isn't?

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/563c78d9ec

<?php

class B {
    public int $c = 1;
}

/**
 * @psalm-immutable
 */
final class A
{
    public B $b;
    
	public function __construct()
    {
        $this->b = new B;
    }
    
    private function mutateB(): void
    {
		$this->b->c = 2;
    }
}

$a = new A();
$a->b->c = 2; // same thing as on line 21, but this time allowed!

Psalm output (using commit 8c761dd):

ERROR: ImpurePropertyAssignment - 21:3 - Cannot assign to a property from a mutation-free context

[weirdan]

@vudaltsov how would you answer the inlined questions in the following snippet: https://psalm.dev/r/23dd904a7a

I suspect the thing that you seem to suggest cannot be implemented without ownership tracking like Rust has.

Alternatively we could require all object properties of fully immutable (for the lack of better name) object to be themselves fully immutable. This way both your snippets would emit errors as neither B nor stdClass are fully immutable.

Isn't it kind of counterintuitive that line 26 is ok, while line 21 isn't?

It could be confusing at first glance, for sure. But if you consult the definition of @psalm-immutable it becomes quite obvious.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/23dd904a7a

<?php

class B { public int $c = 1; }

/** @psalm-immutable */
final class A {    
	public function __construct(
        public B $b = new B
    ) {}
}

$a = new A();
$a->b->c = 2; // assuming this one is flagged

$z = $a->b;
$z->c = 10; // would this have to be flagged as well?
f($a->b); // would this?

function f(object $o): void {
    $o->c = 30;
}

Psalm output (using commit e07e359):

No issues!