Skip cond with side effect in RemoveDeadConditionAboveReturnRector (#…

…5424)

* Create skip_not_readonly_function_call_in_condition.php.inc #8381

Rector should not remove if() conditions if they contain a function call that is not proven read-only

* tidy up

* Skip cond with side effect

---------

Co-authored-by: noother <noothy@gmail.com>

rules-tests/DeadCode/Rector/Return_/RemoveDeadConditionAboveReturnRector/Fixture/skip_method_call_in_condition.php.inc

@@ -0,0 +1,15 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Return_\RemoveDeadConditionAboveReturnRector\Fixture;
+
+final class SkipMethodCallInCondition
+{
+    public function saveMyEntity($entity): bool
+    {
+        if ($entity->save()) {
+            return true;
+        }
+
+        return true;
+    }
+}

rules/DeadCode/Rector/Return_/RemoveDeadConditionAboveReturnRector.php

@@ -5,10 +5,14 @@
 namespace Rector\DeadCode\Rector\Return_;
 
 use PhpParser\Node;
+use PhpParser\Node\Stmt;
 use PhpParser\Node\Stmt\Else_;
 use PhpParser\Node\Stmt\If_;
 use PhpParser\Node\Stmt\Return_;
+use PHPStan\Analyser\Scope;
 use Rector\Contract\PhpParser\Node\StmtsAwareInterface;
+use Rector\DeadCode\SideEffect\SideEffectNodeDetector;
+use Rector\NodeTypeResolver\Node\AttributeKey;
 use Rector\Rector\AbstractRector;
 use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample;
 use Symplify\RuleDocGenerator\ValueObject\RuleDefinition;
@@ -18,6 +22,11 @@
  */
 final class RemoveDeadConditionAboveReturnRector extends AbstractRector
 {
+    public function __construct(
+        private readonly SideEffectNodeDetector $sideEffectNodeDetector,
+    ) {
+    }
+
     public function getRuleDefinition(): RuleDefinition
     {
         return new RuleDefinition('Remove dead condition above return', [
@@ -68,16 +77,16 @@ public function refactor(Node $node): ?StmtsAwareInterface
             }
 
             $previousNode = $node->stmts[$key - 1] ?? null;
-            if (! $previousNode instanceof If_) {
-                return null;
+            if (! $this->isBareIf($previousNode)) {
+                continue;
             }
 
-            if ($previousNode->elseifs !== []) {
-                return null;
-            }
+            /** @var Scope $scope */
+            $scope = $stmt->getAttribute(AttributeKey::SCOPE);
 
-            if ($previousNode->else instanceof Else_) {
-                return null;
+            /** @var If_ $previousNode */
+            if ($this->sideEffectNodeDetector->detect($previousNode->cond, $scope)) {
+                continue;
             }
 
             $countStmt = count($previousNode->stmts);
@@ -105,4 +114,17 @@ public function refactor(Node $node): ?StmtsAwareInterface
 
         return null;
     }
+
+    private function isBareIf(?Stmt $stmt): bool
+    {
+        if (! $stmt instanceof If_) {
+            return false;
+        }
+
+        if ($stmt->elseifs !== []) {
+            return false;
+        }
+
+        return ! $stmt->else instanceof Else_;
+    }
 }