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

Skip code changes on unresolvable/unknown classes #4619

Merged
merged 7 commits into from
Aug 7, 2023
Merged
Show file tree
Hide file tree
Changes from 6 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php

declare(strict_types=1);

namespace Rector\VendorLocker\Exception;

final class UnresolvableClassParentException extends \Exception
{
}
53 changes: 47 additions & 6 deletions packages/VendorLocker/ParentClassMethodTypeOverrideGuard.php
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,13 @@
use PHPStan\Reflection\ClassReflection;
use PHPStan\Reflection\MethodReflection;
use PHPStan\Type\Type;
use Rector\Core\Exception\ShouldNotHappenException;
use Rector\Core\Reflection\ReflectionResolver;
use Rector\Core\Util\Reflection\PrivatesAccessor;
use Rector\NodeNameResolver\NodeNameResolver;
use Rector\NodeTypeResolver\TypeComparator\TypeComparator;
use Rector\StaticTypeMapper\StaticTypeMapper;
use Rector\VendorLocker\Exception\UnresolvableClassParentException;

final class ParentClassMethodTypeOverrideGuard
{
Expand All @@ -20,30 +23,60 @@ public function __construct(
private readonly ReflectionResolver $reflectionResolver,
private readonly TypeComparator $typeComparator,
private readonly StaticTypeMapper $staticTypeMapper,
private readonly PrivatesAccessor $privatesAccessor,
) {
}

public function hasParentClassMethod(ClassMethod $classMethod): bool
public function hasParentClassMethod(ClassMethod $classMethod): ?bool
Copy link
Contributor Author

@staabm staabm Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this method now returns true/false when we can make a final decision or null when classes involved are not known and we can't judge

{
return $this->getParentClassMethod($classMethod) instanceof MethodReflection;
try {
$parentClassMethod = $this->resolveParentClassMethod($classMethod);

return $parentClassMethod instanceof MethodReflection;
} catch (UnresolvableClassParentException) {
// we don't know all involved parents.
return null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just return true here, so it "marked" as has parent so bool can be returned.

Copy link
Contributor Author

@staabm staabm Aug 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think its a good idea to handle "we don't know" the same as "sure, the method exists".

for our callers, as they exist right now, this is acceptable, but it might suprisingly break in the future as soon as someone compares the result of this method against true .

callers might handle the situation differently, depending on the method exists or not.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to create separate service for param guard handling, eg: ClassMethodParamTypeOverrideGuard.

Something like ClassMethodReturnTypeOverrideGuard::shouldSkipClassMethod() do

public function shouldSkipClassMethod(ClassMethod $classMethod, Scope $scope): bool

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@staabm just checked the getParentClassMethod() usages in rules, there are various usage directly, which cause exception, I think try catch should be in consumer instead of in hasParentClassMethod() as that cause double call.

I will create new PR for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm.., the service is named ParentClassMethodTypeOverrideGuard, which has vs get should only validate instead of getting in real use case, so I prefer return bool true for hasParentClassMethod(), and return null for getParentClassMethod() when no parent at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

handling UnresolvableClassException at the caller site could indeed improve the current situation 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I create PR

return bool only should be the way to go as it only validate, not a "real" verification, we then return ?MethodReflection for getParentClassMethod()

}
}

public function getParentClassMethod(ClassMethod $classMethod): ?MethodReflection
{
try {
$parentClassMethod = $this->resolveParentClassMethod($classMethod);

return $parentClassMethod;
} catch (UnresolvableClassParentException) {
// we don't know all involved parents.
throw new ShouldNotHappenException('Unable to resolve involved class. You are likely missing hasParentClassMethod() before calling getParentClassMethod().');
}
}

/**
* @throws UnresolvableClassParentException
*/
private function resolveParentClassMethod(ClassMethod $classMethod): ?MethodReflection
{
$classReflection = $this->reflectionResolver->resolveClassReflection($classMethod);
if (! $classReflection instanceof ClassReflection) {
return null;
// we can't resolve the class, so we don't know.
throw new UnresolvableClassParentException();
}

/** @var string $methodName */
$methodName = $this->nodeNameResolver->getName($classMethod);
$parentClassReflection = $classReflection->getParentClass();
while ($parentClassReflection instanceof ClassReflection) {
$currentClassReflection = $classReflection;
while ($this->hasClassParent($currentClassReflection)) {
$parentClassReflection = $currentClassReflection->getParentClass();
if (!$parentClassReflection instanceof ClassReflection) {
// per AST we have a parent class, but our reflection classes are not able to load its class definition/signature
throw new UnresolvableClassParentException();
}

if ($parentClassReflection->hasNativeMethod($methodName)) {
return $parentClassReflection->getNativeMethod($methodName);
}

$parentClassReflection = $parentClassReflection->getParentClass();
$currentClassReflection = $parentClassReflection;
}

foreach ($classReflection->getInterfaces() as $interfaceReflection) {
Expand All @@ -57,6 +90,14 @@ public function getParentClassMethod(ClassMethod $classMethod): ?MethodReflectio
return null;
}

private function hasClassParent(ClassReflection $classReflection):bool {
// XXX rework this hack, after /~https://github.com/phpstan/phpstan-src/pull/2563 landed
$nativeReflection = $classReflection->getNativeReflection();
$betterReflectionClass = $this->privatesAccessor->getPrivateProperty($nativeReflection, 'betterReflectionClass');
$parentClassName = $this->privatesAccessor->getPrivateProperty($betterReflectionClass, 'parentClassName');
return $parentClassName !== null;
}

public function shouldSkipReturnTypeChange(ClassMethod $classMethod, Type $parentType): bool
{
if ($classMethod->returnType === null) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
<?php

namespace Rector\Tests\TypeDeclaration\Rector\ClassMethod\ParamTypeByMethodCallTypeRector\Fixture;

use Rector\Tests\TypeDeclaration\Rector\ClassMethod\ParamTypeByMethodCallTypeRector\Source\GoOneWayInterface;
use Rector\Tests\TypeDeclaration\Rector\ClassMethod\ParamTypeByMethodCallTypeRector\Source\SomeTypedService;

final class SkipUnknownParent extends UnknownParentClass
{
public function __construct(
private SomeTypedService $someTypedService
) {
}

public function go($value)
{
$this->someTypedService->run($value);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
<?php

namespace Rector\Tests\TypeDeclaration\Rector\ClassMethod\ReturnTypeFromStrictParamRector\Fixture;

class SkipUnknownParent extends UnknownParentClass {
public function doFoo(SkipParentOverridden $param) {
return $param;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php

namespace Rector\Tests\TypeDeclaration\Rector\ClassMethod\StrictArrayParamDimFetchRector\Fixture;

use Rector\Tests\TypeDeclaration\Rector\ClassMethod\StrictArrayParamDimFetchRector\Source\SomeInterface;

class SkipUnknownParent extends UnknownParentClass
{
public function go($item)
Copy link
Contributor Author

@staabm staabm Jul 29, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we don't know the parent class (because e.g. its not autoloadable), we should be pessimistic and do no changes

{
return $item['name'];
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
<?php

namespace Rector\Tests\TypeDeclaration\Rector\Param\ParamTypeFromStrictTypedPropertyRector\Fixture;

final class SkipUnknownParentClass extends UnknownParentClass
{
private array $items = [];

public function redirect(string $path, $args = [])
{
$this->items = $args;
}
}
1 change: 1 addition & 0 deletions rules/Privatization/Guard/ParentPropertyLookupGuard.php
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ public function isLegal(Property $property, ?ClassReflection $classReflection):
return false;
}

// XXX rework this hack, after /~https://github.com/phpstan/phpstan-src/pull/2563 landed
$nativeReflection = $classReflection->getNativeReflection();
$betterReflectionClass = $this->privatesAccessor->getPrivateProperty(
$nativeReflection,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@ function (Node $node) use ($paramName): bool {
continue;
}

if ($this->parentClassMethodTypeOverrideGuard->hasParentClassMethod($node)) {
if ($this->parentClassMethodTypeOverrideGuard->hasParentClassMethod($node) !== false) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ private function shouldSkipClassMethod(ClassMethod $classMethod): bool
return true;
}

return $this->parentClassMethodTypeOverrideGuard->hasParentClassMethod($classMethod);
return $this->parentClassMethodTypeOverrideGuard->hasParentClassMethod($classMethod) !== false;
}

private function mirrorParamType(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ private function shouldSkipNode(ClassMethod|Function_|Closure $node): bool
}

if ($node instanceof ClassMethod) {
if ($this->parentClassMethodTypeOverrideGuard->hasParentClassMethod($node)) {
if ($this->parentClassMethodTypeOverrideGuard->hasParentClassMethod($node) !== false) {
return true;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ public function refactor(Node $node): ?Node
{
$hasChanged = false;

if ($node instanceof ClassMethod && $this->parentClassMethodTypeOverrideGuard->hasParentClassMethod($node)) {
if ($node instanceof ClassMethod && $this->parentClassMethodTypeOverrideGuard->hasParentClassMethod($node) !== false) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public function getNodeTypes(): array
*/
public function refactor(Node $node): ?Node
{
if ($node instanceof ClassMethod && $this->parentClassMethodTypeOverrideGuard->hasParentClassMethod($node)) {
if ($node instanceof ClassMethod && $this->parentClassMethodTypeOverrideGuard->hasParentClassMethod($node) !== false) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ private function decorateParamWithType(ClassMethod $classMethod, Param $param, S
return null;
}

if ($this->parentClassMethodTypeOverrideGuard->hasParentClassMethod($classMethod)) {
if ($this->parentClassMethodTypeOverrideGuard->hasParentClassMethod($classMethod) !== false) {
return null;
}

Expand Down
1 change: 1 addition & 0 deletions src/PhpParser/Node/Value/ValueResolver.php
Original file line number Diff line number Diff line change
Expand Up @@ -333,6 +333,7 @@ private function resolveClassFromSelfStaticParent(ClassConstFetch $classConstFet
);
}

// XXX rework this hack, after /~https://github.com/phpstan/phpstan-src/pull/2563 landed
// ensure parent class name still resolved even not autoloaded
$nativeReflection = $classReflection->getNativeReflection();
$betterReflectionClass = $this->privatesAccessor->getPrivateProperty(
Expand Down