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

…non __construct method (#4614)

* Create skip_call_by_ref_method_scope.php.inc

This PR is a follow-up to #4593. It adds a failing test case to showcase the expected behavior when passing non-readonly properties by reference in a method other than the constructor.

See #4593 (comment) for more details.

* Remove duplicated namespace

* rename fixture

* fix #4613

* [ci-review] Rector Rectify

* [ci-review] Rector Rectify

* fix

* [ci-review] Rector Rectify

* [ci-review] Rector Rectify

* remove unused pass Scope

* [ci-review] Rector Rectify

* add scope

* add scope

* [ci-review] Rector Rectify

* [ci-review] Rector Rectify

---------

Co-authored-by: Elias Häußler <elias@haeussler.dev>
Co-authored-by: GitHub Action <actions@github.com>

packages/NodeTypeResolver/Node/AttributeKey.php

@@ -316,4 +316,9 @@ final class AttributeKey
      * @var string
      */
     public const INSIDE_ARRAY_DIM_FETCH = 'inside_array_dim_fetch';
+
+    /**
+     * @var string
+     */
+    public const IS_USED_AS_ARG_BY_REF_VALUE = 'is_used_as_arg_by_ref_value';
 }

rules-tests/Php81/Rector/Property/ReadOnlyPropertyRector/Fixture/skip_call_by_ref_method_scope.php.inc

@@ -0,0 +1,21 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Tests\Php81\Rector\Property\ReadOnlyPropertyRector\Fixture;
+
+use Rector\Tests\Php81\Rector\Property\ReadOnlyPropertyRector\Source\WithByRefMethod;
+
+final class SkipCallByRefMethodScope
+{
+    public function __construct(
+        private object $output
+    ) {
+    }
+
+    public function prepare(): void
+    {
+        $obj = new WithByRefMethod();
+        $obj->install($this->output);
+    }
+}

rules-tests/Php81/Rector/Property/ReadOnlyPropertyRector/Source/WithByRefMethod.php

@@ -0,0 +1,12 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Tests\Php81\Rector\Property\ReadOnlyPropertyRector\Source;
+
+final class WithByRefMethod
+{
+    public function install(&$output)
+    {
+    }
+}

rules/TypeDeclaration/Rector/ClassMethod/AddMethodCallBasedStrictParamTypeRector.php

@@ -4,12 +4,12 @@
 
 namespace Rector\TypeDeclaration\Rector\ClassMethod;
 
+use PhpParser\Node\Name;
 use PhpParser\Node;
 use PhpParser\Node\Stmt\Class_;
 use PhpParser\Node\Stmt\ClassMethod;
 use Rector\Core\PhpParser\NodeFinder\LocalMethodCallFinder;
 use Rector\Core\Rector\AbstractRector;
-use Rector\Core\Reflection\ReflectionResolver;
 use Rector\TypeDeclaration\NodeAnalyzer\CallTypesResolver;
 use Rector\TypeDeclaration\NodeAnalyzer\ClassMethodParamTypeCompleter;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
@@ -91,11 +91,11 @@ public function refactor(Node $node): ?Node
 
             $isPrivate =
                 ($node->isFinal()
-                && $node->extends === null
+                && !$node->extends instanceof Name
                 && $node->implements === []
                 && $method->isProtected())
                 || ($method->isFinal()
-                    && $node->extends === null
+                    && !$node->extends instanceof Name
                     && $node->implements === []
                 )
                 || $method->isPrivate();

src/NodeManipulator/PropertyManipulator.php

@@ -4,18 +4,16 @@
 
 namespace Rector\Core\NodeManipulator;
 
+use PHPStan\Analyser\Scope;
 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;
@@ -24,12 +22,10 @@
 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;
@@ -60,8 +56,7 @@ public function __construct(
         private readonly PromotedPropertyResolver $promotedPropertyResolver,
         private readonly ConstructorAssignDetector $constructorAssignDetector,
         private readonly ClassLikeAstResolver $classLikeAstResolver,
-        private readonly PropertyFetchAnalyzer $propertyFetchAnalyzer,
-        private readonly ReflectionResolver $reflectionResolver
+        private readonly PropertyFetchAnalyzer $propertyFetchAnalyzer
     ) {
     }
 
@@ -76,11 +71,11 @@ public function isPropertyChangeableExceptConstructor(
             return true;
         }
 
