Clarify psalm types #112
Clarify psalm types #112
Conversation
They all modify the cache, so they can't be `mutation-free`, and they rely on the object state, so they're not `pure`.
This makes it so Psalm doesn't think we're passing `mixed` values around. We're either constructing using an instance of ourselves, or using a value which we're going to wrap.
Might as well be more precise about this, as Psalm supports doing so.
This makes it clear that we're not passing `mixed` values around.
|
Thank you for all these details! I had a look at the code diff for solution 2, I don't think it is worth changing the code so much (and the external API of the class, i.e. the constructor parameters) just for Psalm annotations. I am fine suppressing the error here (solution 1), and you describe very well the reasons why. Having enums as immutable is a must-have IMO. |
|
|
| @@ -86,6 +86,7 @@ public function getKey() | |||
|
|
|||
| /** | |||
| * @psalm-pure | |||
| * @psalm-suppress InvalidCast | |||
There was a problem hiding this comment.
This change doesn't seem to match the title of the commit.
There was a problem hiding this comment.
Oh yes, somehow I messed these commit messages up! Fix incoming
Fixed
This means every function can be called from an immutable / pure context Note: ToArray will fail psalm on this commit as (rightfully) there is an impure psalm issue This commit is a bit of a lie to psalm: Psalm is correct that our object is not actually immutable / pure. But is if you remove caching. This lie seems tolerable (despite how bad a practice overriding the typechecker is) because the mutation is entirely memoization.
See previous commit. Psalm isn't actually wrong here, but we are willing to lie to psalm since the impurity is just an internal cache.
jarstelfox
force-pushed
the
clarify-psalm-types
branch
from
caee04b
to
2e58f99
Compare
This happened due to a bad find replace
|
We should be good to go now 😊 |
|
Thank you both! I'll be merging and releasing. FYI there is #113 that might interest you as well. |
I noticed that some of the Psalm types added in #111 could be made more precise, so I cleaned them up a bit.
I'm very interested in feedback on my decision to mark all the static methods as
external-mutation-freerather thanmutation-free. It's more correct to useexternal-mutation-freeas I've done, but then it's invalid to call the Enum functions in immutable contexts:is disallowed in a function marked
psalm-immutable, because of the internal mutation of the cache array. The mutation seems insignificant, since it could be omitted with no change to the results (other than in speed), so I'd like to find a way around this.I can see two approaches which would allow the use of Enums in immutable contexts (three, if you count the user ignoring the Psalm errors):
toArrayis immmutable. This seems tolerable (despite how bad a practice overriding the typechecker is) because the mutation is entirely memoization. If PHP were a lazy language, no caching would be necessary in the first place.toArrayin the constructor and look up the value there.__callStaticcan in fact be markedpureif this approach is taken. I've written the code for this in the iFixit/php-enum@clarify-psalm-types...iFixit:make-callstatic-pure branch; feel free to ask for a PR from that if you like that approach.