[fixes] Couple ClassPropertyAssignToConstructorPromotionRector, RemoveAlwaysTrueIfConditionRector and RemoveUnusedVariableAssignRector fixes (#8055)

* [php 8.0] Skip merged property in PromotedPropertyCandidateResolver

* [dead-code] Fix /RemoveAlwaysTrueIfConditionRector with dynamic variable

Author: TomasVotruba

rules-tests/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector/Fixture/null_reset_of_plain_object.php.inc

@@ -0,0 +1,33 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Assign\RemoveUnusedVariableAssignRector\Fixture;
+
+use Rector\Tests\DeadCode\Rector\Assign\RemoveUnusedVariableAssignRector\Source\SomeLock;
+
+final class NullResetOfUserObject
+{
+    public function run(): void
+    {
+        $value = new SomeLock();
+        echo get_class($value);
+
+        $value = null;
+    }
+}
+?>
+-----
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Assign\RemoveUnusedVariableAssignRector\Fixture;
+
+use Rector\Tests\DeadCode\Rector\Assign\RemoveUnusedVariableAssignRector\Source\SomeLock;
+
+final class NullResetOfUserObject
+{
+    public function run(): void
+    {
+        $value = new SomeLock();
+        echo get_class($value);
+    }
+}
+?>

rules-tests/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector/Fixture/null_reset_of_user_object.php.inc

@@ -0,0 +1,14 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\Assign\RemoveUnusedVariableAssignRector\Fixture;
+
+final class SkipNullResetOfInternalObject
+{
+    public function run(string $path): void
+    {
+        $finfo = new \finfo(FILEINFO_MIME_TYPE);
+        echo $finfo->file($path);
+
+        $finfo = null;
+    }
+}

rules-tests/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector/Fixture/skip_null_reset_of_internal_object.php.inc

@@ -0,0 +1,23 @@
+<?php
+
+namespace Rector\Tests\DeadCode\Rector\If_\RemoveAlwaysTrueIfConditionRector\Fixture;
+
+class SkipDynamicVariableAssign
+{
+    public function build(array $params): string
+    {
+        $addHelpMessage = true;
+
+        foreach (['addHelpMessage'] as $f) {
+            if (isset($params[$f])) {
+                ${$f} = (bool) $params[$f];
+            }
+        }
+
+        if ($addHelpMessage) {
+            return 'help';
+        }
+
+        return 'no help';
+    }
+}

rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_dynamic_variable_assign.php.inc

@@ -0,0 +1,24 @@
+<?php
+
+namespace Rector\Tests\Php80\Rector\Class_\ClassPropertyAssignToConstructorPromotionRector\Fixture;
+
+use Rector\Tests\Php80\Rector\Class_\ClassPropertyAssignToConstructorPromotionRector\Source\SomeCacheAdapterInterface;
+use Rector\Tests\Php80\Rector\Class_\ClassPropertyAssignToConstructorPromotionRector\Source\SomeCacheProvider;
+
+final class SkipNarrowingVarDoc
+{
+    /**
+     * @var SomeCacheProvider
+     */
+    private SomeCacheAdapterInterface $cacheProvider;
+
+    public function __construct(SomeCacheAdapterInterface $cacheProvider)
+    {
+        $this->cacheProvider = $cacheProvider;
+    }
+
+    public function run(): object
+    {
+        return $this->cacheProvider->getCacheAdapter();
+    }
+}

rules-tests/Php80/Rector/Class_/ClassPropertyAssignToConstructorPromotionRector/Fixture/skip_narrowing_var_doc.php.inc

@@ -0,0 +1,10 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Tests\Php80\Rector\Class_\ClassPropertyAssignToConstructorPromotionRector\Source;
+
+interface SomeCacheAdapterInterface
+{
+    public function clear(): bool;
+}

rules-tests/Php80/Rector/Class_/ClassPropertyAssignToConstructorPromotionRector/Source/SomeCacheAdapterInterface.php

@@ -0,0 +1,18 @@
+<?php
+
+declare(strict_types=1);
+
+namespace Rector\Tests\Php80\Rector\Class_\ClassPropertyAssignToConstructorPromotionRector\Source;
+
+final class SomeCacheProvider implements SomeCacheAdapterInterface
+{
+    public function clear(): bool
+    {
+        return true;
+    }
+
+    public function getCacheAdapter(): object
+    {
+        return $this;
+    }
+}

rules-tests/Php80/Rector/Class_/ClassPropertyAssignToConstructorPromotionRector/Source/SomeCacheProvider.php

@@ -118,7 +118,7 @@ public function refactor(Node $node): null|ClassMethod|Function_
                 continue;
             }
 
-            if ($this->isNullResetOfObject($assign)) {
+            if ($this->isNullResetOfInternalObject($assign)) {
                 continue;
             }
 
@@ -142,16 +142,25 @@ public function refactor(Node $node): null|ClassMethod|Function_
         return null;
     }
 
