Skip to content

Commit

Permalink
[Php81] Fix regression skip call by ref on ReadOnlyPropertyRector (#4593
Browse files Browse the repository at this point in the history 
)

* [Php81] Fix regression skip call by ref on ReadOnlyPropertyRector

* Fixed 🎉

* [ci-review] Rector Rectify

* [ci-review] Rector Rectify

* Fixed 🎉

* scoped check

* scoped check

* remove comment

---------

Co-authored-by: GitHub Action <actions@github.com>
  • Loading branch information
@samsonasik @actions-user
samsonasik and actions-user authored Jul 25, 2023
1 parent 665bc65 commit 031f2de
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 14 deletions.
16 changes: 16 additions & 0 deletions rules-tests/Php81/Rector/Property/ReadOnlyPropertyRector/Fixture/skip_call_by_ref.php.inc
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<?php

namespace Rector\Tests\Php81\Rector\Property\ReadOnlyPropertyRector\Fixture;

final class SkipCallByRef
{
public function __construct(
private array $someArray,
) {
$this->doSomething($this->someArray);
}

public function doSomething(array &$someArray = null): void
{
}
}
12 changes: 6 additions & 6 deletions rules/Php81/Rector/Property/ReadOnlyPropertyRector.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,7 +109,7 @@ public function refactorWithScope(Node $node, Scope $scope): ?Node

