diff --git a/composer.json b/composer.json index 6e3361ad414..5924bbc962f 100644 --- a/composer.json +++ b/composer.json @@ -43,7 +43,7 @@ "webmozart/assert": "^1.11" }, "require-dev": { - "brianium/paratest": "^7.1.2", + "brianium/paratest": "^7.1.3", "cweagans/composer-patches": "^1.7.2", "icanhazstring/composer-unused": "^0.8.5", "myclabs/php-enum": "^1.8.4", @@ -53,7 +53,7 @@ "phpstan/phpstan-php-parser": "^1.1", "phpstan/phpstan-strict-rules": "^1.4.4", "phpstan/phpstan-webmozart-assert": "^1.2.2", - "phpunit/phpunit": "^10.0.17", + "phpunit/phpunit": "^10.1.0", "rector/phpstan-rules": "^0.6.5", "rector/rector-generator": "dev-main", "spatie/enum": "^3.13", diff --git a/rector.php b/rector.php index c6ca105e623..6fac866bb6b 100644 --- a/rector.php +++ b/rector.php @@ -82,6 +82,11 @@ // false positive on plurals __DIR__ . '/packages/Testing/PHPUnit/Behavior/MovingFilesTrait.php', ], + + // race condition with stmts aware patch and PHPStan type + \Rector\TypeDeclaration\Rector\ClassMethod\AddMethodCallBasedStrictParamTypeRector::class => [ + __DIR__ . '/rules/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector.php', + ], ]); $rectorConfig->phpstanConfig(__DIR__ . '/phpstan-for-rector.neon'); diff --git a/rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/if_check_with_empty.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/if_check_with_empty.php.inc new file mode 100644 index 00000000000..86c384b5da0 --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/if_check_with_empty.php.inc @@ -0,0 +1,35 @@ + +----- + diff --git a/rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/if_with_return_value_inside_foreach.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/if_with_return_value_inside_foreach.php.inc new file mode 100644 index 00000000000..39392e83de1 --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/if_with_return_value_inside_foreach.php.inc @@ -0,0 +1,35 @@ + +----- + diff --git a/rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/multi_if_check_with_empty.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/multi_if_check_with_empty.php.inc new file mode 100644 index 00000000000..8f9e6415cd0 --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/multi_if_check_with_empty.php.inc @@ -0,0 +1,51 @@ + +----- + diff --git a/rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/skip_if_check_throw_with_empty.php.inc b/rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/skip_if_check_throw_with_empty.php.inc new file mode 100644 index 00000000000..f5c7da14c2e --- /dev/null +++ b/rules-tests/DeadCode/Rector/If_/RemoveUnusedNonEmptyArrayBeforeForeachRector/Fixture/skip_if_check_throw_with_empty.php.inc @@ -0,0 +1,19 @@ +isUselessBeforeForeachCheck($node, $scope)) { - return null; - } + if ($node instanceof If_) { + if (! $this->isUselessBeforeForeachCheck($node, $scope)) { + return null; + } - /** @var Foreach_ $stmt */ - $stmt = $node->stmts[0]; - $ifComments = $node->getAttribute(AttributeKey::COMMENTS) ?? []; - $stmtComments = $stmt->getAttribute(AttributeKey::COMMENTS) ?? []; + /** @var Foreach_ $stmt */ + $stmt = $node->stmts[0]; + $ifComments = $node->getAttribute(AttributeKey::COMMENTS) ?? []; + $stmtComments = $stmt->getAttribute(AttributeKey::COMMENTS) ?? []; - $comments = array_merge($ifComments, $stmtComments); - $stmt->setAttribute(AttributeKey::COMMENTS, $comments); + $comments = array_merge($ifComments, $stmtComments); + $stmt->setAttribute(AttributeKey::COMMENTS, $comments); - return $stmt; + return $stmt; + } + + return $this->refactorStmtsAware($node); } private function isUselessBeforeForeachCheck(If_ $if, Scope $scope): bool @@ -156,4 +161,34 @@ private function isUselessBooleanAnd(BooleanAnd $booleanAnd, Expr $foreachExpr): return $this->countManipulator->isCounterHigherThanOne($booleanAnd->right, $foreachExpr); } + + private function refactorStmtsAware(StmtsAwareInterface $stmtsAware): ?StmtsAwareInterface + { + if ($stmtsAware->stmts === null) { + return null; + } + + /** @var int $lastKey */ + $lastKey = array_key_last($stmtsAware->stmts); + if (! isset($stmtsAware->stmts[$lastKey], $stmtsAware->stmts[$lastKey - 1])) { + return null; + } + + $stmt = $stmtsAware->stmts[$lastKey - 1]; + if (! $stmt instanceof If_) { + return null; + } + + $nextStmt = $stmtsAware->stmts[$lastKey]; + if (! $nextStmt instanceof Foreach_) { + return null; + } + + if (! $this->uselessIfCondBeforeForeachDetector->isMatchingEmptyAndForeachedExpr($stmt, $nextStmt->expr)) { + return null; + } + + unset($stmtsAware->stmts[$lastKey - 1]); + return $stmtsAware; + } } diff --git a/rules/DeadCode/UselessIfCondBeforeForeachDetector.php b/rules/DeadCode/UselessIfCondBeforeForeachDetector.php index e7d0fdf7fa8..9cf16381f5d 100644 --- a/rules/DeadCode/UselessIfCondBeforeForeachDetector.php +++ b/rules/DeadCode/UselessIfCondBeforeForeachDetector.php @@ -14,6 +14,7 @@ use PhpParser\Node\Expr\Variable; use PhpParser\Node\Param; use PhpParser\Node\Stmt\If_; +use PhpParser\Node\Stmt\Return_; use PHPStan\Analyser\Scope; use Rector\Core\NodeAnalyzer\ParamAnalyzer; use Rector\Core\PhpParser\Comparing\NodeComparator; @@ -30,42 +31,52 @@ public function __construct( /** * Matches: - * !empty($values) + * empty($values) */ - public function isMatchingNotEmpty(If_ $if, Expr $foreachExpr, Scope $scope): bool + public function isMatchingEmptyAndForeachedExpr(If_ $if, Expr $foreachExpr): bool { - $cond = $if->cond; - if (! $cond instanceof BooleanNot) { - return false; - } - - if (! $cond->expr instanceof Empty_) { + if (! $if->cond instanceof Empty_) { return false; } /** @var Empty_ $empty */ - $empty = $cond->expr; + $empty = $if->cond; if (! $this->nodeComparator->areNodesEqual($empty->expr, $foreachExpr)) { return false; } - // is array though? - $arrayType = $scope->getType($empty->expr); - if (! $arrayType->isArray()->yes()) { + if ($if->stmts === []) { + return true; + } + + if (count($if->stmts) !== 1) { return false; } - $previousParam = $this->fromPreviousParam($foreachExpr); - if (! $previousParam instanceof Param) { - return true; + $stmt = $if->stmts[0]; + return $stmt instanceof Return_ && ! $stmt->expr instanceof Expr; + } + + /** + * Matches: + * !empty($values) + */ + public function isMatchingNotEmpty(If_ $if, Expr $foreachExpr, Scope $scope): bool + { + $cond = $if->cond; + if (! $cond instanceof BooleanNot) { + return false; } - if ($this->paramAnalyzer->isNullable($previousParam)) { + if (! $cond->expr instanceof Empty_) { return false; } - return ! $this->paramAnalyzer->hasDefaultNull($previousParam); + /** @var Empty_ $empty */ + $empty = $cond->expr; + + return $this->areCondExprAndForeachExprSame($empty, $foreachExpr, $scope); } /** @@ -128,4 +139,29 @@ private function isEmptyArray(Expr $expr): bool return $expr->items === []; } + + private function areCondExprAndForeachExprSame(Empty_ $empty, Expr $foreachExpr, Scope $scope): bool + { + if (! $this->nodeComparator->areNodesEqual($empty->expr, $foreachExpr)) { + return false; + } + + // is array though? + $arrayType = $scope->getType($empty->expr); + if (! $arrayType->isArray() + ->yes()) { + return false; + } + + $previousParam = $this->fromPreviousParam($foreachExpr); + if (! $previousParam instanceof Param) { + return true; + } + + if ($this->paramAnalyzer->isNullable($previousParam)) { + return false; + } + + return ! $this->paramAnalyzer->hasDefaultNull($previousParam); + } }