-    private function isNullResetOfObject(Assign $assign): bool
+    private function isNullResetOfInternalObject(Assign $assign): bool
     {
-        // resetting a native filesystem object to null releases the held file handle,
-        // e.g. $file = null on a SplFileObject/SplFileInfo/RecursiveDirectoryIterator
+        // resetting an internal PHP object to null releases the held resource/file handle,
+        // e.g. $file = null on a SplFileObject/RecursiveDirectoryIterator/finfo
         if (! $this->valueResolver->isNull($assign->expr)) {
             return false;
         }
 
-        return (new ObjectType('SplFileInfo'))->isSuperTypeOf($this->getType($assign->var))
-            ->yes();
+        $varType = $this->getType($assign->var);
+        if (! $varType instanceof ObjectType) {
+            return false;
+        }
+
+        $classReflection = $varType->getClassReflection();
+        if (! $classReflection instanceof ClassReflection) {
+            return false;
+        }
+
+        return $classReflection->isBuiltin();
     }
 
     private function isObjectWithDestructMethod(Expr $expr): bool

rules/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector.php

@@ -150,11 +150,16 @@ private function shouldSkipFromVariable(Expr $expr): bool
         /** @var Variable[] $variables */
         $variables = $this->betterNodeFinder->findInstancesOf($expr, [Variable::class]);
 
+        $hasPlainVariable = false;
         foreach ($variables as $variable) {
             if ($this->exprAnalyzer->isNonTypedFromParam($variable)) {
                 return true;
             }
 
+            if (! $variable->name instanceof Expr) {
+                $hasPlainVariable = true;
+            }
+
             $type = $this->nodeTypeResolver->getNativeType($variable);
             if ($type instanceof IntersectionType) {
                 foreach ($type->getTypes() as $subType) {
@@ -165,7 +170,18 @@ private function shouldSkipFromVariable(Expr $expr): bool
             }
         }
 
-        return false;
+        // a dynamic variable assignment, e.g. ${$name} = ..., is invisible to native type inference, so the
+        // condition variable may be overwritten at runtime and is not safe to evaluate as always true
+        return $hasPlainVariable && $this->hasDynamicVariable();
+    }
+
+    private function hasDynamicVariable(): bool
+    {
+        return (bool) $this->betterNodeFinder->findFirst(
+            $this->getFile()
+                ->getNewStmts(),
+            static fn (Node $node): bool => $node instanceof Variable && $node->name instanceof Expr
+        );
     }
 
     private function shouldSkipExpr(Expr $expr): bool

rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php

@@ -20,9 +20,12 @@
 use PhpParser\Node\UnionType;
 use PhpParser\NodeVisitor;
 use PHPStan\PhpDocParser\Ast\PhpDoc\ParamTagValueNode;
+use PHPStan\PhpDocParser\Ast\PhpDoc\VarTagValueNode;
 use PHPStan\Reflection\ClassReflection;
 use PHPStan\Type\MixedType;
 use PHPStan\Type\TypeCombinator;
+use PHPStan\Type\TypeWithClassName;
+use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfo;
 use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfoFactory;
 use Rector\Contract\Rector\ConfigurableRectorInterface;
 use Rector\Naming\PropertyRenamer\PropertyPromotionRenamer;
@@ -213,6 +216,10 @@ public function refactor(Node $node): ?Node
                 continue;
             }
 
+            if ($this->shouldSkipNarrowingVarDoc($property, $param, $constructorPhpDocInfo, $paramName)) {
+                continue;
+            }
+
             $hasChanged = true;
 
             // remove property from class
@@ -388,6 +395,49 @@ private function shouldSkipPropertyOrParam(Property $property, Param $param): bo
             && ! $this->nodeComparator->areNodesEqual($property->type, $param->type);
     }
 
+    /**
+     * A class-typed property may carry a narrowing @var (e.g. native AdapterInterface, @var CacheProvider) used to
+     * type calls on the property. Promotion would drop that @var (it is not preserved as a @param here), $property so skip to
+     * avoid losing type information.
+     */
+    private function shouldSkipNarrowingVarDoc(
+        Property $property,
+        Param $param,
+        PhpDocInfo $constructorPhpDocInfo,
+        string $paramName
+    ): bool {
+        if (! $property->type instanceof Node || ! $param->type instanceof Node) {
+            return false;
+        }
+
+        // an explicit @param already carries the type onto the promoted property
+        if ($constructorPhpDocInfo->getParamTagValueByName($paramName) instanceof ParamTagValueNode) {
+            return false;
+        }
+
+        $propertyPhpDocInfo = $this->phpDocInfoFactory->createFromNodeOrEmpty($property);
+        $varTagValueNode = $propertyPhpDocInfo->getVarTagValueNode();
+        if (! $varTagValueNode instanceof VarTagValueNode) {
+            return false;
+        }
+
+        // a description keeps the docblock around anyway
+        if ($varTagValueNode->description !== '') {
+            return false;
+        }
+
+        $varType = $this->staticTypeMapper->mapPHPStanPhpDocTypeToPHPStanType($varTagValueNode, $property);
+
+        // only class-typed @var narrowing is lost; generics, arrays and int ranges are preserved on promotion
+        if (! $varType instanceof TypeWithClassName) {
+            return false;
+        }
+
+        $paramType = $this->staticTypeMapper->mapPhpParserNodePHPStanType($param->type);
+
+        return ! $this->typeComparator->areTypesEqual($varType, $paramType);
+    }
+
     private function shouldRemoveNullFromForPromotedParamType(Property $property, Param $param): bool
     {
         if (! $property->type instanceof Node || ! $param->type instanceof Node) {