Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

This file was deleted.

Original file line number Diff line number Diff line change
@@ -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);
}
}
?>
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
@@ -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';
}
}
Original file line number Diff line number Diff line change
@@ -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();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

declare(strict_types=1);

namespace Rector\Tests\Php80\Rector\Class_\ClassPropertyAssignToConstructorPromotionRector\Source;

interface SomeCacheAdapterInterface
{
public function clear(): bool;
}
Original file line number Diff line number Diff line change
@@ -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;
}
}
21 changes: 15 additions & 6 deletions rules/DeadCode/Rector/Assign/RemoveUnusedVariableAssignRector.php
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ public function refactor(Node $node): null|ClassMethod|Function_
continue;
}

if ($this->isNullResetOfObject($assign)) {
if ($this->isNullResetOfInternalObject($assign)) {
continue;
}

Expand All @@ -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
Expand Down
18 changes: 17 additions & 1 deletion rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand All @@ -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(),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lookup on whole new stmts, seems too much, I think we can lookup from phpstan $scope->getParentScope()->hasVariableType() or something....

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

static fn (Node $node): bool => $node instanceof Variable && $node->name instanceof Expr
);
}

private function shouldSkipExpr(Expr $expr): bool
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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) {
Expand Down
Loading