[DeadCode] Allow not has side effect duplicate assign on RemoveDouble…

…AssignRector (#679)

* [DeadCode] Allow not has side effect duplicat assign on RemoveDoubleAssignRector

* skip different args value

* do not remove comment first not has side effect func call

* add stream_filter_append to impure function

* clean up

* clean up

* skip stream_filter

* phpstan

* phpstan

* clean up

* found a bug different scope

* fixture

* debug

* Fixed 🎉

rules-tests/DeadCode/Rector/Assign/RemoveDoubleAssignRector/Fixture/do_not_remove_comment_if_assignment_not_removed.php.inc

@@ -0,0 +1,28 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Assign\RemoveDoubleAssignRector\Fixture;
+
+class DoNotRemoveCommentIfAssignmentNotRemoved
+{
+    protected function run(array $data)
+    {
+        foreach ($data as $key => $val) {
+
+            if (rand(0, 1)) {
+                // a comment
+                $val = str_replace('a', 'b', $val);
+                $val = str_replace('b', 'c', $val);
+            }
+
+            if (rand(0, 1)) {
+                if (rand(0, 1)) {
+                    // a comment
+                    $val = str_replace('a', 'b', $val);
+                    $val = str_replace('b', 'c', $val);
+                }
+            }
+        }
+
+        return false;
+    }
+}

rules-tests/DeadCode/Rector/Assign/RemoveDoubleAssignRector/Fixture/not_has_side_effect_func_call.php.inc

@@ -0,0 +1,16 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Assign\RemoveDoubleAssignRector\Fixture;
+
+$value = strlen('test');
+$value = strlen('test');
+
+?>
+-----
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Assign\RemoveDoubleAssignRector\Fixture;
+
+$value = strlen('test');
+
+?>

rules-tests/DeadCode/Rector/Assign/RemoveDoubleAssignRector/Fixture/skip_stream_filter_append.php.inc

@@ -0,0 +1,4 @@
+<?php
+
+$streamFilter = stream_filter_append(STDOUT, 'AFilter');
+$streamFilter = stream_filter_append(STDERR, 'AFilter');

rules/DeadCode/Rector/Assign/RemoveDoubleAssignRector.php

@@ -5,16 +5,13 @@
 namespace Rector\DeadCode\Rector\Assign;
 
 use PhpParser\Node;
-use PhpParser\Node\Expr;
 use PhpParser\Node\Expr\Assign;
-use PhpParser\Node\Expr\FuncCall;
-use PhpParser\Node\Expr\MethodCall;
 use PhpParser\Node\Expr\PropertyFetch;
-use PhpParser\Node\Expr\StaticCall;
 use PhpParser\Node\Expr\StaticPropertyFetch;
 use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\Stmt\Expression;
 use Rector\Core\Rector\AbstractRector;
+use Rector\DeadCode\SideEffect\SideEffectNodeDetector;
 use Rector\NodeNestingScope\ScopeNestingComparator;
 use Rector\NodeTypeResolver\Node\AttributeKey;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
@@ -26,7 +23,8 @@
 final class RemoveDoubleAssignRector extends AbstractRector
 {
     public function __construct(
-        private ScopeNestingComparator $scopeNestingComparator
+        private ScopeNestingComparator $scopeNestingComparator,
+        private SideEffectNodeDetector $sideEffectNodeDetector
     ) {
     }
 
@@ -74,15 +72,18 @@ public function refactor(Node $node): ?Node
             return null;
         }
 
-        if ($this->isCall($previousStatement->expr->expr)) {
+        // early check self referencing, ensure that variable not re-used
+        if ($this->isSelfReferencing($node)) {
             return null;
         }
 
-        if ($this->shouldSkipForDifferentScope($node, $previousStatement)) {
+        // detect call expression has side effect
+        if ($this->sideEffectNodeDetector->detectCallExpr($previousStatement->expr->expr)) {
             return null;
         }
 
-        if ($this->isSelfReferencing($node)) {
+        // check scoping variable
+        if ($this->shouldSkipForDifferentScope($node, $previousStatement)) {
             return null;
         }
 
@@ -92,11 +93,6 @@ public function refactor(Node $node): ?Node
         return $node;
     }
 
-    private function isCall(Expr $expr): bool
-    {
-        return $expr instanceof FuncCall || $expr instanceof StaticCall || $expr instanceof MethodCall;
-    }
-
     private function shouldSkipForDifferentScope(Assign $assign, Expression $expression): bool
     {
         if (! $this->areInSameClassMethod($assign, $expression)) {

rules/DeadCode/SideEffect/PureFunctionDetector.php

@@ -110,6 +110,12 @@ final class PureFunctionDetector
 
         // json
         'json_encode', 'json_decode', 'json_last_error',
+
+        // array
+        'array_pop', 'array_push', 'array_shift', 'next', 'prev',
+
+        // stream
+        'stream_filter_append',
     ];
 
     public function __construct(

rules/DeadCode/SideEffect/SideEffectNodeDetector.php

@@ -4,6 +4,7 @@
 
 namespace Rector\DeadCode\SideEffect;
 
+use Nette\Utils\Strings;
 use PhpParser\Node;
 use PhpParser\Node\Expr;
 use PhpParser\Node\Expr\ArrayDimFetch;
@@ -17,6 +18,7 @@
 use PhpParser\Node\Expr\StaticCall;
 use PhpParser\Node\Expr\Variable;
 use PhpParser\Node\Name;
+use PhpParser\Node\Name\FullyQualified;
 use PhpParser\Node\Scalar\Encapsed;
 use PHPStan\Type\ConstantType;
 use PHPStan\Type\ObjectType;
@@ -85,6 +87,10 @@ public function detectCallExpr(Node $node): bool
             return false;
         }
 
+        if ($node instanceof New_ && $this->isPhpParser($node)) {
+            return false;
+        }
+
         $exprClass = $node::class;
         if (in_array($exprClass, self::CALL_EXPR_SIDE_EFFECT_NODE_TYPES, true)) {
             return true;
@@ -97,6 +103,18 @@ public function detectCallExpr(Node $node): bool
         return false;
     }
 
+    private function isPhpParser(New_ $new): bool
+    {
+        if (! $new->class instanceof FullyQualified) {
+            return false;
+        }
+
+        $className = $new->class->toString();
+        $namespace = Strings::before($className, '\\', 1);
+
+        return  $namespace === 'PhpParser';
+    }
+
     private function isClassCallerThrowable(StaticCall $staticCall): bool
     {
         $class = $staticCall->class;