Skip to content

Commit

Permalink
Improved inference of inherited method calls (fixes issues when `pare…
Browse files Browse the repository at this point in the history
…nt::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 <romalytvynenko@users.noreply.github.com>
  • Loading branch information
romalytvynenko and romalytvynenko authored Nov 24, 2024
1 parent a350179 commit 58ea1e7
Show file tree
Hide file tree
Showing 12 changed files with 98 additions and 38 deletions.
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

0 comments on commit 58ea1e7

Please sign in to comment.