Psalm v5: __toString of some types does not return exact return type anymore

[boesing]

In psalm v4, using (string) $type on an instance of TNumericString returned numeric-string.
As of psalm v5, this now returns string.

Is this an expected behavior?

[psalm-github-bot]

Hey @boesing, can you reproduce the issue on https://psalm.dev ?

[orklah]

This would probably be an effect of #7409

But I don't get how it happens. Calling __toString on a TNumericString targets Atomic::__toString which itself calls $this->getId() which resolves to TNumericString::getId() with $exact to true. When calling that, it should return numeric-string. Are you sure this is the behavior you have?

[boesing]

I at least have failing codeception tests regarding this.

Will see if I can debug the reason.

[boesing]

It seems that in case of a Union, false is passed to Atomic#getId:

psalm/src/Psalm/Type/Union.php

Line 371 in 894e4e4

$types[] = (string)$type;

psalm/src/Psalm/Type/Union.php

Line 381 in f960d71

$types[] = $type->getId(false);

[orklah]

Oh I see, it was in a union. Could you try pushing a PR to change that to true? I think this will have other unwanted effects but I can't remember which. CI should tell us that

[boesing]

I probably just gonna switch to Atomic#getId directly instead.
Since the affected commit did not just change the Union but also other places, either reverting the commit or moving over to Atomic#getId would be a proper solution, right?

I guess you knew what you did there and thus I better not question that 😂

[orklah]

Honestly, the whole thing was a mess before :(

__toString was generally used to return a less detailed version of a type but it was not consistent, some function were calling other and then were overriden to call something else entirely. And nothing was documented.

We have a few BC break for Psalm 5 linked to this PR

[BC] Atomic::__toString() used to return a string representation of the type that was using double quotes (") to quote literals. This is now using single quotes (') to be more aligned with the rest of the codebase.

[BC] Atomic::__toString() is now final

[BC] Atomic::__toString() now returns a more detailed version of the type (it calls getId() under the hood)

[BC] Atomic::getId() has now a first param $exact. Calling the method with false will return a less detailed version of the type in some cases (similarly to what __toString used to return)

To give you a slight idea, here's what happened in 4.x:
TNumericString extends TNonEmptyString which itself extends TString.

Calling __toString on TString returns string
Calling __toString on TNonEmptyString returns string
Calling __toString on TNumericString returns numeric-string

Calling getKey on TString returns string
Calling getKey on TNonEmptyString returns string
Calling getKey on TNumericString returns numeric-string

Calling getId on TString returns string
Calling getId on TNonEmptyString returns non-empty-string
Calling getId on TNumericString returns numeric-string

So I had to make choices and break things in some places. I kept the original idea that __toString should be the one used for debug and that can be less detailed while getId should always be the more detailed but this could break things in some places when relying on the output of __toString

Disclaimer: I'm aware this is still a mess. For example, Union::__toString use less detailed types but Atomic::__toString use more detailed types. (IIRC, it's because one is used by Psalm for tests so I can't change that), but at least, it reduces the complexity of the system. It could probably still be improved and I'd be surprised if we don't refactor that once again in Psalm 6 !

[boesing]

Yah, for my use-case, Atomic#getId is exactly what I need. Was not aware of that back in the days and thus, using that method is fine for me.
Closing here as this is actually working as expected for psalm v5.

Thanks for explanation!