Added new type-safe API that allows validating a given value against a given `class-string<Enum>` #143

Ocramius · 2021-03-11T17:48:31Z

This patch introduces two new methods:

Enum::isValidEnumValue()
Enum::assertValidEnumValue()

The Enum::isValidEnumValue() is still wonky due to vimeo/psalm#5372, but
overall, this allows for matching against other Enum sub-types, by validating that a given $value
is effectively a subtype of T within an Enum<T>.

The previous approach did not work, because static method types are not coupled with the class they
are put on (they are effectively namespaced functions, and not much else).

Detailed explanation of what was done is available commit-by-commit, but the idea is that it is impossible to validate T for a given Enum<T> when a static method is used to do so, because static methods, by definition, are not coupled to the class they live on.

Therefore, I had to be a bit more creative: Enum::assertValidEnumValue(ConcreteEnumClass::class, $value) is to be used instead.

TODOs:

discuss deprecation, and if they are needed at all
assert-if-true and template types that involve union types are not correctly recognized vimeo/psalm#5372
discuss naming of new API

/cc @michaelpetri @guidobonuzzi @DavideBicego

These added tests verify that: * `Enum::assert*` methods can be used in a pure form * `Enum::from*` methods can be used in a pure form * `Enum#__construct()` properly binds the instance type of the enumerable with the input value * `Enum::assert*` methods currently **CANNOT** bind the input to an enumerable potential value (BUG) * `Enum::from*` methods currently **CANNOT** bind the input to an enumerable potential value (BUG) In addition to that, `psalm.xml` has been made a bit stricter: * `resolveFromConfigFile` was removed (we were using the default value) * `findUnusedPsalmSuppress` was added, allowing us to find whether intentional suppressions are now invalid. This allows us to verify negative scenarios, in which we **expect** a type error to appear * `restrictReturnTypes` better refines templated types, requiring inputs/outputs to match for those too

…ound to any template These functions had a `T` template parameter in them, but this `T` is completely decoupled from the one defined at class-level (and therefore at `Enum` instance level, rather than statically): * `Enum::from()` * `Enum::isValid()` * `Enum::assertValidValue()` * `Enum::assertValidValueReturningKey()` (internal detail) In practice, this means that myclabs#135 (Added new named constructor to create enum from mixed) was not a valid approach to infer types for enumerable values, when performed statically. A new approach will be attempted, but the current one will likely need to be scrapped for now. In practice, `@psalm-assert`, `@psalm-assert-if-true` and `@psalm-return static<T>` had no effect on these methods, due to a design-level issue that wasn't spotted (by myself either) at review.

…ide effects

…a given `class-string<Enum>` This patch introduces two new methods: * `Enum::isValidEnumValue()` * `Enum::assertValidEnumValue()` The `Enum::isValidEnumValue()` is still wonky due to vimeo/psalm#5372, but overall, this allows for matching against other `Enum` sub-types, by validating that a given `$value` is effectively a subtype of `T` within an `Enum<T>`. The previous approach did **not** work, because static method types are not coupled with the class they are put on (they are effectively namespaced functions, and not much else). In here, we also: * completed test coverage for `Enum::from()` * pinned `vimeo/psalm:4.6.2`, mostly for build stability * added coverage mapping for `phpunit.xml`

Ocramius · 2021-03-11T17:49:10Z

composer.json

@@ -28,6 +28,6 @@
    "require-dev": {
        "phpunit/phpunit": "^9.5",
        "squizlabs/php_codesniffer": "1.*",
-        "vimeo/psalm": "^4.6.2"
+        "vimeo/psalm": "4.6.2"


Pinned: changing this dependency is too fragile for builds to be kept stable.

My suggestion is to instead use dependabot to regularly keep it updated.

Ocramius · 2021-03-11T17:50:26Z

static-analysis/EnumInstantiation.php

+}
+
+/** @psalm-pure */
+function canCallFromWithKnownValue(): InstantiatedEnum


Notice how we CANNOT infer InstantiatedEnum<'C'> from this signature: this is a known problem with Enum::from() (static method), as I've described in the patch overview.

Ocramius · 2021-03-11T17:50:41Z

static-analysis/EnumIsPure.php

@@ -11,7 +11,7 @@
 * @method static PureEnum C()
 *
 * @psalm-immutable
- * @psalm-template T of 'A'|'B'
+ * @psalm-template T of 'A'|'C'


This was a bug with this file - otherwise all good.

Ocramius · 2021-03-11T17:51:25Z

static-analysis/EnumValidation.php

+ * @psalm-return 'A'|'C'
+ *
+ * @psalm-suppress MixedReturnStatement
+ * @psalm-suppress MixedInferredReturnType at the time of this writing, we did not yet find


This comment is kinda oudated - perhaps I can drop the static analysis check overall, now that I designed Enum::assertValidEnumValue() 🤔

Ocramius · 2021-03-11T17:51:36Z

static-analysis/EnumValidation.php

+ * @psalm-return 'A'|'C'
+ *
+ * @psalm-suppress MixedReturnStatement
+ * @psalm-suppress MixedInferredReturnType at the time of this writing, we did not yet find


