From a394e5ea44ef09812ec1224aa0f402a483400d73 Mon Sep 17 00:00:00 2001 From: Tomas Votruba Date: Sat, 10 Jun 2023 20:48:27 +0200 Subject: [PATCH] Split instance of check to 2 rules (#4165) * Extract RemoveTypedPropertyDeadInstanceOfRector to handle properties in own scope * be sure --- config/set/dead-code.php | 2 + phpstan.neon | 2 + .../Fixture/skip_direct_method_call.php.inc | 4 +- .../Fixture/skip_property_fetch.php.inc | 26 +++ ...typed_property_filled_by_construct.php.inc | 4 +- ...perty_filled_by_construct_hardcode.php.inc | 6 +- ...d_property_not_filled_by_construct.php.inc | 2 +- .../Fixture/typed_property.php.inc | 4 +- ...perty_filled_by_construct_hardcode.php.inc | 4 +- ...ed_property_via_property_promotion.php.inc | 4 +- ...eTypedPropertyDeadInstanceOfRectorTest.php | 28 +++ .../config/configured_rule.php | 10 + .../Rector/If_/RemoveDeadInstanceOfRector.php | 141 +++-------- ...emoveTypedPropertyDeadInstanceOfRector.php | 220 ++++++++++++++++++ 14 files changed, 330 insertions(+), 127 deletions(-) create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveDeadInstanceOfRector/Fixture/skip_property_fetch.php.inc rename rules-tests/DeadCode/Rector/If_/{RemoveDeadInstanceOfRector => RemoveTypedPropertyDeadInstanceOfRector}/Fixture/non_typed_property_filled_by_construct.php.inc (78%) rename rules-tests/DeadCode/Rector/If_/{RemoveDeadInstanceOfRector => RemoveTypedPropertyDeadInstanceOfRector}/Fixture/non_typed_property_filled_by_construct_hardcode.php.inc (75%) rename rules-tests/DeadCode/Rector/If_/{RemoveDeadInstanceOfRector => RemoveTypedPropertyDeadInstanceOfRector}/Fixture/skip_non_typed_property_not_filled_by_construct.php.inc (80%) rename rules-tests/DeadCode/Rector/If_/{RemoveDeadInstanceOfRector => RemoveTypedPropertyDeadInstanceOfRector}/Fixture/typed_property.php.inc (76%) rename rules-tests/DeadCode/Rector/If_/{RemoveDeadInstanceOfRector => RemoveTypedPropertyDeadInstanceOfRector}/Fixture/typed_property_filled_by_construct_hardcode.php.inc (77%) rename rules-tests/DeadCode/Rector/If_/{RemoveDeadInstanceOfRector => RemoveTypedPropertyDeadInstanceOfRector}/Fixture/typed_property_via_property_promotion.php.inc (74%) create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveTypedPropertyDeadInstanceOfRector/RemoveTypedPropertyDeadInstanceOfRectorTest.php create mode 100644 rules-tests/DeadCode/Rector/If_/RemoveTypedPropertyDeadInstanceOfRector/config/configured_rule.php create mode 100644 rules/DeadCode/Rector/If_/RemoveTypedPropertyDeadInstanceOfRector.php diff --git a/config/set/dead-code.php b/config/set/dead-code.php index 71e9e76ccc9..4e84d9e0f1f 100644 --- a/config/set/dead-code.php +++ b/config/set/dead-code.php @@ -28,6 +28,7 @@ use Rector\DeadCode\Rector\FunctionLike\RemoveDeadReturnRector; use Rector\DeadCode\Rector\If_\RemoveAlwaysTrueIfConditionRector; use Rector\DeadCode\Rector\If_\RemoveDeadInstanceOfRector; +use Rector\DeadCode\Rector\If_\RemoveTypedPropertyDeadInstanceOfRector; use Rector\DeadCode\Rector\If_\RemoveUnusedNonEmptyArrayBeforeForeachRector; use Rector\DeadCode\Rector\If_\SimplifyIfElseWithSameContentRector; use Rector\DeadCode\Rector\If_\UnwrapFutureCompatibleIfPhpVersionRector; @@ -78,6 +79,7 @@ RemoveDeadConditionAboveReturnRector::class, RemoveUnusedConstructorParamRector::class, RemoveDeadInstanceOfRector::class, + RemoveTypedPropertyDeadInstanceOfRector::class, RemoveDeadLoopRector::class, RemoveUnusedPrivateMethodParameterRector::class, // docblock diff --git a/phpstan.neon b/phpstan.neon index b808b6213d3..8d5815cf389 100644 --- a/phpstan.neon +++ b/phpstan.neon @@ -704,3 +704,5 @@ parameters: - '#Function "class_exists\(\)" cannot be used/left in the code\: use ReflectionProvider\->has\*\(\) instead#' - '#Cognitive complexity for "Rector\\Php81\\Rector\\Property\\ReadOnlyPropertyRector\:\:refactorWithScope\(\)" is 12, keep it under 11#' + + - '#Parameter \#2 \$callable of method Rector\\Core\\Rector\\AbstractRector\:\:traverseNodesWithCallable\(\) expects callable\(PhpParser\\Node\)\: \(int\|PhpParser\\Node\|null\), Closure\(PhpParser\\Node\)\: \(array\|int\|null\) given#' diff --git a/rules-tests/DeadCode/Rector/If_/RemoveDeadInstanceOfRector/Fixture/skip_direct_method_call.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveDeadInstanceOfRector/Fixture/skip_direct_method_call.php.inc index 2de6dfef75c..6d73e4f4c96 100644 --- a/rules-tests/DeadCode/Rector/If_/RemoveDeadInstanceOfRector/Fixture/skip_direct_method_call.php.inc +++ b/rules-tests/DeadCode/Rector/If_/RemoveDeadInstanceOfRector/Fixture/skip_direct_method_call.php.inc @@ -4,7 +4,7 @@ namespace Rector\Tests\DeadCode\Rector\If_\RemoveDeadInstanceOfRector\Fixture; use stdClass; -class SkipDirectMethodCall +final class SkipDirectMethodCall { private function get(): stdClass { @@ -22,5 +22,3 @@ class SkipDirectMethodCall return true; } } - -?> diff --git a/rules-tests/DeadCode/Rector/If_/RemoveDeadInstanceOfRector/Fixture/skip_property_fetch.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveDeadInstanceOfRector/Fixture/skip_property_fetch.php.inc new file mode 100644 index 00000000000..8bb2e782fa8 --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveDeadInstanceOfRector/Fixture/skip_property_fetch.php.inc @@ -0,0 +1,26 @@ +var = new stdClass; + } + + public function go() + { + if (! $this->var instanceof stdClass) { + echo 'you need to run init() first' . PHP_EOL; + return; + } + + echo 'success' . PHP_EOL; + } +} diff --git a/rules-tests/DeadCode/Rector/If_/RemoveDeadInstanceOfRector/Fixture/non_typed_property_filled_by_construct.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveTypedPropertyDeadInstanceOfRector/Fixture/non_typed_property_filled_by_construct.php.inc similarity index 78% rename from rules-tests/DeadCode/Rector/If_/RemoveDeadInstanceOfRector/Fixture/non_typed_property_filled_by_construct.php.inc rename to rules-tests/DeadCode/Rector/If_/RemoveTypedPropertyDeadInstanceOfRector/Fixture/non_typed_property_filled_by_construct.php.inc index 37d41212e98..db26461e316 100644 --- a/rules-tests/DeadCode/Rector/If_/RemoveDeadInstanceOfRector/Fixture/non_typed_property_filled_by_construct.php.inc +++ b/rules-tests/DeadCode/Rector/If_/RemoveTypedPropertyDeadInstanceOfRector/Fixture/non_typed_property_filled_by_construct.php.inc @@ -1,6 +1,6 @@ doTestFile($filePath); + } + + public static function provideData(): Iterator + { + return self::yieldFilesFromDirectory(__DIR__ . '/Fixture'); + } + + public function provideConfigFilePath(): string + { + return __DIR__ . '/config/configured_rule.php'; + } +} diff --git a/rules-tests/DeadCode/Rector/If_/RemoveTypedPropertyDeadInstanceOfRector/config/configured_rule.php b/rules-tests/DeadCode/Rector/If_/RemoveTypedPropertyDeadInstanceOfRector/config/configured_rule.php new file mode 100644 index 00000000000..be5164d1671 --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveTypedPropertyDeadInstanceOfRector/config/configured_rule.php @@ -0,0 +1,10 @@ +rule(RemoveTypedPropertyDeadInstanceOfRector::class); +}; diff --git a/rules/DeadCode/Rector/If_/RemoveDeadInstanceOfRector.php b/rules/DeadCode/Rector/If_/RemoveDeadInstanceOfRector.php index fd12aa069c8..bd0a832266a 100644 --- a/rules/DeadCode/Rector/If_/RemoveDeadInstanceOfRector.php +++ b/rules/DeadCode/Rector/If_/RemoveDeadInstanceOfRector.php @@ -6,25 +6,22 @@ use PhpParser\Node; use PhpParser\Node\Expr; -use PhpParser\Node\Expr\Assign; use PhpParser\Node\Expr\BooleanNot; +use PhpParser\Node\Expr\CallLike; use PhpParser\Node\Expr\Instanceof_; use PhpParser\Node\Expr\PropertyFetch; use PhpParser\Node\Expr\StaticPropertyFetch; -use PhpParser\Node\Expr\Variable; -use PhpParser\Node\FunctionLike; use PhpParser\Node\Name; use PhpParser\Node\Stmt; -use PhpParser\Node\Stmt\Class_; +use PhpParser\Node\Stmt\Do_; +use PhpParser\Node\Stmt\For_; +use PhpParser\Node\Stmt\Foreach_; use PhpParser\Node\Stmt\If_; -use PhpParser\Node\Stmt\Property; +use PhpParser\Node\Stmt\While_; use PhpParser\NodeTraverser; -use Rector\Core\NodeAnalyzer\PropertyFetchAnalyzer; +use PHPStan\Type\MixedType; use Rector\Core\NodeManipulator\IfManipulator; use Rector\Core\Rector\AbstractRector; -use Rector\NodeNestingScope\ContextAnalyzer; -use Rector\Php80\NodeAnalyzer\PromotedPropertyResolver; -use Rector\TypeDeclaration\AlreadyAssignDetector\ConstructorAssignDetector; use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; @@ -35,10 +32,6 @@ final class RemoveDeadInstanceOfRector extends AbstractRector { public function __construct( private readonly IfManipulator $ifManipulator, - private readonly PropertyFetchAnalyzer $propertyFetchAnalyzer, - private readonly ConstructorAssignDetector $constructorAssignDetector, - private readonly PromotedPropertyResolver $promotedPropertyResolver, - private readonly ContextAnalyzer $contextAnalyzer ) { } @@ -47,26 +40,20 @@ public function getRuleDefinition(): RuleDefinition return new RuleDefinition('Remove dead instanceof check on type hinted variable', [ new CodeSample( <<<'CODE_SAMPLE' -final class SomeClass +function run(stdClass $stdClass) { - public function go(stdClass $stdClass) - { - if (! $stdClass instanceof stdClass) { - return false; - } - - return true; + if (! $stdClass instanceof stdClass) { + return false; } + + return true; } CODE_SAMPLE , <<<'CODE_SAMPLE' -final class SomeClass +function run(stdClass $stdClass) { - public function go(stdClass $stdClass) - { - return true; - } + return true; } CODE_SAMPLE ), @@ -78,20 +65,21 @@ public function go(stdClass $stdClass) */ public function getNodeTypes(): array { - return [If_::class]; + return [If_::class, For_::class, Foreach_::class, While_::class, Do_::class]; } /** - * @param If_ $node + * @param If_|For_|Foreach_|While_|Do_ $node * @return Stmt[]|null|int */ public function refactor(Node $node) { - if (! $this->ifManipulator->isIfWithoutElseAndElseIfs($node)) { - return null; + // avoid ifs in a loop, as unexpected behavior + if (! $node instanceof If_) { + return NodeTraverser::STOP_TRAVERSAL; } - if ($this->contextAnalyzer->isInLoop($node)) { + if (! $this->ifManipulator->isIfWithoutElseAndElseIfs($node)) { return null; } @@ -115,6 +103,11 @@ private function refactorStmtAndInstanceof(If_ $if, Instanceof_ $instanceof): nu return null; } + // handle in another rule + if ($this->isPropertyFetch($instanceof->expr) || $instanceof->expr instanceof CallLike) { + return null; + } + $classType = $this->nodeTypeResolver->getType($instanceof->class); $exprType = $this->nodeTypeResolver->getType($instanceof->expr); @@ -125,12 +118,6 @@ private function refactorStmtAndInstanceof(If_ $if, Instanceof_ $instanceof): nu return null; } - if (! $instanceof->expr instanceof Variable && ! $this->isInPropertyPromotedParams( - $instanceof->expr - ) && $this->isSkippedPropertyFetch($instanceof->expr)) { - return null; - } - if ($this->shouldSkipFromNotTypedParam($instanceof)) { return null; } @@ -143,90 +130,22 @@ private function refactorStmtAndInstanceof(If_ $if, Instanceof_ $instanceof): nu return NodeTraverser::REMOVE_NODE; } + // unwrap stmts return $if->stmts; } private function shouldSkipFromNotTypedParam(Instanceof_ $instanceof): bool { - $functionLike = $this->betterNodeFinder->findParentType($instanceof, FunctionLike::class); - if (! $functionLike instanceof FunctionLike) { - return false; - } - - $variable = $instanceof->expr; - $isReAssign = (bool) $this->betterNodeFinder->findFirstPrevious( - $instanceof, - fn (Node $subNode): bool => $subNode instanceof Assign && $this->nodeComparator->areNodesEqual( - $subNode->var, - $variable - ) - ); - - if ($isReAssign) { - return false; - } - - $params = $functionLike->getParams(); - foreach ($params as $param) { - if ($this->nodeComparator->areNodesEqual($param->var, $instanceof->expr)) { - return $param->type === null; - } - } - - return false; + $nativeParamType = $this->nodeTypeResolver->getNativeType($instanceof->expr); + return $nativeParamType instanceof MixedType; } - private function isSkippedPropertyFetch(Expr $expr): bool + private function isPropertyFetch(Expr $expr): bool { - if (! $this->propertyFetchAnalyzer->isPropertyFetch($expr)) { - return true; - } - - /** @var PropertyFetch|StaticPropertyFetch $propertyFetch */ - $propertyFetch = $expr; - $classLike = $this->betterNodeFinder->findParentType($propertyFetch, Class_::class); - - if (! $classLike instanceof Class_) { + if ($expr instanceof PropertyFetch) { return true; } - /** @var string $propertyName */ - $propertyName = $this->nodeNameResolver->getName($propertyFetch); - $property = $classLike->getProperty($propertyName); - - if (! $property instanceof Property) { - return true; - } - - $isPropertyAssignedInConstuctor = $this->constructorAssignDetector->isPropertyAssigned( - $classLike, - $propertyName - ); - - return $property->type === null && ! $isPropertyAssignedInConstuctor; - } - - private function isInPropertyPromotedParams(Expr $expr): bool - { - if (! $expr instanceof PropertyFetch) { - return false; - } - - $classLike = $this->betterNodeFinder->findParentType($expr, Class_::class); - if (! $classLike instanceof Class_) { - return false; - } - - /** @var string $propertyName */ - $propertyName = $this->nodeNameResolver->getName($expr); - $params = $this->promotedPropertyResolver->resolveFromClass($classLike); - - foreach ($params as $param) { - if ($this->nodeNameResolver->isName($param, $propertyName)) { - return true; - } - } - - return false; + return $expr instanceof StaticPropertyFetch; } } diff --git a/rules/DeadCode/Rector/If_/RemoveTypedPropertyDeadInstanceOfRector.php b/rules/DeadCode/Rector/If_/RemoveTypedPropertyDeadInstanceOfRector.php new file mode 100644 index 00000000000..d2ff1e2a051 --- /dev/null +++ b/rules/DeadCode/Rector/If_/RemoveTypedPropertyDeadInstanceOfRector.php @@ -0,0 +1,220 @@ +someObject = $someObject; + } + + public function run() + { + if ($this->someObject instanceof SomeObject) { + return true; + } + + return false; + } +} +CODE_SAMPLE + , + <<<'CODE_SAMPLE' +final class SomeClass +{ + private $someObject; + + public function __construct($someObject) + { + $this->someObject = $someObject; + } + + public function run() + { + return true; + } +} +CODE_SAMPLE + ), + ]); + } + + /** + * @return array> + */ + public function getNodeTypes(): array + { + return [Class_::class]; + } + + /** + * @param Class_ $node + */ + public function refactor(Node $node): ?Class_ + { + $hasChanged = false; + $class = $node; + + $this->traverseNodesWithCallable($node->getMethods(), function (Node $node) use ( + &$hasChanged, + $class + ): int|null|array { + // avoid loop ifs + if ($node instanceof While_ || $node instanceof Foreach_ || $node instanceof For_ || $node instanceof Do_) { + return NodeTraverser::STOP_TRAVERSAL; + } + + if (! $node instanceof If_) { + return null; + } + + if (! $this->ifManipulator->isIfWithoutElseAndElseIfs($node)) { + return null; + } + + if ($node->cond instanceof BooleanNot && $node->cond->expr instanceof Instanceof_) { + $result = $this->refactorStmtAndInstanceof($class, $node, $node->cond->expr); + if ($result !== null) { + $hasChanged = true; + return $result; + } + } + + if ($node->cond instanceof Instanceof_) { + $result = $this->refactorStmtAndInstanceof($class, $node, $node->cond); + if ($result !== null) { + $hasChanged = true; + return $result; + } + } + + return null; + }); + + if ($hasChanged) { + return $node; + } + + return null; + } + + /** + * @return null|Stmt[]|int + */ + private function refactorStmtAndInstanceof(Class_ $class, If_ $if, Instanceof_ $instanceof): null|array|int + { + // check local property only + if (! $instanceof->expr instanceof PropertyFetch || ! $this->isName($instanceof->expr->var, 'this')) { + return null; + } + + if (! $instanceof->class instanceof Name) { + return null; + } + + $classType = $this->nodeTypeResolver->getType($instanceof->class); + $exprType = $this->nodeTypeResolver->getType($instanceof->expr); + + $isSameStaticTypeOrSubtype = $classType->equals($exprType) || $classType->isSuperTypeOf($exprType) + ->yes(); + + if (! $isSameStaticTypeOrSubtype) { + return null; + } + + if (! $this->isInPropertyPromotedParams($class, $instanceof->expr) && $this->isSkippedPropertyFetch( + $class, + $instanceof->expr + )) { + return null; + } + + if ($if->cond !== $instanceof) { + return NodeTraverser::REMOVE_NODE; + } + + if ($if->stmts === []) { + return NodeTraverser::REMOVE_NODE; + } + + return $if->stmts; + } + + private function isSkippedPropertyFetch(Class_ $class, PropertyFetch|StaticPropertyFetch $propertyFetch): bool + { + $propertyName = $this->getName($propertyFetch->name); + if ($propertyName === null) { + return true; + } + + if ($this->constructorAssignDetector->isPropertyAssigned($class, $propertyName)) { + return false; + } + + $property = $class->getProperty($propertyName); + if (! $property instanceof Property) { + return false; + } + + return $property->type === null; + } + + private function isInPropertyPromotedParams(Class_ $class, PropertyFetch|StaticPropertyFetch $propertyFetch): bool + { + /** @var string $propertyName */ + $propertyName = $this->nodeNameResolver->getName($propertyFetch); + $params = $this->promotedPropertyResolver->resolveFromClass($class); + + foreach ($params as $param) { + if ($this->nodeNameResolver->isName($param, $propertyName)) { + return true; + } + } + + return false; + } +}