Begin immutable refactoring

[danog]

As per #8132, fixes #8310

[danog]

Settling on a simpler structure based on ImmutableUnions to avoid churn.

[danog]

Almost done here, the only strange thing left is the assertion testcase failure, they're caused by https://github.com/vimeo/psalm/blob/master/src/Psalm/Internal/Analyzer/Statements/Expression/CallAnalyzer.php#L766, which relies on an inconsistent internal state of Union after template substitution, which the old code caused (the types property is correctly populated but the literal_*_types properties are empty), it's really strange that we're relying on this behavior and I'm not really sure what to do here, any help? :)

[danog]

Actually hang on, it seems like the allowCanBeNotSameAfterAssertionScalar unit test itself is wrong:

<?php
                    namespace Bar;

                    /**
                     * Asserts that two variables are the same.
                     *
                     * @template T
                     * @param T      $expected
                     * @param mixed  $actual
                     * @psalm-assert !=T $actual
                     */
                    function assertNotSame($expected, $actual) : void {}

                    $c = 4;
                    $d = rand(0, 1) ? 4 : 5;
                    assertNotSame($d, $c);

                    function foo(string $a, string $b) : void {
                        assertNotSame($a, $b);
                    }

It's basically asserting that $c != $d, aka 4 != 4|5 aka 4 != 4 || 4 != 5, and the first assertion is indeed impossible

[danog]

The other array-related unit test failure seems actually like a (partial) improvement over the previous behavior, so I'm actually thinking of correcting the testcase to account for the new behavior

[orklah]

I don't get how the unit test is wrong. You should be able to replace assertNotSame($d, $c); by assert($d !== $c);. It's a valid thing to check

[danog]

I don't get how the unit test is wrong. You should be able to replace assertNotSame($d, $c); by assert($d !== $c);. It's a valid thing to check

I feel there is a slight distinction (https://psalm.dev/r/05628e8554):

• assert($expected !== $actual): psalm errors if there is no intersection between the types of $actual and $expected
• assertNotSame($expected, $actual) psalm errors if the type of $actual doesn't contain the type of $expected

Or at least, that's the current behavior with this MR.
The previous behavior was:

• assert($expected !== $actual): psalm errors if there is no intersection between the types of $actual and $expected
• assertNotSame($expected, $actual):
  • if $expected is a union type, psalm errors if there is no intersection between the types of $actual and $expected
  • otherwise, psalm errors if the type of $actual doesn't contain the type of $expected

I'm not sure what should be the correct behavior, given that to obtain the previous behavior we relied on an inconsistent state of union, possibly caused by a mistake/bug.

Could anyone clarify on what was the actual, intended, behavior, and if it relied on that inconsistent state of Union?

[psalm-github-bot]

I found these snippets:

https://psalm.dev/r/05628e8554

<?php
namespace Bar;

/**
 * Asserts that two variables are not the same.
 *
 * @template T
 * @param T      $expected
 * @param mixed  $actual
 * @psalm-assert !=T $actual
 */
function assertNotSame($expected, $actual) : void {}

$expected = 4;
$actual = rand(0, 1) ? 4 : 5;
// 4 != $actual
// We expect $actual to contain the type 4, if it doesn't emit an issue.
assertNotSame($expected, $actual);

$expected = rand(0, 1) ? 4 : 5;
$actual = 4;

// 5|4 != $actual
// We expect $actual to contain the type 5|4, if it doesn't emit an issue.
assertNotSame($expected, $actual);

$expected = rand(0, 1) ? 4 : 5;
$actual = 4;

// $expected != $actual
// We expect $actual to have the same value of $expected
assert($expected !== $actual);

Psalm output (using commit 4b2935f):

INFO: UnusedParam - 12:24 - Param $expected is never referenced in this method

INFO: UnusedParam - 12:35 - Param $actual is never referenced in this method

[danog]

I think this is what was @boesing actually meant when using isSingle, fixed by switching to manually counting the number of types, since the number of literal_* types will always remaing unchanged (usually 0 if there originally was just the template type) after template inferring.

[boesing]

Just as a sidenote, AFAIR the isSingle check was there before I added my stuff. I just refactored it a bit. Probably worth going through the history to find the initial commit which added the check.

I was super confused by these test cases as well when I worked on adding my code. I somehow managed to change the code so it works for the old test cases and my new code.

[danog]

This should do the trick nicely I think :)

