Immutable storage [breaking change proposal]

[danog]

Opening this issue to allow discussion of possible breaking changes in psalm v5 to allow the immutability of types stored inside storages.

I've been encountering a good number of parallelism bugs, that are actually execution order bugs caused by corruption of some part of function, method, class or argument storages.
To debug this, in my fork I have "frozen" the contents of all storages after scanning, by recursively rendering immutable all stored Unions, using a flag that triggers exceptions when using mutator methods.

While this has helped me a lot in finding bugs (which led to bugfix PR #8131), another bug I manually found and fixed in #8098 isn't detected by rendering Unions immutable, and is probably caused by a reassignment of the as property in a TTemplate* Atomic type, which is obviously not detected even if the Union in the as itself is also marked immutable.

This is more problematic, and highlights a problem with the struct-like nature of Atomic types: lacking encapsulation, one can't easily detect the re-assignment of properties in Atomic types.

The breaking change I'm proposing to solve this issue (and to help detect misuse through static analysis) is to render all Atomic classes @immutable, and create a separate AtomicMutable class hierarchy which is mutable, with methods to convert between the two, sharing common methods between the two using traits; the same could be done for Unions, using MutableUnions instead.

This would allow statically detecting misuse of types (including in plugins, etc), and even at runtime if we were to require PHP 8.1 for psalm v5 to use readonly properties.

[danog]

cc'ing maintainers @orklah @weirdan @muglug :)

[orklah]

IIRC, we discussed making Unions immutable (and Atomics would follow) once with @AndrolGenhald. I think it's a very good idea but It may be a massive work: Psalm change properties a lot on Unions and Atomics...

I'm not sure I get the appeal of having both Mutable and Immutable classes though

[danog]

I've gone ahead with a draft refactoring, currently settling more on always-immutable Atomics and Unions and a mutable MutableUnion.

[weirdan]

I'd like to avoid Mutable* hierarchy if at all possible.

[AndrolGenhald]

I've wanted this for a long time, but it's a lot of work. I've been thinking a good way to refactor it would be to add a VariableStorage similar to PropertyStorage and move a lot of the non-type related stuff from Union to that. It would be good to have the type system be not so tightly coupled with the rest of the code.

I'd like to avoid Mutable* hierarchy if at all possible.

Absolutely, I'd rather not have everything duplicated. Maybe it'd be better to work bit by bit and make more classes immutable over time? For some of them it'll probably be easier to make individual properties readonly instead of trying to work through the entire class at once (especially Union since it's used everywhere).

---

I think there were also some performance worries about this. I recently discovered that Context used to clone $vars_in_scope, but that it was removed for performance reasons. I think that actually led to a lot of hard to debug issues that probably made it not worth the performance gain, but if we're cloning things all over the place it could really cause slowdowns.

If we eventually have all of the type-related classes immutable hopefully it will become less of an issue. There's no point having __clone() copy $this->type_params or $this->as if those properties can't be changed anyway, but for that to work I think we'd also need something like @psalm-immutable-strict that I suggested in #7629.

[orklah]

I don't have a lot of experience about immutability in large codebase, but potentially, having immutable unions could be more performant. Changing a type would imply making a new clone, but a lot of places where we clone types "just to be safe" would disappear because the called code won't be able to change the object. Cloning may become better targeted and only needed when changing something

[muglug]

I think there were also some performance worries about this. I recently discovered that Context used to clone $vars_in_scope, but that it was removed for performance reasons. I think that actually led to a lot of hard to debug issues that probably made it not worth the performance gain, but if we're cloning things all over the place it could really cause slowdowns.

This is correct — cloning was removed there for performance reasons, and it led to a bunch of hard-to-debug issues. But the performance gains were very real — when rewriting most of Psalm in Rust that was one of the big speedups, aside from paralellisation.

Immutable union types (and thus immutable atomic types) could be a performance benefit, but it would add quite a lot of extra code. I'm skeptical that it's worth it, but happy if someone wants to try — it would be easy to add the annotation, and hard to fix all the very many issues that crop up.

[AndrolGenhald]

rewriting most of Psalm in Rust

Is that available somewhere? I've semi-seriously considered trying something like that a few times, but I've never had the time.

[muglug]

Is that available somewhere? I've semi-seriously considered trying something like that a few times, but I've never had the time.

No, and I added some extra detail here: #8137

[danog]

Wow, that's a lot of useful information and feedback!

I'd like to avoid Mutable* hierarchy if at all possible.

Sure, I could work around this by making Unions completely immutable, using references where modification is required and use a UnionBuilder where simple helper mutator methods are required (like in SimpleAssertionReconciler)

Edit: actually, I don't like the idea of using references in methods like TemplateInferredTypeReplacer::replace, since it has potential to reintroduce the same class of issues we're trying to solve: instead, I'll switch to returning values and explicitly assigning them wherever needed.

Having immutable unions could be more performant. Changing a type would imply making a new clone, but a lot of places where we clone types "just to be safe" would disappear because the called code won't be able to change the object.

I thought of the exact same thing, it would be awesome!

rewriting most of Psalm in Rust

I spent the last month writing a php-to-C transpiler based on zephir to try and obtain easy performance gains in Psalm, currently the project is on hold due to the absolute unusability of the pre-alpha quality official zephir toolchain.
That being said, I still haven't completely given up on a compiled port of Psalm, I would absolutely love to give a go at integrating PHP support into that Rust port of Psalm :D

[muglug]

I spent the last month writing a php-to-C transpiler based on zephir to try and obtain easy performance gains in Psalm

I think this would be a better route to go than rewriting in Rust, and there's some precedent with other typecheckers — MyPy uses Mypyc to generate a transpiled version. But I still think it's beyond the scope of the Psalm project, and would have to be taken up by a well-resourced company (MyPy's development is sponsored by a number of them, including Dropbox).

[weirdan]

This has been implemented.