-        $propertyFetches = $this->propertyFetchFinder->findPrivatePropertyFetches($class, $propertyOrParam);
+        $propertyFetches = $this->propertyFetchFinder->findPrivatePropertyFetches($class, $propertyOrParam, $scope);
         $classMethod = $class->getMethod(MethodName::CONSTRUCT);
 
         foreach ($propertyFetches as $propertyFetch) {
-            if ($this->isChangeableContext($propertyFetch, $scope, $classMethod)) {
+            if ($this->isChangeableContext($propertyFetch)) {
                 return true;
             }
 
@@ -169,77 +164,19 @@ private function isPropertyAssignedOnlyInConstructor(
         return $this->constructorAssignDetector->isPropertyAssigned($class, $propertyName);
     }
 
-    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 {
+    private function isChangeableContext(PropertyFetch | StaticPropertyFetch $propertyFetch): bool
+    {
         if ($propertyFetch->getAttribute(AttributeKey::IS_UNSET_VAR, false)) {
             return true;
         }
 
-        if ($propertyFetch->getAttribute(AttributeKey::IS_ARG_VALUE)) {
-            $caller = $this->resolveCaller($propertyFetch, $classMethod);
-            return $this->isFoundByRefParam($caller, $scope);
+        if ($propertyFetch->getAttribute(AttributeKey::INSIDE_ARRAY_DIM_FETCH, false)) {
+            return true;
         }
 
-        return $propertyFetch->getAttribute(AttributeKey::INSIDE_ARRAY_DIM_FETCH, false);
+        return $propertyFetch->getAttribute(AttributeKey::IS_USED_AS_ARG_BY_REF_VALUE, false) === true;
     }
 
-    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
     {

src/PhpParser/NodeFinder/PropertyFetchFinder.php

@@ -8,21 +8,26 @@
 use PhpParser\Node\Expr;
 use PhpParser\Node\Expr\ArrayDimFetch;
 use PhpParser\Node\Expr\Assign;
+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;
 use PhpParser\Node\Stmt\Class_;
 use PhpParser\Node\Stmt\Property;
 use PhpParser\Node\Stmt\Trait_;
+use PHPStan\Analyser\Scope;
 use PHPStan\Reflection\ClassReflection;
 use PHPStan\Type\TypeWithClassName;
 use Rector\Core\NodeAnalyzer\PropertyFetchAnalyzer;
 use Rector\Core\PhpParser\AstResolver;
 use Rector\Core\PhpParser\Node\BetterNodeFinder;
 use Rector\Core\Reflection\ReflectionResolver;
 use Rector\NodeNameResolver\NodeNameResolver;
+use Rector\NodeTypeResolver\Node\AttributeKey;
 use Rector\NodeTypeResolver\NodeTypeResolver;
+use Rector\NodeTypeResolver\PHPStan\ParametersAcceptorSelectorVariantsWrapper;
 use Rector\PhpDocParser\NodeTraverser\SimpleCallableNodeTraverser;
 use Rector\StaticTypeMapper\ValueObject\Type\FullyQualifiedObjectType;
 
@@ -42,7 +47,7 @@ public function __construct(
     /**
      * @return array<PropertyFetch|StaticPropertyFetch>
      */
-    public function findPrivatePropertyFetches(Class_ $class, Property | Param $propertyOrPromotedParam): array
+    public function findPrivatePropertyFetches(Class_ $class, Property | Param $propertyOrPromotedParam, Scope $scope): array
     {
         $propertyName = $this->resolvePropertyName($propertyOrPromotedParam);
         if ($propertyName === null) {
@@ -56,7 +61,7 @@ public function findPrivatePropertyFetches(Class_ $class, Property | Param $prop
         $hasTrait = $nodesTrait !== [];
         $nodes = array_merge($nodes, $nodesTrait);
 
-        return $this->findPropertyFetchesInClassLike($class, $nodes, $propertyName, $hasTrait);
+        return $this->findPropertyFetchesInClassLike($class, $nodes, $propertyName, $hasTrait, $scope);
     }
 
     /**
@@ -150,12 +155,18 @@ private function findPropertyFetchesInClassLike(
         Class_|Trait_ $class,
         array $stmts,
         string $propertyName,
-        bool $hasTrait
+        bool $hasTrait,
+        Scope $scope
     ): array {
         /** @var PropertyFetch[]|StaticPropertyFetch[] $propertyFetches */
         $propertyFetches = $this->betterNodeFinder->find(
             $stmts,
-            function (Node $subNode) use ($class, $hasTrait, $propertyName): bool {
+            function (Node $subNode) use ($class, $hasTrait, $propertyName, $scope): bool {
+                if ($subNode instanceof MethodCall || $subNode instanceof StaticCall) {
+                    $this->decoratePropertyFetch($subNode, $scope);
+                    return false;
+                }
+
                 if ($subNode instanceof PropertyFetch) {
                     if ($this->isInAnonymous($subNode, $class, $hasTrait)) {
                         return false;
@@ -175,6 +186,50 @@ function (Node $subNode) use ($class, $hasTrait, $propertyName): bool {
         return $propertyFetches;
     }
 
+    private function decoratePropertyFetch(Node $node, Scope $scope): void
+    {
+        if (! $node instanceof MethodCall && ! $node instanceof StaticCall) {
+            return;
+        }
+
+        if ($node->isFirstClassCallable()) {
+            return;
+        }
+
+        foreach ($node->getArgs() as $key => $arg) {
+            if (!$arg->value instanceof PropertyFetch && !$arg->value instanceof StaticPropertyFetch) {
+                continue;
+            }
+
+            if (!$this->isFoundByRefParam($node, $key, $scope)) {
+                continue;
+            }
+
+            $arg->value->setAttribute(AttributeKey::IS_USED_AS_ARG_BY_REF_VALUE, true);
+        }
+    }
+
+    private function isFoundByRefParam(MethodCall | StaticCall $node, int $key, Scope $scope): bool
+    {
+        $functionLikeReflection = $this->reflectionResolver->resolveFunctionLikeReflectionFromCall($node);
+        if ($functionLikeReflection === null) {
+            return false;
+        }
+
+        $parametersAcceptor = ParametersAcceptorSelectorVariantsWrapper::select(
+            $functionLikeReflection,
+            $node,
+            $scope
+        );
+
+        $parameters = $parametersAcceptor->getParameters();
+        if (! isset($parameters[$key])) {
+            return false;
+        }
+
+        return $parameters[$key]->passedByReference()->yes();
+    }
+
     private function isInAnonymous(PropertyFetch $propertyFetch, Class_|Trait_ $class, bool $hasTrait): bool
     {
         $classReflection = $this->reflectionResolver->resolveClassReflection($propertyFetch);