Edit: never mind :(

[danog]

All done!

[danog]

I'll continue refactoring in a separate MR, I still have to implement atomic immutability (at least for nested Union properties) and switch to @readonly properties in storages.

[orklah]

A few things:

• do we need bustCache in Union still? IIRC, it's used to rerender the internal id when the union changed. It shouldn't do that anymore.
• Could we make a check for all clone $union everywhere and get rid of them? it shouldn't be needed anymore
• Could you document this change in README.md. It's actually a big change for plugin authors and they'll need to be aware of this

@weirdan @muglug @AndrolGenhald this is a big fundamental change. I could use your help reviewing this :)

[AndrolGenhald]

do we need bustCache in Union still? IIRC, it's used to rerender the internal id when the union changed. It shouldn't do that anymore.

The scope of this PR is actually smaller than I expected. The Atomics in the Union are mutable and can still change so that'll still be required when that happens. Some calls might be removable now though, if they haven't been already.

Could we make a check for all clone $union everywhere and get rid of them? it shouldn't be needed anymore

It's probably still needed in most cases since Atomics are still mutable.

[orklah]

Oh right, didn't thought about that

[danog]

do we need bustCache in Union still? IIRC, it's used to rerender the internal id when the union changed. It shouldn't do that anymore.

As atomics are still mutable (they won't be in my next PR), bustCache still has some possible uses.

Could we make a check for all clone $union everywhere and get rid of them? it shouldn't be needed anymore

Same thing about atomics + even after immutable atomics, there might be some usecases to cloning types, since I'm still not sure whether to mark as @readonly all properties of Unions and Atomics (not just those with type Union/Atomic).

Could you document this change in README.md. It's actually a big change for plugin authors and they'll need to be aware of this

Done!

[orklah]

Seems great. Let's wait a few days to see if Matt or Bruce have time to chime in and we'll be good to go :)

Thanks for the massive work!

[orklah]

Alright, looks like we didn't get more comments.

I'm ready to merge, I just need to make sure you're ready to do the last part (otherwise, it won't make much sense as is) :)

[orklah]

I wonder if this is a change big enough to consider delivering V5 soon and merge for a V6 that we would start immediately after.

All things considered, we probably should have released V5 long ago and started a V6 already. Given how we lack long term vision, we should probably deliver major version more often to avoid piling big changes together...

[AndrolGenhald]

I wonder if this is a change big enough to consider delivering V5 soon and merge for a V6 that we would start immediately after.

FWIW I was hoping to get a V6 relatively soon after V5 anyway (or at least sooner than V5 is after V4) with the major type system changes I've been wanting to do (and never managed to get fully working), but it's probably not going to be until next year till that I can devote a lot of time to that again.

[danog]

I'm ready to merge, I just need to make sure you're ready to do the last part (otherwise, it won't make much sense as is) :)

I'm actually already working on it on the immutable_readonly branch! :)

About v5/v6, of course I'd love to see this land in v5, if only because I'll be able to get better feedback on this and my other PRs you already merged :)

I'm a bit afraid of giving ETAs, but I'll try finishing up the immutable refactoring by September.
Do you think September would be an OK release date for v5?

[orklah]

Yeah, September is as good a date as any, we just need to lock a date and keep it. I'll leave this unmerged for now, it may be hard to merge incoming 4.x changes in the meantime otherwise

[muglug]

I think this is a fine v5 change if you want to target September. Sorry I haven’t been more active, but I wanted to say earlier: when it comes to these sorts of big changes, I rely on Psalm and Psalm’s test suite to detect regressions.

Over Psalm’s history I have done many big refactors, and very few of them broke some downstream consumer’s pipeline.

[orklah]

Thanks for feedback Matt! Did you had issues with plugin authors on previous majors?

This is definitely a change that will affect them directly

[muglug]

No — I think if the intent of the refactoring is sound, people generally “get it”