diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector/Fixture/assign_ops.php.inc b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector/Fixture/assign_ops.php.inc deleted file mode 100644 index a4723b5de5e..00000000000 --- a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector/Fixture/assign_ops.php.inc +++ /dev/null @@ -1,47 +0,0 @@ - ------ - diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector/FixtureSkipConcat/skip_assign_ops.php.inc b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector/Fixture/skip_assign_ops.php.inc similarity index 100% rename from rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector/FixtureSkipConcat/skip_assign_ops.php.inc rename to rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector/Fixture/skip_assign_ops.php.inc diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector/Fixture/skip_commented_code_between.php.inc b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector/Fixture/skip_commented_code_between.php.inc new file mode 100644 index 00000000000..0d8c2d53dae --- /dev/null +++ b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector/Fixture/skip_commented_code_between.php.inc @@ -0,0 +1,18 @@ +doTestFile($filePath); - } - - public static function provideData(): Iterator - { - return self::yieldFilesFromDirectory(__DIR__ . '/FixtureSkipConcat'); - } - - public function provideConfigFilePath(): string - { - return __DIR__ . '/config/skip_concat.php'; - } -} diff --git a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector/config/skip_concat.php b/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector/config/skip_concat.php deleted file mode 100644 index b815f89a85f..00000000000 --- a/rules-tests/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector/config/skip_concat.php +++ /dev/null @@ -1,13 +0,0 @@ -ruleWithConfiguration(SimplifyUselessVariableRector::class, [ - SimplifyUselessVariableRector::ONLY_DIRECT_ASSIGN => true, - ]); -}; diff --git a/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector.php b/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector.php index ee5d6b8c685..c000a4fc6df 100644 --- a/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector.php +++ b/rules/CodeQuality/Rector/FunctionLike/SimplifyUselessVariableRector.php @@ -7,56 +7,36 @@ use PhpParser\Comment\Doc; use PhpParser\Node; use PhpParser\Node\Expr\Assign; -use PhpParser\Node\Expr\AssignOp; -use PhpParser\Node\Expr\Ternary; use PhpParser\Node\Expr\Variable; use PhpParser\Node\Stmt; use PhpParser\Node\Stmt\Expression; use PhpParser\Node\Stmt\Return_; use PHPStan\Type\MixedType; use Rector\BetterPhpDocParser\PhpDocInfo\PhpDocInfoFactory; -use Rector\Contract\Rector\ConfigurableRectorInterface; use Rector\NodeAnalyzer\CallAnalyzer; use Rector\NodeAnalyzer\VariableAnalyzer; use Rector\NodeTypeResolver\Node\AttributeKey; use Rector\PhpParser\Enum\NodeGroup; -use Rector\PhpParser\Node\AssignAndBinaryMap; use Rector\Rector\AbstractRector; -use Symplify\RuleDocGenerator\ValueObject\CodeSample\ConfiguredCodeSample; +use Symplify\RuleDocGenerator\ValueObject\CodeSample\CodeSample; use Symplify\RuleDocGenerator\ValueObject\RuleDefinition; /** * @see \Rector\Tests\CodeQuality\Rector\FunctionLike\SimplifyUselessVariableRector\SimplifyUselessVariableRectorTest */ -final class SimplifyUselessVariableRector extends AbstractRector implements ConfigurableRectorInterface +final class SimplifyUselessVariableRector extends AbstractRector { - /** - * @api - */ - public const string ONLY_DIRECT_ASSIGN = 'only_direct_assign'; - - private bool $onlyDirectAssign = false; - public function __construct( - private readonly AssignAndBinaryMap $assignAndBinaryMap, private readonly VariableAnalyzer $variableAnalyzer, private readonly CallAnalyzer $callAnalyzer, private readonly PhpDocInfoFactory $phpDocInfoFactory ) { } - /** - * @param array $configuration - */ - public function configure(array $configuration): void - { - $this->onlyDirectAssign = $configuration[self::ONLY_DIRECT_ASSIGN] ?? false; - } - public function getRuleDefinition(): RuleDefinition { return new RuleDefinition('Remove useless variable assigns', [ - new ConfiguredCodeSample( + new CodeSample( <<<'CODE_SAMPLE' function () { $a = true; @@ -69,33 +49,6 @@ function () { return true; }; CODE_SAMPLE - , - // default - [ - self::ONLY_DIRECT_ASSIGN => true, - ] - ), - new ConfiguredCodeSample( - <<<'CODE_SAMPLE' -function () { - $a = 'Hello, '; - $a .= 'World!'; - - return $a; -}; -CODE_SAMPLE - , - <<<'CODE_SAMPLE' -function () { - $a = 'Hello, '; - - return $a . 'World!'; -}; -CODE_SAMPLE - , - [ - self::ONLY_DIRECT_ASSIGN => false, - ] ), ]); } @@ -137,42 +90,31 @@ public function refactor(Node $node): ?Node return null; } - if ($this->isReturnWithVarAnnotation($stmt)) { + // the variable might be used in commented-out code between assign and return + if ($this->isVariableMentionedInComment($stmt)) { return null; } - /** @var Expression $previousStmt */ - $assign = $previousStmt->expr; - - return $this->processSimplifyUselessVariable($node, $stmt, $assign, $key); - } + if ($this->isReturnWithVarAnnotation($stmt)) { + return null; + } - return null; - } + if (! $previousStmt instanceof Expression) { + return null; + } - /** - * @param StmtsAware $stmtsAware - * @return StmtsAware|null - */ - private function processSimplifyUselessVariable( - Node $stmtsAware, - Return_ $return, - Assign|AssignOp $assign, - int $key - ): ?Node { - if (! $assign instanceof Assign) { - $binaryClass = $this->assignAndBinaryMap->getAlternative($assign); - if ($binaryClass === null) { + $assign = $previousStmt->expr; + if (! $assign instanceof Assign) { return null; } - $return->expr = new $binaryClass($assign->var, $assign->expr); - } else { - $return->expr = $assign->expr; + $stmt->expr = $assign->expr; + unset($node->stmts[$key - 1]); + + return $node; } - unset($stmtsAware->stmts[$key - 1]); - return $stmtsAware; + return null; } private function shouldSkipStmt(Return_ $return, Stmt $previousStmt): bool @@ -192,15 +134,7 @@ private function shouldSkipStmt(Return_ $return, Stmt $previousStmt): bool // is variable part of single assign $previousNode = $previousStmt->expr; - if (! $previousNode instanceof AssignOp && ! $previousNode instanceof Assign) { - return true; - } - - if ($this->onlyDirectAssign && $previousNode instanceof AssignOp) { - return true; - } - - if ($previousNode instanceof AssignOp && $previousNode->expr instanceof Ternary) { + if (! $previousNode instanceof Assign) { return true; } @@ -223,6 +157,25 @@ private function shouldSkipStmt(Return_ $return, Stmt $previousStmt): bool return $this->variableAnalyzer->isUsedByReference($variable); } + private function isVariableMentionedInComment(Return_ $return): bool + { + $comments = $return->getComments(); + if ($comments === []) { + return false; + } + + if (! $return->expr instanceof Variable) { + return false; + } + + $variableName = $return->expr->name; + if (! is_string($variableName)) { + return false; + } + + return array_any($comments, fn ($comment): bool => str_contains($comment->getText(), '$' . $variableName)); + } + private function hasSomeComment(Stmt $stmt): bool { if ($stmt->getComments() !== []) { diff --git a/src/PhpParser/Node/AssignAndBinaryMap.php b/src/PhpParser/Node/AssignAndBinaryMap.php index af424f5aea1..4bcceb104ec 100644 --- a/src/PhpParser/Node/AssignAndBinaryMap.php +++ b/src/PhpParser/Node/AssignAndBinaryMap.php @@ -4,7 +4,6 @@ namespace Rector\PhpParser\Node; -use PhpParser\Node; use PhpParser\Node\Expr\AssignOp; use PhpParser\Node\Expr\AssignOp\BitwiseAnd as AssignBitwiseAnd; use PhpParser\Node\Expr\AssignOp\BitwiseOr as AssignBitwiseOr; @@ -87,21 +86,11 @@ public function __construct() } /** - * @return class-string|null + * @return class-string|null */ - public function getAlternative(Node $node): ?string + public function getAlternative(BinaryOp $binaryOp): ?string { - $nodeClass = $node::class; - - if ($node instanceof AssignOp) { - return self::ASSIGN_OP_TO_BINARY_OP_CLASSES[$nodeClass] ?? null; - } - - if ($node instanceof BinaryOp) { - return $this->binaryOpToAssignClasses[$nodeClass] ?? null; - } - - return null; + return $this->binaryOpToAssignClasses[$binaryOp::class] ?? null; } /**