From 96598dad114b05bc973ff916074c0e0daa70e8e3 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Thu, 11 Mar 2021 12:50:36 +0100 Subject: [PATCH 1/5] Better type tests around instantiation and validation scenarios 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 --- psalm.xml | 7 ++-- static-analysis/EnumInstantiation.php | 48 ++++++++++++++++++++++++ static-analysis/EnumIsPure.php | 2 +- static-analysis/EnumValidation.php | 54 +++++++++++++++++++++++++++ 4 files changed, 106 insertions(+), 5 deletions(-) create mode 100644 static-analysis/EnumInstantiation.php create mode 100644 static-analysis/EnumValidation.php diff --git a/psalm.xml b/psalm.xml index ff06b66..24ed533 100644 --- a/psalm.xml +++ b/psalm.xml @@ -1,10 +1,11 @@ @@ -16,8 +17,6 @@ - - diff --git a/static-analysis/EnumInstantiation.php b/static-analysis/EnumInstantiation.php new file mode 100644 index 0000000..322cca8 --- /dev/null +++ b/static-analysis/EnumInstantiation.php @@ -0,0 +1,48 @@ + + */ +final class InstantiatedEnum extends Enum +{ + const A = 'A'; + const C = 'C'; +} + +/** + * @psalm-pure + * @psalm-return InstantiatedEnum<'A'> + */ +function canCallConstructorWithConstantValue(): InstantiatedEnum +{ + return new InstantiatedEnum('A'); +} + +/** + * @psalm-pure + * @psalm-return InstantiatedEnum<'C'> + */ +function canCallConstructorWithConstantReference(): InstantiatedEnum +{ + return new InstantiatedEnum(InstantiatedEnum::C); +} + +/** @psalm-pure */ +function canCallFromWithKnownValue(): InstantiatedEnum +{ + return InstantiatedEnum::from('C'); +} + +/** @psalm-pure */ +function canCallFromWithUnknownValue(): InstantiatedEnum +{ + return InstantiatedEnum::from(123123); +} diff --git a/static-analysis/EnumIsPure.php b/static-analysis/EnumIsPure.php index 5875fd8..696694e 100644 --- a/static-analysis/EnumIsPure.php +++ b/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' * @template-extends Enum */ final class PureEnum extends Enum diff --git a/static-analysis/EnumValidation.php b/static-analysis/EnumValidation.php new file mode 100644 index 0000000..df72a0f --- /dev/null +++ b/static-analysis/EnumValidation.php @@ -0,0 +1,54 @@ + + */ +final class ValidationEnum extends Enum +{ + const A = 'A'; + const C = 'C'; +} + +/** + * @psalm-pure + * @param mixed $input + * @psalm-return 'A'|'C' + * + * @psalm-suppress MixedReturnStatement + * @psalm-suppress MixedInferredReturnType at the time of this writing, we did not yet find + * a proper approach to constraint input values through + * validation via static methods. + */ +function canValidateValue($input): string +{ + ValidationEnum::assertValidValue($input); + + return $input; +} + +/** + * @psalm-pure + * @param mixed $input + * @psalm-return 'A'|'C' + * + * @psalm-suppress MixedReturnStatement + * @psalm-suppress MixedInferredReturnType at the time of this writing, we did not yet find + * a proper approach to constraint input values through + * validation via static methods. + */ +function canValidateValueThroughIsValid($input): string +{ + if (! ValidationEnum::isValid($input)) { + throw new \InvalidArgumentException('Value not valid'); + } + + return $input; +} From ec76a45b9581089d63d304a248014f67d609c31d Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Thu, 11 Mar 2021 12:55:31 +0100 Subject: [PATCH 2/5] Removed invalid `T` template parameter from methods where it is not bound 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 #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` had no effect on these methods, due to a design-level issue that wasn't spotted (by myself either) at review. --- src/Enum.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Enum.php b/src/Enum.php index 6967ab5..1cc30b6 100644 --- a/src/Enum.php +++ b/src/Enum.php @@ -95,7 +95,7 @@ public function __wakeup() /** * @param mixed $value * @return static - * @psalm-return static + * @psalm-return static */ public static function from($value): self { @@ -212,7 +212,6 @@ public static function toArray() * @param $value * @psalm-param mixed $value * @psalm-pure - * @psalm-assert-if-true T $value * @return bool */ public static function isValid($value) @@ -224,7 +223,6 @@ public static function isValid($value) * Asserts valid enum value * * @psalm-pure - * @psalm-assert T $value * @param mixed $value */ public static function assertValidValue($value): void @@ -236,7 +234,6 @@ public static function assertValidValue($value): void * Asserts valid enum value * * @psalm-pure - * @psalm-assert T $value * @param mixed $value * @return string */ From 6262a50fad59aa48d6c4e0937d12ce7bd4aba8f3 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Thu, 11 Mar 2021 12:56:10 +0100 Subject: [PATCH 3/5] Marked `Enum::from()` as `@psalm-pure`, since it does not cause any side effects --- src/Enum.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Enum.php b/src/Enum.php index 1cc30b6..9e1298c 100644 --- a/src/Enum.php +++ b/src/Enum.php @@ -93,6 +93,7 @@ public function __wakeup() } /** + * @psalm-pure * @param mixed $value * @return static * @psalm-return static From 74b9e22141cc17661d4eeffc310ff838085bc174 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Thu, 11 Mar 2021 12:56:32 +0100 Subject: [PATCH 4/5] Removed suppressions that no longer apply as per `vimeo/psalm:4.6.2` --- src/Enum.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Enum.php b/src/Enum.php index 9e1298c..cda3a38 100644 --- a/src/Enum.php +++ b/src/Enum.php @@ -128,7 +128,6 @@ public function getKey() /** * @psalm-pure - * @psalm-suppress InvalidCast * @return string */ public function __toString() @@ -188,7 +187,6 @@ public static function values() * Returns all possible values as an array * * @psalm-pure - * @psalm-suppress ImpureStaticProperty * * @psalm-return array * @return array Constant name in key, constant value in value From be511e46008299829b2cc331181389a598f88de3 Mon Sep 17 00:00:00 2001 From: Marco Pivetta Date: Thu, 11 Mar 2021 18:44:55 +0100 Subject: [PATCH 5/5] Added new type-safe API that allows validating a given value against a given `class-string` This patch introduces two new methods: * `Enum::isValidEnumValue()` * `Enum::assertValidEnumValue()` The `Enum::isValidEnumValue()` is still wonky due to https://github.com/vimeo/psalm/issues/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`. 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` --- composer.json | 2 +- phpunit.xml | 5 ++++ src/Enum.php | 32 +++++++++++++++++++++ static-analysis/EnumValidation.php | 29 +++++++++++++++++++ tests/EnumTest.php | 46 +++++++++++++++++++++++++++++- 5 files changed, 112 insertions(+), 2 deletions(-) diff --git a/composer.json b/composer.json index 924f924..6d37f2f 100644 --- a/composer.json +++ b/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" } } diff --git a/phpunit.xml b/phpunit.xml index bd714f1..13b4060 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -10,4 +10,9 @@ ./tests + + + src + + diff --git a/src/Enum.php b/src/Enum.php index cda3a38..5f8b068 100644 --- a/src/Enum.php +++ b/src/Enum.php @@ -212,23 +212,55 @@ public static function toArray() * @psalm-param mixed $value * @psalm-pure * @return bool + * + * deprecated use {@see Enum::isValidEnumValue()} instead */ public static function isValid($value) { return \in_array($value, static::toArray(), true); } + /** + * Asserts valid enum value + * + * @psalm-pure + * @psalm-template CheckedValueType + * @psalm-param class-string> $enumType + * @param mixed $value + * @psalm-assert-if-true CheckedValueType $value + */ + public static function isValidEnumValue(string $enumType, $value): bool + { + return $enumType::isValid($value); + } + /** * Asserts valid enum value * * @psalm-pure * @param mixed $value + * + * deprecated use {@see Enum::assertValidEnumValue()} instead */ public static function assertValidValue($value): void { self::assertValidValueReturningKey($value); } + /** + * Asserts valid enum value + * + * @psalm-pure + * @psalm-template ValueType + * @psalm-param class-string> $enumType + * @param mixed $value + * @psalm-assert ValueType $value + */ + public static function assertValidEnumValue(string $enumType, $value): void + { + $enumType::assertValidValue($value); + } + /** * Asserts valid enum value * diff --git a/static-analysis/EnumValidation.php b/static-analysis/EnumValidation.php index df72a0f..c6d7ba0 100644 --- a/static-analysis/EnumValidation.php +++ b/static-analysis/EnumValidation.php @@ -34,6 +34,18 @@ function canValidateValue($input): string return $input; } +/** + * @psalm-pure + * @param mixed $input + * @psalm-return 'A'|'C' + */ +function canAssertValidEnumValue($input): string +{ + ValidationEnum::assertValidEnumValue(ValidationEnum::class, $input); + + return $input; +} + /** * @psalm-pure * @param mixed $input @@ -52,3 +64,20 @@ function canValidateValueThroughIsValid($input): string return $input; } + +/** + * @psalm-pure + * @param mixed $input + * @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 + */ +function canValidateValueThroughIsValidEnumValue($input) +{ + if (! ValidationEnum::isValidEnumValue(ValidationEnum::class, $input)) { + return 1; + } + + return $input; +} diff --git a/tests/EnumTest.php b/tests/EnumTest.php index 6fcc5b1..37559ea 100755 --- a/tests/EnumTest.php +++ b/tests/EnumTest.php @@ -6,6 +6,8 @@ namespace MyCLabs\Tests\Enum; +use MyCLabs\Enum\Enum; + /** * @author Matthieu Napoli * @author Daniel Costa @@ -59,6 +61,27 @@ public function testFailToCreateEnumWithInvalidValueThroughNamedConstructor($val EnumFixture::from($value); } + /** + * @dataProvider invalidValueProvider + * @param mixed $value + */ + public function testRejectInvalidEnumValueWhenAssertingOnIt($value): void + { + $this->expectException(\UnexpectedValueException::class); + $this->expectExceptionMessage('is not part of the enum MyCLabs\Tests\Enum\EnumFixture'); + + Enum::assertValidEnumValue(EnumFixture::class, $value); + } + + /** + * @dataProvider invalidValueProvider + * @param mixed $value + */ + public function testReportsFalseOnNonValidEnumValueUponChecking($value): void + { + self::assertFalse(Enum::isValidEnumValue(EnumFixture::class, $value)); + } + public function testFailToCreateEnumWithEnumItselfThroughNamedConstructor(): void { $this->expectException(\UnexpectedValueException::class); @@ -67,6 +90,11 @@ public function testFailToCreateEnumWithEnumItselfThroughNamedConstructor(): voi EnumFixture::from(EnumFixture::FOO()); } + public function testCreateEnumWithValidEnumValueThroughNamedConstructor(): void + { + self::assertEquals(EnumFixture::FOO(), EnumFixture::from('foo')); + } + /** * Contains values not existing in EnumFixture * @return array @@ -179,7 +207,8 @@ public function testBadStaticAccess() */ public function testIsValid($value, $isValid) { - $this->assertSame($isValid, EnumFixture::isValid($value)); + self::assertSame($isValid, EnumFixture::isValid($value)); + self::assertSame($isValid, Enum::isValidEnumValue(EnumFixture::class, $value)); } public function isValidProvider() @@ -381,4 +410,19 @@ public function testAssertValidValue($value, $isValid): void self::assertTrue(EnumFixture::isValid($value)); } + + /** + * @dataProvider isValidProvider + */ + public function testAssertValidValueWithTypedApi($value, $isValid): void + { + if (!$isValid) { + $this->expectException(\UnexpectedValueException::class); + $this->expectExceptionMessage("Value '$value' is not part of the enum " . EnumFixture::class); + } + + Enum::assertValidEnumValue(EnumFixture::class, $value); + + self::assertTrue(Enum::isValidEnumValue(EnumFixture::class, $value)); + } }