From 1e3ed8abecff1e1c3de1b46d32495a65f216e8ca Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 28 Jul 2022 00:11:21 +0700 Subject: [PATCH 01/13] [DeadCode] Skip RemoveAlwaysTrueIfConditionRector on property use by @var docblock --- .../Fixture/skip_property_by_var_doc.php.inc | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+) create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc.php.inc diff --git a/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc.php.inc new file mode 100644 index 00000000000..e3017b3bdba --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc.php.inc @@ -0,0 +1,25 @@ +property = new \Memcached(); + } + } + + public function verify() + { + if ($this->property instanceof \Memcached) { + return true; + } + + return false; + } +} \ No newline at end of file From 052dcaa9bad9c18b44c5022cd2545a68e4fcb878 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 28 Jul 2022 00:26:25 +0700 Subject: [PATCH 02/13] Fixed :tada: --- .../If_/RemoveAlwaysTrueIfConditionRector.php | 44 +++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php b/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php index a29469ef4f8..d0eca6f6853 100644 --- a/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php +++ b/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php @@ -5,8 +5,13 @@ namespace Rector\DeadCode\Rector\If_; use PhpParser\Node; +use PhpParser\Node\Expr; +use PhpParser\Node\Expr\PropertyFetch; +use PhpParser\Node\Expr\StaticPropertyFetch; use PhpParser\Node\Stmt; +use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\If_; +use PhpParser\Node\Stmt\Property; use PHPStan\Type\Constant\ConstantBooleanType; use Rector\Core\Rector\AbstractRector; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; @@ -73,6 +78,10 @@ public function refactor(Node $node): If_|null|array return null; } + if ($this->shouldSkipPropertyFetch($node->cond)) { + return null; + } + $conditionStaticType = $this->getType($node->cond); if (! $conditionStaticType instanceof ConstantBooleanType) { return null; @@ -89,4 +98,39 @@ public function refactor(Node $node): If_|null|array return $node->stmts; } + + private function shouldSkipPropertyFetch(Expr $expr): bool + { + /** @var PropertyFetch[]|StaticPropertyFetch[] $propertyFetches */ + $propertyFetches = $this->betterNodeFinder->findInstancesOf( + $expr, + [PropertyFetch::class, StaticPropertyFetch::class] + ); + $classLike = $this->betterNodeFinder->findParentType($expr, ClassLike::class); + + if (! $classLike instanceof ClassLike) { + return false; + } + + foreach ($propertyFetches as $propertyFetch) { + $propertyFetchClassLike = $this->betterNodeFinder->findParentType($propertyFetch, ClassLike::class); + + if ($propertyFetchClassLike !== $classLike) { + continue; + } + + $propertyName = (string) $this->nodeNameResolver->getName($propertyFetch); + $property = $classLike->getProperty($propertyName); + + if (! $property instanceof Property) { + continue; + } + + if (! $property->type instanceof Node) { + return true; + } + } + + return false; + } } From 1e7a0ee98f4432bd4f55e9b9378c63959cc7d2e1 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 28 Jul 2022 00:27:05 +0700 Subject: [PATCH 03/13] final touch: eol --- .../Fixture/skip_property_by_var_doc.php.inc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc.php.inc index e3017b3bdba..6385f18db04 100644 --- a/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc.php.inc +++ b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc.php.inc @@ -22,4 +22,4 @@ class SkipPropertyByVarDoc return false; } -} \ No newline at end of file +} From 3b9e9b9249e296c544cc1d0c6e2f69e2fc98d9e8 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 28 Jul 2022 00:31:56 +0700 Subject: [PATCH 04/13] final touch: clean up --- .../Rector/If_/RemoveAlwaysTrueIfConditionRector.php | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php b/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php index d0eca6f6853..3c3ed48d596 100644 --- a/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php +++ b/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php @@ -78,10 +78,6 @@ public function refactor(Node $node): If_|null|array return null; } - if ($this->shouldSkipPropertyFetch($node->cond)) { - return null; - } - $conditionStaticType = $this->getType($node->cond); if (! $conditionStaticType instanceof ConstantBooleanType) { return null; @@ -91,6 +87,10 @@ public function refactor(Node $node): If_|null|array return null; } + if ($this->shouldSkipPropertyFetch($node->cond)) { + return null; + } + if ($node->stmts === []) { $this->removeNode($node); return null; From 7bb9ed1d47a8f9e408ff03474881a7ecec76ae80 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 28 Jul 2022 00:33:46 +0700 Subject: [PATCH 05/13] final touch: clean up --- .../Fixture/skip_property_by_var_doc.php.inc | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc.php.inc index 6385f18db04..740997bbb4d 100644 --- a/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc.php.inc +++ b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc.php.inc @@ -4,19 +4,19 @@ namespace Rector\Tests\DeadCode\Rector\If_\RemoveAlwaysTrueIfConditionRector\Fix class SkipPropertyByVarDoc { - /** @var \Memcached */ + /** @var \DateTime */ private $property; public function __construct() { - if (class_exists(\Memcached::class)) { - $this->property = new \Memcached(); + if (rand(0, 1)) { + $this->property = new \DateTime('now'); } } public function verify() { - if ($this->property instanceof \Memcached) { + if ($this->property instanceof \DateTime) { return true; } From a467fe79e0f97ce0628e82a4bf9276a78ae580ec Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 28 Jul 2022 00:40:40 +0700 Subject: [PATCH 06/13] final touch: clean up --- .../If_/RemoveAlwaysTrueIfConditionRector.php | 25 ++++++++----------- 1 file changed, 10 insertions(+), 15 deletions(-) diff --git a/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php b/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php index 3c3ed48d596..588a968956a 100644 --- a/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php +++ b/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php @@ -9,11 +9,11 @@ use PhpParser\Node\Expr\PropertyFetch; use PhpParser\Node\Expr\StaticPropertyFetch; use PhpParser\Node\Stmt; -use PhpParser\Node\Stmt\ClassLike; use PhpParser\Node\Stmt\If_; -use PhpParser\Node\Stmt\Property; +use PHPStan\Reflection\ClassReflection; use PHPStan\Type\Constant\ConstantBooleanType; use Rector\Core\Rector\AbstractRector; +use Rector\Core\Reflection\ReflectionResolver; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; @@ -22,6 +22,10 @@ */ final class RemoveAlwaysTrueIfConditionRector extends AbstractRector { + public function __construct(private readonly ReflectionResolver $reflectionResolver) + { + } + public function getRuleDefinition(): RuleDefinition { return new RuleDefinition('Remove if condition that is always true', [ @@ -106,27 +110,18 @@ private function shouldSkipPropertyFetch(Expr $expr): bool $expr, [PropertyFetch::class, StaticPropertyFetch::class] ); - $classLike = $this->betterNodeFinder->findParentType($expr, ClassLike::class); - - if (! $classLike instanceof ClassLike) { - return false; - } foreach ($propertyFetches as $propertyFetch) { - $propertyFetchClassLike = $this->betterNodeFinder->findParentType($propertyFetch, ClassLike::class); + $classReflection = $this->reflectionResolver->resolveClassReflection($propertyFetch); - if ($propertyFetchClassLike !== $classLike) { + if (! $classReflection instanceof ClassReflection) { continue; } $propertyName = (string) $this->nodeNameResolver->getName($propertyFetch); - $property = $classLike->getProperty($propertyName); - - if (! $property instanceof Property) { - continue; - } + $nativeProperty = $classReflection->getNativeProperty($propertyName); - if (! $property->type instanceof Node) { + if (! $nativeProperty->hasNativeType()) { return true; } } From 96c52c3c1a490f0610d56c7e0ac1cfce8ae1e15e Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 28 Jul 2022 00:42:14 +0700 Subject: [PATCH 07/13] clean up --- .../Rector/If_/RemoveAlwaysTrueIfConditionRector.php | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php b/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php index 588a968956a..4633496d5eb 100644 --- a/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php +++ b/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php @@ -119,8 +119,12 @@ private function shouldSkipPropertyFetch(Expr $expr): bool } $propertyName = (string) $this->nodeNameResolver->getName($propertyFetch); - $nativeProperty = $classReflection->getNativeProperty($propertyName); + if (! $classReflection->hasNativeProperty($propertyName)) { + continue; + } + + $nativeProperty = $classReflection->getNativeProperty($propertyName); if (! $nativeProperty->hasNativeType()) { return true; } From 98e0347898c9eaf376a89f3a71baaa623cf577f4 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 28 Jul 2022 00:48:55 +0700 Subject: [PATCH 08/13] add failing test case for call outside object --- .../Fixture/skip_property_by_var_doc2.php.inc | 18 ++++++++++++++++++ .../SomeClassWithPropertyUseVarDocblock.php | 9 +++++++++ 2 files changed, 27 insertions(+) create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc2.php.inc create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Source/SomeClassWithPropertyUseVarDocblock.php diff --git a/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc2.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc2.php.inc new file mode 100644 index 00000000000..0ef8f7719f8 --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Fixture/skip_property_by_var_doc2.php.inc @@ -0,0 +1,18 @@ +property instanceof \DateTime) { + return true; + } + + return false; + } +} diff --git a/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Source/SomeClassWithPropertyUseVarDocblock.php b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Source/SomeClassWithPropertyUseVarDocblock.php new file mode 100644 index 00000000000..d51ac677115 --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Source/SomeClassWithPropertyUseVarDocblock.php @@ -0,0 +1,9 @@ + Date: Thu, 28 Jul 2022 01:02:54 +0700 Subject: [PATCH 09/13] Fixed :tada: --- .../If_/RemoveAlwaysTrueIfConditionRector.php | 2 +- src/Reflection/ReflectionResolver.php | 26 ++++++++++++++++--- 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php b/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php index 4633496d5eb..4a01d280d95 100644 --- a/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php +++ b/rules/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector.php @@ -112,7 +112,7 @@ private function shouldSkipPropertyFetch(Expr $expr): bool ); foreach ($propertyFetches as $propertyFetch) { - $classReflection = $this->reflectionResolver->resolveClassReflection($propertyFetch); + $classReflection = $this->reflectionResolver->resolveClassReflectionSourceObject($propertyFetch); if (! $classReflection instanceof ClassReflection) { continue; diff --git a/src/Reflection/ReflectionResolver.php b/src/Reflection/ReflectionResolver.php index 153fca7f8bc..867d8fc97d7 100644 --- a/src/Reflection/ReflectionResolver.php +++ b/src/Reflection/ReflectionResolver.php @@ -31,6 +31,7 @@ use Rector\NodeNameResolver\NodeNameResolver; use Rector\NodeTypeResolver\Node\AttributeKey; use Rector\NodeTypeResolver\NodeTypeResolver; +use Rector\StaticTypeMapper\ValueObject\Type\FullyQualifiedObjectType; use Symfony\Contracts\Service\Attribute\Required; final class ReflectionResolver @@ -84,9 +85,28 @@ public function resolveClassReflection(?Node $node): ?ClassReflection return $scope->getClassReflection(); } - public function resolveClassReflectionSourceObject(MethodCall|StaticCall $call): ?ClassReflection - { - $classMethod = $this->astResolver->resolveClassMethodFromCall($call); + public function resolveClassReflectionSourceObject( + MethodCall|StaticCall|PropertyFetch|StaticPropertyFetch $node + ): ?ClassReflection { + if ($node instanceof PropertyFetch || $node instanceof StaticPropertyFetch) { + $objectType = $node instanceof PropertyFetch + ? $this->nodeTypeResolver->getType($node->var) + : $this->nodeTypeResolver->getType($node->class); + + if (! in_array($objectType::class, [TypeWithClassName::class, FullyQualifiedObjectType::class], true)) { + return null; + } + + /** @var TypeWithClassName|FullyQualifiedObjectType $objectType */ + $className = $objectType->getClassName(); + if (! $this->reflectionProvider->hasClass($className)) { + return null; + } + + return $this->reflectionProvider->getClass($className); + } + + $classMethod = $this->astResolver->resolveClassMethodFromCall($node); return $this->resolveClassReflection($classMethod); } From 07a769f371a672eb49a545cbbed8efc85b796884 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 28 Jul 2022 01:03:38 +0700 Subject: [PATCH 10/13] final touch: eol --- .../Source/SomeClassWithPropertyUseVarDocblock.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Source/SomeClassWithPropertyUseVarDocblock.php b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Source/SomeClassWithPropertyUseVarDocblock.php index d51ac677115..4c1f74d4ca3 100644 --- a/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Source/SomeClassWithPropertyUseVarDocblock.php +++ b/rules-tests/DeadCode/Rector/If_/RemoveAlwaysTrueIfConditionRector/Source/SomeClassWithPropertyUseVarDocblock.php @@ -6,4 +6,4 @@ class SomeClassWithPropertyUseVarDocblock { /** @var \DateTime */ public $property; -} \ No newline at end of file +} From 6d747518bc092c43755b061843099de220db5bef Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 28 Jul 2022 01:05:27 +0700 Subject: [PATCH 11/13] this type check --- src/Reflection/ReflectionResolver.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/Reflection/ReflectionResolver.php b/src/Reflection/ReflectionResolver.php index 867d8fc97d7..bc87f0f2544 100644 --- a/src/Reflection/ReflectionResolver.php +++ b/src/Reflection/ReflectionResolver.php @@ -22,7 +22,7 @@ use PHPStan\Reflection\Php\PhpPropertyReflection; use PHPStan\Reflection\ReflectionProvider; use PHPStan\Type\TypeUtils; -use PHPStan\Type\TypeWithClassName; +use PHPStan\Type\ThisType; use Rector\Core\NodeAnalyzer\ClassAnalyzer; use Rector\Core\PhpParser\AstResolver; use Rector\Core\PhpParser\Node\BetterNodeFinder; @@ -93,11 +93,11 @@ public function resolveClassReflectionSourceObject( ? $this->nodeTypeResolver->getType($node->var) : $this->nodeTypeResolver->getType($node->class); - if (! in_array($objectType::class, [TypeWithClassName::class, FullyQualifiedObjectType::class], true)) { + if (! in_array($objectType::class, [ThisType::class, FullyQualifiedObjectType::class], true)) { return null; } - /** @var TypeWithClassName|FullyQualifiedObjectType $objectType */ + /** @var ThisType|FullyQualifiedObjectType $objectType */ $className = $objectType->getClassName(); if (! $this->reflectionProvider->hasClass($className)) { return null; From ed258ded032f47c468c7fcb4028dceab91b86d90 Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 28 Jul 2022 01:07:07 +0700 Subject: [PATCH 12/13] this type check --- src/Reflection/ReflectionResolver.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Reflection/ReflectionResolver.php b/src/Reflection/ReflectionResolver.php index bc87f0f2544..bc0966fe6d4 100644 --- a/src/Reflection/ReflectionResolver.php +++ b/src/Reflection/ReflectionResolver.php @@ -22,6 +22,7 @@ use PHPStan\Reflection\Php\PhpPropertyReflection; use PHPStan\Reflection\ReflectionProvider; use PHPStan\Type\TypeUtils; +use PHPStan\Type\TypeWithClassName; use PHPStan\Type\ThisType; use Rector\Core\NodeAnalyzer\ClassAnalyzer; use Rector\Core\PhpParser\AstResolver; From a8c00254b4afc5427057e6b81820d3c36fea7c5a Mon Sep 17 00:00:00 2001 From: Abdul Malik Ikhsan Date: Thu, 28 Jul 2022 01:08:36 +0700 Subject: [PATCH 13/13] clean up --- src/Reflection/ReflectionResolver.php | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/Reflection/ReflectionResolver.php b/src/Reflection/ReflectionResolver.php index bc0966fe6d4..2516767b97f 100644 --- a/src/Reflection/ReflectionResolver.php +++ b/src/Reflection/ReflectionResolver.php @@ -23,7 +23,6 @@ use PHPStan\Reflection\ReflectionProvider; use PHPStan\Type\TypeUtils; use PHPStan\Type\TypeWithClassName; -use PHPStan\Type\ThisType; use Rector\Core\NodeAnalyzer\ClassAnalyzer; use Rector\Core\PhpParser\AstResolver; use Rector\Core\PhpParser\Node\BetterNodeFinder; @@ -32,7 +31,6 @@ use Rector\NodeNameResolver\NodeNameResolver; use Rector\NodeTypeResolver\Node\AttributeKey; use Rector\NodeTypeResolver\NodeTypeResolver; -use Rector\StaticTypeMapper\ValueObject\Type\FullyQualifiedObjectType; use Symfony\Contracts\Service\Attribute\Required; final class ReflectionResolver @@ -94,11 +92,10 @@ public function resolveClassReflectionSourceObject( ? $this->nodeTypeResolver->getType($node->var) : $this->nodeTypeResolver->getType($node->class); - if (! in_array($objectType::class, [ThisType::class, FullyQualifiedObjectType::class], true)) { + if (! $objectType instanceof TypeWithClassName) { return null; } - /** @var ThisType|FullyQualifiedObjectType $objectType */ $className = $objectType->getClassName(); if (! $this->reflectionProvider->hasClass($className)) { return null;