prevent variable scope changing

ecs.yaml

@@ -75,6 +75,7 @@ parameters:
             - 'src/Rector/AbstractRector.php'
 
         Symplify\CodingStandard\Sniffs\CleanCode\CognitiveComplexitySniff:
+            - 'packages/MinimalScope/src/Rector/Class_/ChangeLocalPropertyToVariableRector.php'
             # solve later
             - 'src/Console/Command/ScreenFileCommand.php'
             - 'packages/Doctrine/src/Rector/ClassMethod/AddMethodCallBasedParamTypeRector.php'

packages/MinimalScope/src/Rector/Class_/ChangeLocalPropertyToVariableRector.php

@@ -5,10 +5,19 @@ declare(strict_types=1);
 namespace Rector\MinimalScope\Rector\Class_;
 
 use PhpParser\Node;
+use PhpParser\Node\Expr\Assign;
 use PhpParser\Node\Expr\PropertyFetch;
 use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\Stmt\Class_;
+use PhpParser\Node\Stmt\ClassMethod;
+use PhpParser\Node\Stmt\Do_;
+use PhpParser\Node\Stmt\Else_;
+use PhpParser\Node\Stmt\If_;
+use PhpParser\Node\Stmt\While_;
+use PhpParser\NodeTraverser;
+use Rector\NodeTypeResolver\Node\AttributeKey;
 use Rector\PhpParser\Node\Manipulator\ClassManipulator;
+use Rector\PhpParser\Node\Manipulator\PropertyFetchManipulator;
 use Rector\Rector\AbstractRector;
 use Rector\RectorDefinition\CodeSample;
 use Rector\RectorDefinition\RectorDefinition;
