Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Nov 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Infer/Analyzer/ClassAnalyzer.php
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ public function analyze(string $name): ClassDefinition
returnType: new UnknownType,
),
definingClassName: $name,
isStatic: $reflectionMethod->isStatic(),
);
}

Expand Down
30 changes: 30 additions & 0 deletions src/Infer/Definition/ClassDefinition.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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)) {
Expand Down
1 change: 1 addition & 0 deletions src/Infer/Definition/FunctionLikeDefinition.php
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
1 change: 1 addition & 0 deletions src/Infer/Extensions/Event/MethodCallEvent.php
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
1 change: 1 addition & 0 deletions src/Infer/Handler/FunctionLikeHandler.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down
2 changes: 1 addition & 1 deletion src/Infer/Scope/Scope.php
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
29 changes: 28 additions & 1 deletion src/Infer/Services/ReferenceTypeResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -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,
);
}

Expand Down Expand Up @@ -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.

Expand All @@ -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,
Expand All @@ -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;
}
Expand Down Expand Up @@ -747,6 +773,7 @@ private function getMethodCallsSideEffectIntroducedTypesInConstructor(Generic $t
name: $se->methodName,
scope: $scope,
arguments: $se->arguments,
methodDefiningClassName: $type->name,
));
}

Expand Down
32 changes: 3 additions & 29 deletions src/Support/InferExtensions/JsonResourceExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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
{
Expand All @@ -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
Expand Down Expand Up @@ -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) {
Expand All @@ -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);
Expand Down
6 changes: 1 addition & 5 deletions src/Support/InferExtensions/ModelExtension.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
}

Expand Down
3 changes: 3 additions & 0 deletions src/Support/Type/ObjectType.php
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
5 changes: 4 additions & 1 deletion tests/InferExtensions/ModelExtensionTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -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([
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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' => [
Expand Down Expand Up @@ -72,14 +82,27 @@ 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),
'foo' => 'bar',
];
}
}
/**
* @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
*/
Expand Down