From e8bb1f045f0c64d4dd3dca54d56489b074590cf5 Mon Sep 17 00:00:00 2001 From: Romain Canon Date: Wed, 27 Mar 2024 13:48:21 +0100 Subject: [PATCH] fix: properly handle nested unresolvable type during mapping This change brings two enhancements: - When mapping to a structure that contains a nested unresolvable type, for instance a doc-block type that does not match the native type, the mapper will detect it and throw a proper exception (it used to just crash on a badly designed assertion). - Unresolvable types that are not used during mapping, for instance parameters of methods that are not used by the mapper will no longer throw an exception even if not needed by the library. Instead, an unresolvable type is assigned to these invalid properties/parameters/ return types. --- rector.php | 1 + src/Definition/Exception/TypesDoNotMatch.php | 31 ---------- .../Reflection/ReflectionTypeResolver.php | 3 +- .../TypeErrorDuringArgumentsMapping.php | 22 +++++++ .../Exception/TypeErrorDuringMapping.php | 22 +++++++ .../Tree/Exception/UnresolvableShellType.php | 17 ++++++ src/Mapper/Tree/Shell.php | 5 +- src/Mapper/TypeArgumentsMapper.php | 8 ++- src/Mapper/TypeTreeMapper.php | 8 ++- src/Type/Types/UnresolvableType.php | 19 ++++++ .../Mapping/TypeErrorDuringMappingTest.php | 61 +++++++++++++++++++ ...eflectionClassDefinitionRepositoryTest.php | 49 +++------------ 12 files changed, 168 insertions(+), 78 deletions(-) delete mode 100644 src/Definition/Exception/TypesDoNotMatch.php create mode 100644 src/Mapper/Exception/TypeErrorDuringArgumentsMapping.php create mode 100644 src/Mapper/Exception/TypeErrorDuringMapping.php create mode 100644 src/Mapper/Tree/Exception/UnresolvableShellType.php create mode 100644 tests/Integration/Mapping/TypeErrorDuringMappingTest.php diff --git a/rector.php b/rector.php index 96e45509..f4fd0a67 100644 --- a/rector.php +++ b/rector.php @@ -35,6 +35,7 @@ ReadOnlyPropertyRector::class, MixedTypeRector::class => [ __DIR__ . '/tests/Unit/Definition/Repository/Reflection/ReflectionClassDefinitionRepositoryTest', + __DIR__ . '/tests/Integration/Mapping/TypeErrorDuringMappingTest.php', ], NullToStrictStringFuncCallArgRector::class => [ __DIR__ . '/tests/Traits/TestIsSingleton.php', diff --git a/src/Definition/Exception/TypesDoNotMatch.php b/src/Definition/Exception/TypesDoNotMatch.php deleted file mode 100644 index 603b9497..00000000 --- a/src/Definition/Exception/TypesDoNotMatch.php +++ /dev/null @@ -1,31 +0,0 @@ -toString()}` (docblock) does not accept `{$typeFromReflection->toString()}` (native)."; - } elseif ($reflection instanceof ReflectionParameter) { - $message = "Types for parameter `$signature` do not match: `{$typeFromDocBlock->toString()}` (docblock) does not accept `{$typeFromReflection->toString()}` (native)."; - } else { - $message = "Return types for method `$signature` do not match: `{$typeFromDocBlock->toString()}` (docblock) does not accept `{$typeFromReflection->toString()}` (native)."; - } - - parent::__construct($message, 1638471381); - } -} diff --git a/src/Definition/Repository/Reflection/ReflectionTypeResolver.php b/src/Definition/Repository/Reflection/ReflectionTypeResolver.php index 9b6e01fd..4135f0ec 100644 --- a/src/Definition/Repository/Reflection/ReflectionTypeResolver.php +++ b/src/Definition/Repository/Reflection/ReflectionTypeResolver.php @@ -4,7 +4,6 @@ namespace CuyZ\Valinor\Definition\Repository\Reflection; -use CuyZ\Valinor\Definition\Exception\TypesDoNotMatch; use CuyZ\Valinor\Type\GenericType; use CuyZ\Valinor\Type\Parser\Exception\InvalidType; use CuyZ\Valinor\Type\Parser\TypeParser; @@ -51,7 +50,7 @@ public function resolveType(ReflectionProperty|ReflectionParameter|ReflectionFun } if (! $typeFromDocBlock->matches($nativeType)) { - throw new TypesDoNotMatch($reflection, $typeFromDocBlock, $nativeType); + return UnresolvableType::forDocBlockTypeNotMatchingNative($reflection, $typeFromDocBlock, $nativeType); } return $typeFromDocBlock; diff --git a/src/Mapper/Exception/TypeErrorDuringArgumentsMapping.php b/src/Mapper/Exception/TypeErrorDuringArgumentsMapping.php new file mode 100644 index 00000000..3ccbc02f --- /dev/null +++ b/src/Mapper/Exception/TypeErrorDuringArgumentsMapping.php @@ -0,0 +1,22 @@ +signature`: {$exception->getMessage()}", + 1711534351, + $exception, + ); + } +} diff --git a/src/Mapper/Exception/TypeErrorDuringMapping.php b/src/Mapper/Exception/TypeErrorDuringMapping.php new file mode 100644 index 00000000..21b443c5 --- /dev/null +++ b/src/Mapper/Exception/TypeErrorDuringMapping.php @@ -0,0 +1,22 @@ +toString()}`: {$exception->getMessage()}", + 1711526329, + $exception, + ); + } +} diff --git a/src/Mapper/Tree/Exception/UnresolvableShellType.php b/src/Mapper/Tree/Exception/UnresolvableShellType.php new file mode 100644 index 00000000..55529733 --- /dev/null +++ b/src/Mapper/Tree/Exception/UnresolvableShellType.php @@ -0,0 +1,17 @@ +message()); + } +} diff --git a/src/Mapper/Tree/Shell.php b/src/Mapper/Tree/Shell.php index ec308242..ca87bafe 100644 --- a/src/Mapper/Tree/Shell.php +++ b/src/Mapper/Tree/Shell.php @@ -5,6 +5,7 @@ namespace CuyZ\Valinor\Mapper\Tree; use CuyZ\Valinor\Definition\Attributes; +use CuyZ\Valinor\Mapper\Tree\Exception\UnresolvableShellType; use CuyZ\Valinor\Type\Type; use CuyZ\Valinor\Type\Types\UnresolvableType; @@ -27,7 +28,9 @@ final class Shell private function __construct(private Type $type) { - assert(! $type instanceof UnresolvableType); + if ($type instanceof UnresolvableType) { + throw new UnresolvableShellType($type); + } } public static function root(Type $type, mixed $value): self diff --git a/src/Mapper/TypeArgumentsMapper.php b/src/Mapper/TypeArgumentsMapper.php index dfa4a21e..f0d73f81 100644 --- a/src/Mapper/TypeArgumentsMapper.php +++ b/src/Mapper/TypeArgumentsMapper.php @@ -6,7 +6,9 @@ use CuyZ\Valinor\Definition\ParameterDefinition; use CuyZ\Valinor\Definition\Repository\FunctionDefinitionRepository; +use CuyZ\Valinor\Mapper\Exception\TypeErrorDuringArgumentsMapping; use CuyZ\Valinor\Mapper\Tree\Builder\RootNodeBuilder; +use CuyZ\Valinor\Mapper\Tree\Exception\UnresolvableShellType; use CuyZ\Valinor\Mapper\Tree\Shell; use CuyZ\Valinor\Type\Types\ShapedArrayElement; use CuyZ\Valinor\Type\Types\ShapedArrayType; @@ -42,7 +44,11 @@ public function mapArguments(callable $callable, mixed $source): array $type = new ShapedArrayType(...$elements); $shell = Shell::root($type, $source); - $node = $this->nodeBuilder->build($shell); + try { + $node = $this->nodeBuilder->build($shell); + } catch (UnresolvableShellType $exception) { + throw new TypeErrorDuringArgumentsMapping($function, $exception); + } if (! $node->isValid()) { throw new ArgumentsMapperError($function, $node->node()); diff --git a/src/Mapper/TypeTreeMapper.php b/src/Mapper/TypeTreeMapper.php index a9b4194c..d7691dc1 100644 --- a/src/Mapper/TypeTreeMapper.php +++ b/src/Mapper/TypeTreeMapper.php @@ -5,8 +5,10 @@ namespace CuyZ\Valinor\Mapper; use CuyZ\Valinor\Mapper\Exception\InvalidMappingTypeSignature; +use CuyZ\Valinor\Mapper\Exception\TypeErrorDuringMapping; use CuyZ\Valinor\Mapper\Tree\Builder\RootNodeBuilder; use CuyZ\Valinor\Mapper\Tree\Builder\TreeNode; +use CuyZ\Valinor\Mapper\Tree\Exception\UnresolvableShellType; use CuyZ\Valinor\Mapper\Tree\Shell; use CuyZ\Valinor\Type\Parser\Exception\InvalidType; use CuyZ\Valinor\Type\Parser\TypeParser; @@ -41,6 +43,10 @@ private function node(string $signature, mixed $source): TreeNode $shell = Shell::root($type, $source); - return $this->nodeBuilder->build($shell); + try { + return $this->nodeBuilder->build($shell); + } catch (UnresolvableShellType $exception) { + throw new TypeErrorDuringMapping($type, $exception); + } } } diff --git a/src/Type/Types/UnresolvableType.php b/src/Type/Types/UnresolvableType.php index a212e124..08664c96 100644 --- a/src/Type/Types/UnresolvableType.php +++ b/src/Type/Types/UnresolvableType.php @@ -7,8 +7,12 @@ use CuyZ\Valinor\Type\ClassType; use CuyZ\Valinor\Type\Parser\Exception\InvalidType; use CuyZ\Valinor\Type\Type; +use CuyZ\Valinor\Utility\Reflection\Reflection; use CuyZ\Valinor\Utility\ValueDumper; use LogicException; +use ReflectionFunctionAbstract; +use ReflectionParameter; +use ReflectionProperty; /** @internal */ final class UnresolvableType implements Type @@ -62,6 +66,21 @@ public static function forInvalidParameterDefaultValue(string $signature, Type $ ); } + public static function forDocBlockTypeNotMatchingNative(ReflectionProperty|ReflectionParameter|ReflectionFunctionAbstract $reflection, Type $typeFromDocBlock, Type $typeFromReflection): self + { + $signature = Reflection::signature($reflection); + + if ($reflection instanceof ReflectionProperty) { + $message = "Types for property `$signature` do not match: `{$typeFromDocBlock->toString()}` (docblock) does not accept `{$typeFromReflection->toString()}` (native)."; + } elseif ($reflection instanceof ReflectionParameter) { + $message = "Types for parameter `$signature` do not match: `{$typeFromDocBlock->toString()}` (docblock) does not accept `{$typeFromReflection->toString()}` (native)."; + } else { + $message = "Return types for method `$signature` do not match: `{$typeFromDocBlock->toString()}` (docblock) does not accept `{$typeFromReflection->toString()}` (native)."; + } + + return new self($typeFromDocBlock->toString(), $message); + } + public static function forLocalAlias(string $raw, string $name, ClassType $type, InvalidType $exception): self { return new self( diff --git a/tests/Integration/Mapping/TypeErrorDuringMappingTest.php b/tests/Integration/Mapping/TypeErrorDuringMappingTest.php new file mode 100644 index 00000000..c762092a --- /dev/null +++ b/tests/Integration/Mapping/TypeErrorDuringMappingTest.php @@ -0,0 +1,61 @@ +expectException(TypeErrorDuringMapping::class); + $this->expectExceptionCode(1711526329); + $this->expectExceptionMessage("Error while trying to map to `$class`: Types for property `$class::\$propertyWithNotMatchingTypes` do not match: `string` (docblock) does not accept `bool` (native)."); + + $this->mapperBuilder()->mapper()->map($class, ['propertyWithNotMatchingTypes' => true]); + } + + public function test_parameter_with_non_matching_types_throws_exception(): void + { + $class = (new class (true) { + /** + * @param string $parameterWithNotMatchingTypes + * @phpstan-ignore-next-line + */ + public function __construct(public bool $parameterWithNotMatchingTypes) {} + })::class; + + $this->expectException(TypeErrorDuringMapping::class); + $this->expectExceptionCode(1711526329); + $this->expectExceptionMessage("Error while trying to map to `$class`: Types for parameter `$class::__construct(\$parameterWithNotMatchingTypes)` do not match: `string` (docblock) does not accept `bool` (native)."); + + $this->mapperBuilder()->mapper()->map($class, ['parameterWithNotMatchingTypes' => true]); + } + + public function test_function_parameter_with_non_matching_types_throws_exception(): void + { + $function = + /** + * @param string $parameterWithNotMatchingTypes + */ + fn (bool $parameterWithNotMatchingTypes): string => 'foo'; + + $this->expectException(TypeErrorDuringArgumentsMapping::class); + $this->expectExceptionCode(1711534351); + $this->expectExceptionMessageMatches("/Could not map arguments of `.*`: Types for parameter `.*` do not match: `string` \(docblock\) does not accept `bool` \(native\)\./"); + + $this->mapperBuilder()->argumentsMapper()->mapArguments($function, ['parameterWithNotMatchingTypes' => true]); + } +} diff --git a/tests/Unit/Definition/Repository/Reflection/ReflectionClassDefinitionRepositoryTest.php b/tests/Unit/Definition/Repository/Reflection/ReflectionClassDefinitionRepositoryTest.php index 9ab31b6d..8cfcc605 100644 --- a/tests/Unit/Definition/Repository/Reflection/ReflectionClassDefinitionRepositoryTest.php +++ b/tests/Unit/Definition/Repository/Reflection/ReflectionClassDefinitionRepositoryTest.php @@ -11,7 +11,6 @@ use CuyZ\Valinor\Definition\Exception\InvalidTypeAliasImportClass; use CuyZ\Valinor\Definition\Exception\InvalidTypeAliasImportClassType; use CuyZ\Valinor\Definition\Exception\SeveralExtendTagsFound; -use CuyZ\Valinor\Definition\Exception\TypesDoNotMatch; use CuyZ\Valinor\Definition\Exception\UnknownTypeAliasImport; use CuyZ\Valinor\Definition\Repository\Reflection\ReflectionClassDefinitionRepository; use CuyZ\Valinor\Tests\Fake\Type\FakeType; @@ -177,7 +176,6 @@ public function test_invalid_property_type_throws_exception(): void $type = $class->properties->get('propertyWithInvalidType')->type; self::assertInstanceOf(UnresolvableType::class, $type); - /** @var UnresolvableType $type */ self::assertMatchesRegularExpression('/^The type `InvalidType` for property `.*` could not be resolved: .*$/', $type->message()); } @@ -195,23 +193,6 @@ public function test_invalid_property_default_value_throws_exception(): void self::assertMatchesRegularExpression('/Property `.*::\$propertyWithInvalidDefaultValue` of type `string` has invalid default value false/', $type->message()); } - public function test_property_with_non_matching_types_throws_exception(): void - { - $class = (new class () { - /** - * @phpstan-ignore-next-line - * @var string - */ - public bool $propertyWithNotMatchingTypes; - })::class; - - $this->expectException(TypesDoNotMatch::class); - $this->expectExceptionCode(1638471381); - $this->expectExceptionMessage("Types for property `$class::\$propertyWithNotMatchingTypes` do not match: `string` (docblock) does not accept `bool` (native)."); - - $this->repository->for(new NativeClassType($class)); - } - public function test_invalid_parameter_type_throws_exception(): void { $class = (new class () { @@ -228,7 +209,6 @@ public function publicMethod($parameterWithInvalidType): void {} $type = $class->methods->get('publicMethod')->parameters->get('parameterWithInvalidType')->type; self::assertInstanceOf(UnresolvableType::class, $type); - /** @var UnresolvableType $type */ self::assertMatchesRegularExpression('/^The type `InvalidTypeWithPendingSpaces` for parameter `.*` could not be resolved: .*$/', $type->message()); } @@ -246,7 +226,6 @@ public function publicMethod($parameterWithInvalidType): void {} $type = $class->methods->get('publicMethod')->returnType; self::assertInstanceOf(UnresolvableType::class, $type); - /** @var UnresolvableType $type */ self::assertMatchesRegularExpression('/^The type `InvalidType` for return type of method `.*` could not be resolved: .*$/', $type->message()); } @@ -267,23 +246,6 @@ public function publicMethod($parameterWithInvalidDefaultValue = false): void {} self::assertMatchesRegularExpression('/Parameter `.*::publicMethod\(\$parameterWithInvalidDefaultValue\)` of type `string` has invalid default value false/', $type->message()); } - public function test_parameter_with_non_matching_types_throws_exception(): void - { - $class = (new class () { - /** - * @param string $parameterWithNotMatchingTypes - * @phpstan-ignore-next-line - */ - public function publicMethod(bool $parameterWithNotMatchingTypes): void {} - })::class; - - $this->expectException(TypesDoNotMatch::class); - $this->expectExceptionCode(1638471381); - $this->expectExceptionMessage("Types for parameter `$class::publicMethod(\$parameterWithNotMatchingTypes)` do not match: `string` (docblock) does not accept `bool` (native)."); - - $this->repository->for(new NativeClassType($class)); - } - public function test_method_with_non_matching_return_types_throws_exception(): void { $class = (new class () { @@ -297,11 +259,14 @@ public function publicMethod(): string } })::class; - $this->expectException(TypesDoNotMatch::class); - $this->expectExceptionCode(1638471381); - $this->expectExceptionMessage("Return types for method `$class::publicMethod()` do not match: `bool` (docblock) does not accept `string` (native)."); + $returnType = $this->repository + ->for(new NativeClassType($class)) + ->methods + ->get('publicMethod') + ->returnType; - $this->repository->for(new NativeClassType($class)); + self::assertInstanceOf(UnresolvableType::class, $returnType); + self::assertMatchesRegularExpression('/^Return types for method `.*` do not match: `bool` \(docblock\) does not accept `string` \(native\).$/', $returnType->message()); } public function test_class_with_local_type_alias_name_duplication_throws_exception(): void