diff --git a/rules-tests/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector/Fixture/null_reset_of_plain_object.php.inc b/rules-tests/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector/Fixture/null_reset_of_plain_object.php.inc deleted file mode 100644 index c7ce8dfabaf..00000000000 --- a/rules-tests/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector/Fixture/null_reset_of_plain_object.php.inc +++ /dev/null @@ -1,29 +0,0 @@ -name; - - $value = null; - } -} -?> ------ -name; - } -} -?> diff --git a/rules-tests/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector/Fixture/null_reset_of_user_object.php.inc b/rules-tests/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector/Fixture/null_reset_of_user_object.php.inc new file mode 100644 index 00000000000..4bc03859d7e --- /dev/null +++ b/rules-tests/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector/Fixture/null_reset_of_user_object.php.inc @@ -0,0 +1,33 @@ + +----- + diff --git a/rules-tests/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector/Fixture/skip_null_reset_of_internal_object.php.inc b/rules-tests/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector/Fixture/skip_null_reset_of_internal_object.php.inc new file mode 100644 index 00000000000..d11dbc4ee28 --- /dev/null +++ b/rules-tests/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector/Fixture/skip_null_reset_of_internal_object.php.inc @@ -0,0 +1,14 @@ +file($path); + + $finfo = null; + } +} diff --git a/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_dynamic_variable_assign.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_dynamic_variable_assign.php.inc new file mode 100644 index 00000000000..41a1dcd22ee --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_dynamic_variable_assign.php.inc @@ -0,0 +1,23 @@ +cacheProvider = $cacheProvider; + } + + public function run(): object + { + return $this->cacheProvider->getCacheAdapter(); + } +} diff --git a/rules-tests/Php80/Rector/Class_/ClassPropertyAssignToConstructorPromotionRector/Source/SomeCacheAdapterInterface.php b/rules-tests/Php80/Rector/Class_/ClassPropertyAssignToConstructorPromotionRector/Source/SomeCacheAdapterInterface.php new file mode 100644 index 00000000000..77add8857bb --- /dev/null +++ b/rules-tests/Php80/Rector/Class_/ClassPropertyAssignToConstructorPromotionRector/Source/SomeCacheAdapterInterface.php @@ -0,0 +1,10 @@ +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 diff --git a/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php b/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php index b17a1b2678e..37dc380a6e7 100644 --- a/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php +++ b/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.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 diff --git a/rules/Php80/Rector/Class_/ClassPropertyAssignToConstructorPromotionRector.php b/rules/Php80/Rector/Class_/ClassPropertyAssignToConstructorPromotionRector.php index cd2038334ea..8ccc1c27ba7 100644 --- a/rules/Php80/Rector/Class_/ClassPropertyAssignToConstructorPromotionRector.php +++ b/rules/Php80/Rector/Class_/ClassPropertyAssignToConstructorPromotionRector.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) {