From 58ea1e7b5a99c519cf53ff41081a7d73185cde64 Mon Sep 17 00:00:00 2001 From: romalytvynenko Date: Sun, 24 Nov 2024 13:41:21 +0200 Subject: [PATCH] Improved inference of inherited method calls (fixes issues when `parent::toArray()` calls in JSON resources were not working as expected with larger classes hierarchies) (#635) * improved static method calls analysis * wip parent call analysis * remove redundant code * Fix styling --------- Co-authored-by: romalytvynenko --- src/Infer/Analyzer/ClassAnalyzer.php | 1 + src/Infer/Definition/ClassDefinition.php | 30 +++++++++++++++++ .../Definition/FunctionLikeDefinition.php | 1 + .../Extensions/Event/MethodCallEvent.php | 1 + src/Infer/Handler/FunctionLikeHandler.php | 1 + src/Infer/Scope/Scope.php | 2 +- src/Infer/Services/ReferenceTypeResolver.php | 29 ++++++++++++++++- .../InferExtensions/JsonResourceExtension.php | 32 ++----------------- .../InferExtensions/ModelExtension.php | 6 +--- src/Support/Type/ObjectType.php | 3 ++ tests/InferExtensions/ModelExtensionTest.php | 5 ++- .../JsonResourceTypeToSchemaTest.php | 25 ++++++++++++++- 12 files changed, 98 insertions(+), 38 deletions(-) diff --git a/src/Infer/Analyzer/ClassAnalyzer.php b/src/Infer/Analyzer/ClassAnalyzer.php index e55692f7..a0e749e3 100644 --- a/src/Infer/Analyzer/ClassAnalyzer.php +++ b/src/Infer/Analyzer/ClassAnalyzer.php @@ -108,6 +108,7 @@ public function analyze(string $name): ClassDefinition returnType: new UnknownType, ), definingClassName: $name, + isStatic: $reflectionMethod->isStatic(), ); } diff --git a/src/Infer/Definition/ClassDefinition.php b/src/Infer/Definition/ClassDefinition.php index 4940d18a..eeb24a67 100644 --- a/src/Infer/Definition/ClassDefinition.php +++ b/src/Infer/Definition/ClassDefinition.php @@ -5,6 +5,7 @@ use Dedoc\Scramble\Infer\Analyzer\MethodAnalyzer; use Dedoc\Scramble\Infer\Reflector\ClassReflector; use Dedoc\Scramble\Infer\Scope\GlobalScope; +use Dedoc\Scramble\Infer\Scope\Index; use Dedoc\Scramble\Infer\Scope\NodeTypesResolver; use Dedoc\Scramble\Infer\Scope\Scope; use Dedoc\Scramble\Infer\Scope\ScopeContext; @@ -48,6 +49,35 @@ public function hasMethodDefinition(string $name): bool return array_key_exists($name, $this->methods); } + public function getMethodDefinitionWithoutAnalysis(string $name) + { + if (! array_key_exists($name, $this->methods)) { + return null; + } + + return $this->methods[$name]; + } + + public function getMethodDefiningClassName(string $name, Index $index) + { + $lastLookedUpClassName = $this->name; + while ($lastLookedUpClassDefinition = $index->getClassDefinition($lastLookedUpClassName)) { + if ($methodDefinition = $lastLookedUpClassDefinition->getMethodDefinitionWithoutAnalysis($name)) { + return $methodDefinition->definingClassName; + } + + if ($lastLookedUpClassDefinition->parentFqn) { + $lastLookedUpClassName = $lastLookedUpClassDefinition->parentFqn; + + continue; + } + + break; + } + + return $lastLookedUpClassName; + } + public function getMethodDefinition(string $name, Scope $scope = new GlobalScope, array $indexBuilders = []) { if (! array_key_exists($name, $this->methods)) { diff --git a/src/Infer/Definition/FunctionLikeDefinition.php b/src/Infer/Definition/FunctionLikeDefinition.php index d7475bc4..07b0e052 100644 --- a/src/Infer/Definition/FunctionLikeDefinition.php +++ b/src/Infer/Definition/FunctionLikeDefinition.php @@ -17,6 +17,7 @@ public function __construct( public array $sideEffects = [], public array $argumentsDefaults = [], public ?string $definingClassName = null, + public bool $isStatic = false, ) {} public function isFullyAnalyzed(): bool diff --git a/src/Infer/Extensions/Event/MethodCallEvent.php b/src/Infer/Extensions/Event/MethodCallEvent.php index 4d4c3659..68c7b9d6 100644 --- a/src/Infer/Extensions/Event/MethodCallEvent.php +++ b/src/Infer/Extensions/Event/MethodCallEvent.php @@ -15,6 +15,7 @@ public function __construct( public readonly string $name, public readonly Scope $scope, public readonly array $arguments, + public readonly ?string $methodDefiningClassName, ) {} public function getDefinition() diff --git a/src/Infer/Handler/FunctionLikeHandler.php b/src/Infer/Handler/FunctionLikeHandler.php index e66da521..6b9eba90 100644 --- a/src/Infer/Handler/FunctionLikeHandler.php +++ b/src/Infer/Handler/FunctionLikeHandler.php @@ -49,6 +49,7 @@ public function enter(FunctionLike $node, Scope $scope) $scope->context->setFunctionDefinition($fnDefinition = new FunctionLikeDefinition( type: $fnType = new FunctionType($node->name->name ?? 'anonymous'), sideEffects: [], + isStatic: $node instanceof Node\Stmt\ClassMethod ? $node->isStatic() : false, )); $fnDefinition->isFullyAnalyzed = true; diff --git a/src/Infer/Scope/Scope.php b/src/Infer/Scope/Scope.php index 2b147f0d..09b5b438 100644 --- a/src/Infer/Scope/Scope.php +++ b/src/Infer/Scope/Scope.php @@ -116,7 +116,7 @@ public function getType(Node $node): Type $calleeType = $this->getType($node->var); $event = $calleeType instanceof ObjectType - ? new MethodCallEvent($calleeType, $node->name->name, $this, $this->getArgsTypes($node->args)) + ? new MethodCallEvent($calleeType, $node->name->name, $this, $this->getArgsTypes($node->args), $calleeType->name) : null; $type = ($event ? app(ExtensionsBroker::class)->getMethodReturnType($event) : null) diff --git a/src/Infer/Services/ReferenceTypeResolver.php b/src/Infer/Services/ReferenceTypeResolver.php index 710b3d3f..5821e0c4 100644 --- a/src/Infer/Services/ReferenceTypeResolver.php +++ b/src/Infer/Services/ReferenceTypeResolver.php @@ -265,11 +265,14 @@ private function resolveMethodCallReferenceType(Scope $scope, MethodCallReferenc : $calleeType; if ($unwrappedType instanceof ObjectType) { + $classDefinition = $this->index->getClassDefinition($unwrappedType->name); + $event = new MethodCallEvent( instance: $unwrappedType, name: $type->methodName, scope: $scope, arguments: $type->arguments, + methodDefiningClassName: $classDefinition ? $classDefinition->getMethodDefiningClassName($type->methodName, $scope->index) : $unwrappedType->name, ); } @@ -314,12 +317,16 @@ private function resolveStaticMethodCallReferenceType(Scope $scope, StaticMethod $type->arguments, ); + $calleeName = $type->callee; $contextualClassName = $this->resolveClassName($scope, $type->callee); if (! $contextualClassName) { return new UnknownType; } $type->callee = $contextualClassName; + $isStaticCall = ! in_array($calleeName, StaticReference::KEYWORDS) + || (in_array($calleeName, StaticReference::KEYWORDS) && $scope->context->functionDefinition?->isStatic); + // Assuming callee here can be only string of known name. Reality is more complex than // that, but it is fine for now. @@ -329,7 +336,7 @@ private function resolveStaticMethodCallReferenceType(Scope $scope, StaticMethod $this->resolveUnknownClass($type->callee); // Attempting extensions broker before potentially giving up on type inference - if ($returnType = Context::getInstance()->extensionsBroker->getStaticMethodReturnType(new StaticMethodCallEvent( + if ($isStaticCall && $returnType = Context::getInstance()->extensionsBroker->getStaticMethodReturnType(new StaticMethodCallEvent( callee: $type->callee, name: $type->methodName, scope: $scope, @@ -338,6 +345,25 @@ private function resolveStaticMethodCallReferenceType(Scope $scope, StaticMethod return $returnType; } + // Attempting extensions broker before potentially giving up on type inference + if (! $isStaticCall && $scope->context->classDefinition) { + $definingMethodName = ($definingClass = $scope->index->getClassDefinition($contextualClassName)) + ? $definingClass->getMethodDefiningClassName($type->methodName, $scope->index) + : $contextualClassName; + + $returnType = Context::getInstance()->extensionsBroker->getMethodReturnType($e = new MethodCallEvent( + instance: $i = new ObjectType($scope->context->classDefinition->name), + name: $type->methodName, + scope: $scope, + arguments: $type->arguments, + methodDefiningClassName: $definingMethodName, + )); + + if ($returnType) { + return $returnType; + } + } + if (! array_key_exists($type->callee, $this->index->classesDefinitions)) { return new UnknownType; } @@ -747,6 +773,7 @@ private function getMethodCallsSideEffectIntroducedTypesInConstructor(Generic $t name: $se->methodName, scope: $scope, arguments: $se->arguments, + methodDefiningClassName: $type->name, )); } diff --git a/src/Support/InferExtensions/JsonResourceExtension.php b/src/Support/InferExtensions/JsonResourceExtension.php index 4abecfdb..8717bc54 100644 --- a/src/Support/InferExtensions/JsonResourceExtension.php +++ b/src/Support/InferExtensions/JsonResourceExtension.php @@ -4,10 +4,8 @@ use Dedoc\Scramble\Infer\Extensions\Event\MethodCallEvent; use Dedoc\Scramble\Infer\Extensions\Event\PropertyFetchEvent; -use Dedoc\Scramble\Infer\Extensions\Event\StaticMethodCallEvent; use Dedoc\Scramble\Infer\Extensions\MethodReturnTypeExtension; use Dedoc\Scramble\Infer\Extensions\PropertyTypeExtension; -use Dedoc\Scramble\Infer\Extensions\StaticMethodReturnTypeExtension; use Dedoc\Scramble\Infer\Scope\Scope; use Dedoc\Scramble\Infer\Services\ReferenceTypeResolver; use Dedoc\Scramble\Support\Helpers\JsonResourceHelper; @@ -29,7 +27,7 @@ use Illuminate\Http\Resources\MergeValue; use Illuminate\Http\Resources\MissingValue; -class JsonResourceExtension implements MethodReturnTypeExtension, PropertyTypeExtension, StaticMethodReturnTypeExtension +class JsonResourceExtension implements MethodReturnTypeExtension, PropertyTypeExtension { public function shouldHandle(ObjectType|string $type): bool { @@ -43,10 +41,10 @@ public function shouldHandle(ObjectType|string $type): bool public function getMethodReturnType(MethodCallEvent $event): ?Type { return match ($event->name) { - // @todo This should work automatically as toArray calls must be proxied to parents. - 'toArray' => ($event->getInstance()->name === JsonResource::class || ($event->getDefinition() && ! $event->getDefinition()->hasMethodDefinition('toArray'))) + 'toArray' => $event->methodDefiningClassName === JsonResource::class ? $this->getModelMethodReturn($event->getInstance()->name, 'toArray', $event->arguments, $event->scope) : null, + 'response', 'toResponse' => new Generic(JsonResponse::class, [$event->getInstance(), new LiteralIntegerType(200), new ArrayType]), 'whenLoaded' => count($event->arguments) === 1 @@ -88,14 +86,6 @@ public function getMethodReturnType(MethodCallEvent $event): ?Type }; } - public function getStaticMethodReturnType(StaticMethodCallEvent $event): ?Type - { - return match ($event->name) { - 'toArray' => $this->handleToArrayStaticCall($event), - default => null, - }; - } - public function getPropertyType(PropertyFetchEvent $event): ?Type { return match ($event->name) { @@ -112,22 +102,6 @@ public function getPropertyType(PropertyFetchEvent $event): ?Type }; } - /** - * Note: In fact, this is not a static call to the JsonResource. This is how type inference system treats it for - * now, when analyzing parent::toArray() call. `parent::` becomes `JsonResource::`. So this should be fixed in - * future just for the sake of following how real code works. - */ - private function handleToArrayStaticCall(StaticMethodCallEvent $event): ?Type - { - $contextClassName = $event->scope->context->classDefinition->name ?? null; - - if (! $contextClassName) { - return null; - } - - return $this->getModelMethodReturn($contextClassName, 'toArray', $event->arguments, $event->scope); - } - private function proxyMethodCallToModel(MethodCallEvent $event) { return $this->getModelMethodReturn($event->getInstance()->name, $event->name, $event->arguments, $event->scope); diff --git a/src/Support/InferExtensions/ModelExtension.php b/src/Support/InferExtensions/ModelExtension.php index 941b4591..0c5e616e 100644 --- a/src/Support/InferExtensions/ModelExtension.php +++ b/src/Support/InferExtensions/ModelExtension.php @@ -4,7 +4,6 @@ use Carbon\Carbon; use Carbon\CarbonImmutable; -use Dedoc\Scramble\Infer\Definition\ClassDefinition; use Dedoc\Scramble\Infer\Extensions\Event\MethodCallEvent; use Dedoc\Scramble\Infer\Extensions\Event\PropertyFetchEvent; use Dedoc\Scramble\Infer\Extensions\MethodReturnTypeExtension; @@ -154,10 +153,7 @@ public function getMethodReturnType(MethodCallEvent $event): ?Type return null; } - /** @var ClassDefinition $definition */ - $definition = $event->getDefinition(); - - if (array_key_exists('toArray', $definition?->methods ?: [])) { + if ($event->methodDefiningClassName !== Model::class) { return null; } diff --git a/src/Support/Type/ObjectType.php b/src/Support/Type/ObjectType.php index 520e523d..87fedb5c 100644 --- a/src/Support/Type/ObjectType.php +++ b/src/Support/Type/ObjectType.php @@ -53,11 +53,14 @@ public function getMethodDefinition(string $methodName, Scope $scope = new Globa public function getMethodReturnType(string $methodName, array $arguments = [], Scope $scope = new GlobalScope): ?Type { + $classDefinition = $scope->index->getClassDefinition($this->name); + if ($returnType = app(ExtensionsBroker::class)->getMethodReturnType(new MethodCallEvent( instance: $this, name: $methodName, scope: $scope, arguments: $arguments, + methodDefiningClassName: $classDefinition ? $classDefinition->getMethodDefiningClassName($methodName, $scope->index) : $this->name, ))) { return $returnType; } diff --git a/tests/InferExtensions/ModelExtensionTest.php b/tests/InferExtensions/ModelExtensionTest.php index 71a48767..58e191fc 100644 --- a/tests/InferExtensions/ModelExtensionTest.php +++ b/tests/InferExtensions/ModelExtensionTest.php @@ -49,8 +49,11 @@ it('adds toArray method type the model class without defined toArray class', function () { $this->infer->analyzeClass(SampleUserModel::class); + $scope = new Infer\Scope\GlobalScope; + $scope->index = $this->infer->index; + $toArrayReturnType = (new ObjectType(SampleUserModel::class)) - ->getMethodReturnType('toArray'); + ->getMethodReturnType('toArray', scope: $scope); expect(collect($toArrayReturnType->items)->mapWithKeys(fn (ArrayItemType_ $t) => [$t->key.($t->isOptional ? '?' : '') => $t->value->toString()])) ->toMatchArray([ diff --git a/tests/Support/TypeToSchemaExtensions/JsonResourceTypeToSchemaTest.php b/tests/Support/TypeToSchemaExtensions/JsonResourceTypeToSchemaTest.php index a8692622..95dcf58b 100644 --- a/tests/Support/TypeToSchemaExtensions/JsonResourceTypeToSchemaTest.php +++ b/tests/Support/TypeToSchemaExtensions/JsonResourceTypeToSchemaTest.php @@ -37,6 +37,16 @@ expect($extension->toSchema($type)->toArray())->toBe($expectedSchemaArray); })->with([ + [JsonResourceTypeToSchemaTest_NestedSample::class, [ + 'type' => 'object', + 'properties' => [ + 'id' => ['type' => 'integer'], + 'name' => ['type' => 'string'], + 'foo' => ['type' => 'string', 'example' => 'bar'], + 'nested' => ['type' => 'string', 'example' => 'true'], + ], + 'required' => ['id', 'name', 'foo', 'nested'], + ]], [JsonResourceTypeToSchemaTest_Sample::class, [ 'type' => 'object', 'properties' => [ @@ -72,7 +82,7 @@ class JsonResourceTypeToSchemaTest_NoToArraySample extends \Illuminate\Http\Reso */ class JsonResourceTypeToSchemaTest_SpreadSample extends \Illuminate\Http\Resources\Json\JsonResource { - public function toArray($request) + public function toArray($request): array { return [ ...parent::toArray($request), @@ -80,6 +90,19 @@ public function toArray($request) ]; } } +/** + * @property JsonResourceTypeToSchemaTest_User $resource + */ +class JsonResourceTypeToSchemaTest_NestedSample extends JsonResourceTypeToSchemaTest_SpreadSample +{ + public function toArray($request): array + { + return [ + ...parent::toArray($request), + 'nested' => 'true', + ]; + } +} /** * @property JsonResourceTypeToSchemaTest_User $resource */