@@ -18,14 +27,25 @@ use Rector\RectorDefinition\RectorDefinition;
  */
 final class ChangeLocalPropertyToVariableRector extends AbstractRector
 {
+    /**
+     * @var string[]
+     */
+    private const SCOPE_CHANGING_NODE_TYPES = [Do_::class, While_::class, If_::class, Else_::class];
+
     /**
      * @var ClassManipulator
      */
     private $classManipulator;
 
-    public function __construct(ClassManipulator $classManipulator)
+    /**
+     * @var PropertyFetchManipulator
+     */
+    private $propertyFetchManipulator;
+
+    public function __construct(ClassManipulator $classManipulator, PropertyFetchManipulator $propertyFetchManipulator)
     {
         $this->classManipulator = $classManipulator;
+        $this->propertyFetchManipulator = $propertyFetchManipulator;
     }
 
     public function getDefinition(): RectorDefinition
@@ -136,6 +156,7 @@ PHP
     private function collectPropertyFetchByMethods(Class_ $class, array $privatePropertyNames): array
     {
         $propertyUsageByMethods = [];
+
         foreach ($privatePropertyNames as $privatePropertyName) {
             foreach ($class->getMethods() as $method) {
                 $hasProperty = (bool) $this->betterNodeFinder->findFirst($method, function (Node $node) use (
@@ -152,6 +173,13 @@ PHP
                     continue;
                 }
 
+                $isPropertyChangingInMultipleMethodCalls = $this->isPropertyChangingInMultipleMethodCalls($method,
+                    $privatePropertyName);
+
+                if ($isPropertyChangingInMultipleMethodCalls) {
+                    continue;
+                }
+
                 /** @var string $classMethodName */
                 $classMethodName = $this->getName($method);
                 $propertyUsageByMethods[$privatePropertyName][] = $classMethodName;
@@ -159,4 +187,98 @@ PHP
         }
         return $propertyUsageByMethods;
     }
+
+    /**
+     * Covers https://github.com/rectorphp/rector/pull/2558#discussion_r363036110
+     */
+    private function isPropertyChangingInMultipleMethodCalls(
+        ClassMethod $classMethod,
+        string $privatePropertyName
+    ): bool {
+        $isPropertyChanging = false;
+        $isPropertyReadInIf = false;
+        $isIfFollowedByAssign = false;
+
+        $this->traverseNodesWithCallable((array) $classMethod->getStmts(), function (Node $node) use (
+            &$isPropertyChanging,
+            $privatePropertyName,
+            &$isPropertyReadInIf,
+            &$isIfFollowedByAssign
+        ) {
+            if ($isPropertyReadInIf) {
+                if (! $this->propertyFetchManipulator->isLocalPropertyOfNames($node, [$privatePropertyName])) {
+                    return null;
+                }
+
+                $parentNode = $node->getAttribute(AttributeKey::PARENT_NODE);
+                if ($parentNode instanceof Assign) {
+                    if ($parentNode->var === $node) {
+                        $isIfFollowedByAssign = true;
+                    }
+                }
+            }
+
+            if (! $this->isScopeChangingNode($node)) {
+                return null;
+            }
+
+            if ($node instanceof If_) {
+                $this->traverseNodesWithCallable($node->cond, function (Node $node) use (
+                    $privatePropertyName,
+                    &$isPropertyReadInIf
+                ) {
+                    if (! $this->propertyFetchManipulator->isLocalPropertyOfNames($node, [$privatePropertyName])) {
+                        return null;
+                    }
+
+                    $isPropertyReadInIf = true;
+
+                    return NodeTraverser::STOP_TRAVERSAL;
+                });
+            }
+
+            // here cannot be any property assign
+            $this->traverseNodesWithCallable($node, function (Node $node) use (
+                &$isPropertyChanging,
+                $privatePropertyName
+            ) {
+                if (! $node instanceof Assign) {
+                    return null;
+                }
+
+                if (! $node->var instanceof PropertyFetch) {
+                    return null;
+                }
+
+                if (! $this->isName($node->var->name, $privatePropertyName)) {
+                    return null;
+                }
+
+                $isPropertyChanging = true;
+
+                return NodeTraverser::STOP_TRAVERSAL;
+            });
+
+            if ($isPropertyChanging === false) {
+                return null;
+            }
+
+            return NodeTraverser::STOP_TRAVERSAL;
+        });
+
+        return $isPropertyChanging || $isIfFollowedByAssign;
+    }
+
+    private function isScopeChangingNode(Node $node): bool
+    {
+        foreach (self::SCOPE_CHANGING_NODE_TYPES as $scopeChangingNode) {
+            if (! is_a($node, $scopeChangingNode, true)) {
+                continue;
+            }
+
+            return true;
+        }
+
+        return false;
+    }
 }

packages/MinimalScope/tests/Rector/Class_/ChangeLocalPropertyToVariableRector/Fixture/skip_double_use_in_one_method.php.inc

@@ -0,0 +1,22 @@
+<?php
+
+namespace Rector\MinimalScope\Tests\Rector\Class_\ChangeLocalPropertyToVariableRector\Fixture;
+
+class SkipDoubleUseInOneMethod
+{
+    private $count;
+
+    // first ->run() return 5
+    // second ->run() return 10
+    public function run()
+    {
+        if ($this->count === 5) {
+            $this->count = 10;
+            return $this->count;
+        }
+
+        $this->count = 5;
+
+        return $this->count;
+    }
+}

packages/MinimalScope/tests/Rector/Class_/ChangeLocalPropertyToVariableRector/Fixture/skip_double_use_in_one_method_without_change.php.inc

@@ -0,0 +1,21 @@
+<?php
+
+namespace Rector\MinimalScope\Tests\Rector\Class_\ChangeLocalPropertyToVariableRector\Fixture;
+
+class SkipDoubleUseInOneMethodWithoutChange
+{
+    private $count;
+
+    // first ->run() return 5
+    // second ->run() return 10
+    public function run()
+    {
+        if ($this->count === 5) {
+            return 10;
+        }
+
+        $this->count = 5;
+
+        return $this->count;
+    }
+}

phpstan.neon

@@ -248,3 +248,4 @@ parameters:
         - '#Call to an undefined method PhpParser\\Node\\Identifier\|PhpParser\\Node\\Name\|PhpParser\\Node\\NullableType\:\:toString\(\)#'
         - '#Parameter \#1 \$expected of method PHPUnit\\Framework\\Assert\:\:assertInstanceOf\(\) expects class\-string<object\>, string given#'
         - '#Unable to resolve the template type ExpectedType in call to method PHPUnit\\Framework\\Assert\:\:assertInstanceOf\(\)#'
+        - '#Right side of \|\| is always false#'

src/PhpParser/Node/Manipulator/IfManipulator.php

@@ -125,12 +125,12 @@ final class IfManipulator
 
     public function isEarlyElse(If_ $if): bool
     {
-        if (! $this->isAlwayAllowedType((array) $if->stmts, self::ALLOWED_BREAKING_NODE_TYPES)) {
+        if (! $this->isAlwaysAllowedType((array) $if->stmts, self::ALLOWED_BREAKING_NODE_TYPES)) {
             return false;
         }
 
         foreach ($if->elseifs as $elseif) {
-            if (! $this->isAlwayAllowedType((array) $elseif->stmts, self::ALLOWED_BREAKING_NODE_TYPES)) {
+            if (! $this->isAlwaysAllowedType((array) $elseif->stmts, self::ALLOWED_BREAKING_NODE_TYPES)) {
                 return false;
             }
         }
@@ -159,7 +159,7 @@ final class IfManipulator
      * @param Node[] $stmts
      * @param string[] $allowedTypes
      */
-    private function isAlwayAllowedType(array $stmts, array $allowedTypes): bool
+    private function isAlwaysAllowedType(array $stmts, array $allowedTypes): bool
     {
         $isAlwaysReturnValue = false;
 

src/PhpParser/Node/Manipulator/PropertyFetchManipulator.php

@@ -185,14 +185,14 @@ final class PropertyFetchManipulator
     /**
      * @param string[] $propertyNames
      */
-    public function isLocalPropertyOfNames(Expr $expr, array $propertyNames): bool
+    public function isLocalPropertyOfNames(Node $node, array $propertyNames): bool
     {
-        if (! $this->isLocalProperty($expr)) {
+        if (! $this->isLocalProperty($node)) {
             return false;
         }
 
-        /** @var PropertyFetch $expr */
-        return $this->nameResolver->isNames($expr->name, $propertyNames);
+        /** @var PropertyFetch $node */
+        return $this->nameResolver->isNames($node->name, $propertyNames);
     }
 
     public function isLocalProperty(Node $node): bool