This comment is kinda oudated - perhaps I can drop the static analysis check overall, now that I designed Enum::isValidEnumValue() 🤔

Ocramius · 2021-03-11T17:51:47Z

static-analysis/EnumValidation.php

+ * @psalm-return 'A'|'C'|1
+ *
+ * @psalm-suppress InvalidReturnType https://github.com/vimeo/psalm/issues/5372
+ * @psalm-suppress InvalidReturnStatement https://github.com/vimeo/psalm/issues/5372


vimeo/psalm#5372 is still kinda blocking this

Ocramius · 2021-03-11T17:52:56Z

src/Enum.php

     * @return bool
+     *
+     * deprecated use {@see Enum::isValidEnumValue()} instead


This is not yet @deprecated - should I mark it as such, or should I drop the comment and leave it be?

Ocramius · 2021-03-11T17:53:07Z

src/Enum.php

+     * @psalm-pure
+     * @param mixed $value
+     *
+     * deprecated use {@see Enum::assertValidEnumValue()} instead


This is not yet @deprecated - should I mark it as such, or should I drop the comment and leave it be?

simPod · 2021-03-11T18:10:51Z

psalm.xml

    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
    xmlns="https://getpsalm.org/schema/config"
    xsi:schemaLocation="https://getpsalm.org/schema/config vendor/vimeo/psalm/config.xsd"
+    restrictReturnTypes="true"
+    findUnusedPsalmSuppress="true"
+    totallyTyped="true"


Psalm docs say this is deprecated, might be dropped as level is max already

https://psalm.dev/docs/running_psalm/configuration/#totallytyped

WithtotallyTyped="true" the default level is 1, but without totallyTyped it would be 2: https://github.com/vimeo/psalm/pull/5340/files
So I think we should replace totallyTyped="true" with errorLevel="1".

Agree, it's a bit out of scope but has been touched already so 🤷🏾

Yeah, I generally remove errorLevel wherever I go: it's just confusing.

mnapoli · 2021-03-13T16:50:24Z

Hey! Just to be sure I understand, these 2 new methods are only useful for static analysis right?

If we can avoid that that would be great though (I'm not sold on deprecating current methods which are simpler to use).

How about:

public static function isValid($value, ?string $enumType = null)

The new parameter would be optional, and only those using static analysis would go through the trouble of providing the class name. Would that work?

Ocramius · 2021-03-18T16:38:42Z

Heads up: spent a lot of time debugging issues around this today, and will certainly work more on the patch as soon as I can, but I think what I'll do for now is removing <T> from these existing methods, so we fix the immediate issue (will open a new PR for that).

Ocramius · 2021-03-18T16:41:16Z

(I'm not sold on deprecating current methods which are simpler to use).

Agree, probably better to just leave them be.

The new parameter would be optional, and only those using static analysis would go through the trouble of providing the class name. Would that work?

That is an excellent question, and I do indeed need to try it out! 👍

boesing · 2021-07-14T13:20:17Z

I recently stumbled upon this as well. If there is something I can help with @Ocramius, let me know.

Ocramius added 5 commits March 11, 2021 12:50

Marked Enum::from() as @psalm-pure, since it does not cause any s…

6262a50

…ide effects

Removed suppressions that no longer apply as per vimeo/psalm:4.6.2

74b9e22

Ocramius commented Mar 11, 2021

View reviewed changes

simPod approved these changes Mar 11, 2021

View reviewed changes

guidobonuzzi approved these changes Mar 12, 2021

View reviewed changes

gharlan mentioned this pull request Mar 14, 2021

Better support for static<T> vimeo/psalm#5383

Closed

Ocramius mentioned this pull request Jul 4, 2021

Fix psalm annotation on Enum::from #146

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new type-safe API that allows validating a given value against a given `class-string<Enum>` #143

Added new type-safe API that allows validating a given value against a given `class-string<Enum>` #143

Ocramius commented Mar 11, 2021 •

edited

Loading

Ocramius Mar 11, 2021

Ocramius Mar 11, 2021

Ocramius Mar 11, 2021

Ocramius Mar 11, 2021

Ocramius Mar 11, 2021

Ocramius Mar 11, 2021

Ocramius Mar 11, 2021

Ocramius Mar 11, 2021

simPod Mar 11, 2021

gharlan Mar 11, 2021

simPod Mar 11, 2021

Ocramius Mar 18, 2021

mnapoli commented Mar 13, 2021

Ocramius commented Mar 18, 2021

Ocramius commented Mar 18, 2021

boesing commented Jul 14, 2021

Added new type-safe API that allows validating a given value against a given class-string<Enum> #143

Are you sure you want to change the base?

Added new type-safe API that allows validating a given value against a given class-string<Enum> #143

Conversation

Ocramius commented Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mnapoli commented Mar 13, 2021

Ocramius commented Mar 18, 2021

Ocramius commented Mar 18, 2021

boesing commented Jul 14, 2021

Added new type-safe API that allows validating a given value against a given `class-string<Enum>` #143

Added new type-safe API that allows validating a given value against a given `class-string<Enum>` #143

Ocramius commented Mar 11, 2021 •

edited

Loading