foreach ($node->getMethods() as $classMethod) {
foreach ($classMethod->params as $param) {
$justChanged = $this->refactorParam($node, $classMethod, $param);
$justChanged = $this->refactorParam($node, $classMethod, $param, $scope);
// different variable to ensure $hasRemoved not replaced
if ($justChanged instanceof Param) {
$hasChanged = true;
Expand All @@ -118,7 +118,7 @@ public function refactorWithScope(Node $node, Scope $scope): ?Node
}

foreach ($node->getProperties() as $property) {
$changedProperty = $this->refactorProperty($node, $property);
$changedProperty = $this->refactorProperty($node, $property, $scope);
if ($changedProperty instanceof Property) {
$hasChanged = true;
}
Expand All @@ -136,7 +136,7 @@ public function provideMinPhpVersion(): int
return PhpVersionFeature::READONLY_PROPERTY;
}

private function refactorProperty(Class_ $class, Property $property): ?Property
private function refactorProperty(Class_ $class, Property $property, Scope $scope): ?Property
{
// 1. is property read-only?
if ($property->isReadonly()) {
Expand All @@ -159,7 +159,7 @@ private function refactorProperty(Class_ $class, Property $property): ?Property
return null;
}

if ($this->propertyManipulator->isPropertyChangeableExceptConstructor($class, $property)) {
if ($this->propertyManipulator->isPropertyChangeableExceptConstructor($class, $property, $scope)) {
return null;
}

Expand All @@ -177,7 +177,7 @@ private function refactorProperty(Class_ $class, Property $property): ?Property
return $property;
}

private function refactorParam(Class_ $class, ClassMethod $classMethod, Param $param): Param | null
private function refactorParam(Class_ $class, ClassMethod $classMethod, Param $param, Scope $scope): Param | null
{
if (! $this->visibilityManipulator->hasVisibility($param, Visibility::PRIVATE)) {
return null;
Expand All @@ -188,7 +188,7 @@ private function refactorParam(Class_ $class, ClassMethod $classMethod, Param $p
}

// promoted property?
if ($this->propertyManipulator->isPropertyChangeableExceptConstructor($class, $param)) {
if ($this->propertyManipulator->isPropertyChangeableExceptConstructor($class, $param, $scope)) {
return null;
}

Expand Down
83 changes: 75 additions & 8 deletions src/NodeManipulator/PropertyManipulator.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,16 @@

use Doctrine\ORM\Mapping\Table;
use PhpParser\Node;
use PhpParser\Node\Expr\MethodCall;
use PhpParser\Node\Expr\PropertyFetch;
use PhpParser\Node\Expr\StaticCall;
use PhpParser\Node\Expr\StaticPropertyFetch;
use PhpParser\Node\Param;
use PhpParser\Node\Stmt\Class_;
use PhpParser\Node\Stmt\ClassMethod;
use PhpParser\Node\Stmt\Property;
use PhpParser\Node\Stmt\Trait_;
use PHPStan\Analyser\Scope;
use PHPStan\Reflection\ClassReflection;
use PHPStan\Type\ObjectType;
use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfo;
Expand All @@ -21,10 +24,12 @@
use Rector\Core\PhpParser\ClassLikeAstResolver;
use Rector\Core\PhpParser\Node\BetterNodeFinder;
use Rector\Core\PhpParser\NodeFinder\PropertyFetchFinder;
use Rector\Core\Reflection\ReflectionResolver;
use Rector\Core\ValueObject\MethodName;
use Rector\NodeNameResolver\NodeNameResolver;
use Rector\NodeTypeResolver\Node\AttributeKey;
use Rector\NodeTypeResolver\NodeTypeResolver;
use Rector\NodeTypeResolver\PHPStan\ParametersAcceptorSelectorVariantsWrapper;
use Rector\Php80\NodeAnalyzer\PhpAttributeAnalyzer;
use Rector\Php80\NodeAnalyzer\PromotedPropertyResolver;
use Rector\TypeDeclaration\AlreadyAssignDetector\ConstructorAssignDetector;
Expand Down Expand Up @@ -55,12 +60,16 @@ public function __construct(
private readonly PromotedPropertyResolver $promotedPropertyResolver,
private readonly ConstructorAssignDetector $constructorAssignDetector,
private readonly ClassLikeAstResolver $classLikeAstResolver,
private readonly PropertyFetchAnalyzer $propertyFetchAnalyzer
private readonly PropertyFetchAnalyzer $propertyFetchAnalyzer,
private readonly ReflectionResolver $reflectionResolver
) {
}

public function isPropertyChangeableExceptConstructor(Class_ $class, Property | Param $propertyOrParam): bool
{
public function isPropertyChangeableExceptConstructor(
Class_ $class,
Property | Param $propertyOrParam,
Scope $scope
): bool {
$phpDocInfo = $this->phpDocInfoFactory->createFromNodeOrEmpty($class);

if ($this->hasAllowedNotReadonlyAnnotationOrAttribute($phpDocInfo, $class)) {
Expand All @@ -71,7 +80,7 @@ public function isPropertyChangeableExceptConstructor(Class_ $class, Property |
$classMethod = $class->getMethod(MethodName::CONSTRUCT);

foreach ($propertyFetches as $propertyFetch) {
if ($this->isChangeableContext($propertyFetch)) {
if ($this->isChangeableContext($propertyFetch, $scope, $classMethod)) {
return true;
}

Expand Down Expand Up @@ -160,20 +169,78 @@ private function isPropertyAssignedOnlyInConstructor(
return $this->constructorAssignDetector->isPropertyAssigned($class, $propertyName);
}

private function isChangeableContext(PropertyFetch | StaticPropertyFetch $propertyFetch): bool
{
private function resolveCaller(
PropertyFetch|StaticPropertyFetch $propertyFetch,
?ClassMethod $classMethod
): MethodCall | StaticCall | null {
if (! $classMethod instanceof ClassMethod) {
return null;
}

return $this->betterNodeFinder->findFirstInFunctionLikeScoped(
$classMethod,
static function (Node $subNode) use ($propertyFetch): bool {
if (! $subNode instanceof MethodCall && ! $subNode instanceof StaticCall) {
return false;
}

if ($subNode->isFirstClassCallable()) {
return false;
}

foreach ($subNode->getArgs() as $arg) {
if ($arg->value === $propertyFetch) {
return true;
}
}

return false;
}
);
}

private function isChangeableContext(
PropertyFetch | StaticPropertyFetch $propertyFetch,
Scope $scope,
?ClassMethod $classMethod
): bool {
if ($propertyFetch->getAttribute(AttributeKey::IS_UNSET_VAR, false)) {
return true;
}

// args most likely do not change properties
if ($propertyFetch->getAttribute(AttributeKey::IS_ARG_VALUE)) {
return false;
$caller = $this->resolveCaller($propertyFetch, $classMethod);
return $this->isFoundByRefParam($caller, $scope);
}

return $propertyFetch->getAttribute(AttributeKey::INSIDE_ARRAY_DIM_FETCH, false);
}

private function isFoundByRefParam(MethodCall | StaticCall | null $node, Scope $scope): bool
{
if ($node === null) {
return false;
}

$functionLikeReflection = $this->reflectionResolver->resolveFunctionLikeReflectionFromCall($node);
if ($functionLikeReflection === null) {
return false;
}

$parametersAcceptor = ParametersAcceptorSelectorVariantsWrapper::select(
$functionLikeReflection,
$node,
$scope
);
foreach ($parametersAcceptor->getParameters() as $parameterReflection) {
if ($parameterReflection->passedByReference()->yes()) {
return true;
}
}

return false;
}

private function hasAllowedNotReadonlyAnnotationOrAttribute(PhpDocInfo $phpDocInfo, Class_ $class): bool
{
if ($phpDocInfo->hasByAnnotationClasses(self::DOCTRINE_PROPERTY_ANNOTATIONS)) {
Expand Down

0 comments on commit 031f2de

Please sign in to comment.