Associated mutable in immutable class breaks immutability

[snapshotpl]

https://psalm.dev/r/a6ac151a01

IMO psalm should report mutable error when not immutable class is injected.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/a6ac151a01

<?php

class Item {
  private int $i = 0;
	public function mutate(): void {
     $this->i++;
    }
  public function get(): int
  {
    return $this->i;
  }
}
  
/** 
 * @psalm-immutable
 */
class Immutable {
  private $item;
  
  public function __construct(Item $item)
  {
    $this->item = $item;
  }
  
  public function get(): int
  {
    return $this->item->get();
  }
}
$item = new Item();
$immutable = new Immutable($item);
$memory = $immutable->get();
$item->mutate();
if ($immutable->get() !== $memory) {
  // will be true
}

Psalm output (using commit ae0b1a6):

No issues!

[weirdan]

@muglug The fix looks a bit too radical. IMO this should not fail: https://psalm.dev/r/8b58a212bc.

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/8b58a212bc

<?php

class Item {
  private int $i = 0;
	public function mutate(): void {
     $this->i++;
    }
  public function get(): int
  {
    return $this->i;
  }
}
  
/** 
 * @psalm-immutable
 */
class Immutable {  
  public function get(Item $item): int
  {
    return $item->get();
  }
}
$item = new Item();
$immutable = new Immutable();
$memory = $immutable->get($item);
$item->mutate();
if ($immutable->get($item) !== $memory) {
  // will be true, and it's fine
}

Psalm output (using commit 7d99a15):

ERROR: ImpureArgument - 25:27 - Cannot pass mutable value to Immutable::get

ERROR: ImpureArgument - 27:21 - Cannot pass mutable value to Immutable::get

[muglug]

Yup, it doesn't anymore

[muglug]

In retrospect I think this was the wrong fix. The error here is allowing the assignment $this->item = $item. That's the risky thing – instead the code should be $this->item = clone $item to prevent the reference leaking

[muglug]

cc @sebastianbergmann and @ADmad who were caught up in this, among others. Upon further reflection the only issue is when an immutable class stores a reference to a mutable one.

[ADmad]

@muglug The issue I had reported about was a much simpler case #2930. There's no assignment there, just new object creation.

[muglug]

yeah, that's the point – whether or not the object created was immutable, that should not have been reported as an issue

[muglug]

I've actually gone back totally on this: I don't believe it was a bug in the first instance. Issue was raised here: #4126

Basically there's no way for any immutable/pure promises to be broken.