-
-
Notifications
You must be signed in to change notification settings - Fork 365
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
Changes from 6 commits
ba0013f
3fd2724
cc492c2
bde6747
4061246
0f9cec0
d3ab03f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 | ||
{ | ||
} |
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -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 | ||||
{ | ||||
|
@@ -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 | ||||
{ | ||||
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; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. just return true here, so it "marked" as has parent so There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 callers might handle the situation differently, depending on the method exists or not. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer to create separate service for param guard handling, eg: Something like rector-src/packages/VendorLocker/NodeVendorLocker/ClassMethodReturnTypeOverrideGuard.php Line 51 in 73112a2
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @staabm just checked the I will create new PR for that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. hm.., the service is named There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. handling There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I create PR
|
||||
} | ||||
} | ||||
|
||||
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) { | ||||
|
@@ -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) { | ||||
|
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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
} | ||
} |
There was a problem hiding this comment.
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 ornull
when classes involved are not known and we can't judge