From e29f8fafabe43d771f54a428af4d8ab340c177c9 Mon Sep 17 00:00:00 2001 From: TomasVotruba Date: Thu, 27 Feb 2020 23:35:25 +0100 Subject: [PATCH] [DeadCode] Add RemoveDuplicatedIfReturnRector --- config/set/dead-code/dead-code.yaml | 1 + docs/AllRectorsOverview.md | 29 ++- .../RemoveDuplicatedIfReturnRector.php | 200 ++++++++++++++++++ .../Fixture/fixture.php.inc | 39 ++++ .../Fixture/skip_modified_value.php.inc | 19 ++ .../Fixture/skip_this.php.inc | 19 ++ .../Fixture/skip_with_differnt_value.php.inc | 19 ++ .../RemoveDuplicatedIfReturnRectorTest.php | 30 +++ .../Node/Manipulator/IfManipulator.php | 13 ++ .../Printer/BetterStandardPrinter.php | 2 +- 10 files changed, 369 insertions(+), 2 deletions(-) create mode 100644 rules/dead-code/src/Rector/FunctionLike/RemoveDuplicatedIfReturnRector.php create mode 100644 rules/dead-code/tests/Rector/FunctionLike/RemoveDuplicatedIfReturnRector/Fixture/fixture.php.inc create mode 100644 rules/dead-code/tests/Rector/FunctionLike/RemoveDuplicatedIfReturnRector/Fixture/skip_modified_value.php.inc create mode 100644 rules/dead-code/tests/Rector/FunctionLike/RemoveDuplicatedIfReturnRector/Fixture/skip_this.php.inc create mode 100644 rules/dead-code/tests/Rector/FunctionLike/RemoveDuplicatedIfReturnRector/Fixture/skip_with_differnt_value.php.inc create mode 100644 rules/dead-code/tests/Rector/FunctionLike/RemoveDuplicatedIfReturnRector/RemoveDuplicatedIfReturnRectorTest.php diff --git a/config/set/dead-code/dead-code.yaml b/config/set/dead-code/dead-code.yaml index 9eb4bc247da..9482142212e 100644 --- a/config/set/dead-code/dead-code.yaml +++ b/config/set/dead-code/dead-code.yaml @@ -34,3 +34,4 @@ services: Rector\DeadCode\Rector\TryCatch\RemoveDeadTryCatchRector: null Rector\DeadCode\Rector\ClassConst\RemoveUnusedClassConstantRector: null Rector\DeadCode\Rector\Assign\RemoveUnusedVariableAssignRector: null + Rector\DeadCode\Rector\FunctionLike\RemoveDuplicatedIfReturnRector: null diff --git a/docs/AllRectorsOverview.md b/docs/AllRectorsOverview.md index b79a7b7ab86..63f43233cd9 100644 --- a/docs/AllRectorsOverview.md +++ b/docs/AllRectorsOverview.md @@ -1,4 +1,4 @@ -# All 463 Rectors Overview +# All 464 Rectors Overview - [Projects](#projects) - [General](#general) @@ -2791,6 +2791,33 @@ Remove duplicated key in defined arrays.
+### `RemoveDuplicatedIfReturnRector` + +- class: [`Rector\DeadCode\Rector\FunctionLike\RemoveDuplicatedIfReturnRector`](/../master/rules/dead-code/src/Rector/FunctionLike/RemoveDuplicatedIfReturnRector.php) +- [test fixtures](/../master/rules/dead-code/tests/Rector/FunctionLike/RemoveDuplicatedIfReturnRector/Fixture) + +Remove duplicated if stmt with return in function/method body + +```diff + class SomeClass + { + public function run($value) + { + if ($value) { + return true; + } + + $value2 = 100; +- +- if ($value) { +- return true; +- } + } + } +``` + +
+ ### `RemoveDuplicatedInstanceOfRector` - class: [`Rector\DeadCode\Rector\Instanceof_\RemoveDuplicatedInstanceOfRector`](/../master/rules/dead-code/src/Rector/Instanceof_/RemoveDuplicatedInstanceOfRector.php) diff --git a/rules/dead-code/src/Rector/FunctionLike/RemoveDuplicatedIfReturnRector.php b/rules/dead-code/src/Rector/FunctionLike/RemoveDuplicatedIfReturnRector.php new file mode 100644 index 00000000000..a46e83bfa5c --- /dev/null +++ b/rules/dead-code/src/Rector/FunctionLike/RemoveDuplicatedIfReturnRector.php @@ -0,0 +1,200 @@ +ifManipulator = $ifManipulator; + } + + public function getDefinition(): RectorDefinition + { + return new RectorDefinition('Remove duplicated if stmt with return in function/method body', [ + new CodeSample( + <<<'PHP' +class SomeClass +{ + public function run($value) + { + if ($value) { + return true; + } + + $value2 = 100; + + if ($value) { + return true; + } + } +} +PHP +, + <<<'PHP' +class SomeClass +{ + public function run($value) + { + if ($value) { + return true; + } + + $value2 = 100; + } +} +PHP + + ), + ]); + } + + /** + * @return string[] + */ + public function getNodeTypes(): array + { + return [FunctionLike::class]; + } + + /** + * @param FunctionLike $node + */ + public function refactor(Node $node): ?Node + { + $ifWithOnlyReturnsByHash = $this->collectDuplicatedIfWithOnlyReturnByHash($node); + if ($ifWithOnlyReturnsByHash === []) { + return null; + } + + foreach ($ifWithOnlyReturnsByHash as $stmts) { + // keep first one + array_shift($stmts); + + foreach ($stmts as $stmt) { + $this->removeNode($stmt); + } + } + + return $node; + } + + /** + * @return If_[][] + */ + private function collectDuplicatedIfWithOnlyReturnByHash(FunctionLike $functionLike): array + { + $ifWithOnlyReturnsByHash = []; + $modifiedVariableNames = []; + + foreach ((array) $functionLike->getStmts() as $stmt) { + if (! $this->ifManipulator->isIfWithOnlyReturn($stmt)) { + // variable modification + $modifiedVariableNames += $this->collectModifiedVariableNames($stmt); + + continue; + } + + if ($this->containsVariableNames($stmt, $modifiedVariableNames)) { + continue; + } + + /** @var If_ $stmt */ + $hash = $this->printWithoutComments($stmt); + $ifWithOnlyReturnsByHash[$hash][] = $stmt; + } + + return $this->filterOutSingleItemStmts($ifWithOnlyReturnsByHash); + } + + /** + * @param If_[][] $ifWithOnlyReturnsByHash + * @return If_[][] + */ + private function filterOutSingleItemStmts(array $ifWithOnlyReturnsByHash): array + { + return array_filter($ifWithOnlyReturnsByHash, function (array $stmts) { + return count($stmts) >= 2; + }); + } + + /** + * @return string[] + */ + private function collectModifiedVariableNames(Node $node): array + { + $modifiedVariableNames = []; + + $this->traverseNodesWithCallable($node, function (Node $node) use (&$modifiedVariableNames) { + if (! $node instanceof Assign) { + return null; + } + + if (! $node->var instanceof Variable) { + return null; + } + + $variableName = $this->getName($node->var); + if ($variableName === null) { + return null; + } + + $modifiedVariableNames[] = $variableName; + }); + + return $modifiedVariableNames; + } + + /** + * @param string[] $modifiedVariableNames + */ + private function containsVariableNames(Node $node, array $modifiedVariableNames): bool + { + if ($modifiedVariableNames === []) { + return false; + } + + $containsVariableNames = false; + $this->traverseNodesWithCallable($node, function (Node $node) use ( + $modifiedVariableNames, + &$containsVariableNames + ) { + if (! $node instanceof Variable) { + return null; + } + + if (! $this->isNames($node, $modifiedVariableNames)) { + return null; + } + + $containsVariableNames = true; + + return NodeTraverser::STOP_TRAVERSAL; + }); + + return $containsVariableNames; + } +} diff --git a/rules/dead-code/tests/Rector/FunctionLike/RemoveDuplicatedIfReturnRector/Fixture/fixture.php.inc b/rules/dead-code/tests/Rector/FunctionLike/RemoveDuplicatedIfReturnRector/Fixture/fixture.php.inc new file mode 100644 index 00000000000..9380ab37807 --- /dev/null +++ b/rules/dead-code/tests/Rector/FunctionLike/RemoveDuplicatedIfReturnRector/Fixture/fixture.php.inc @@ -0,0 +1,39 @@ + +----- + diff --git a/rules/dead-code/tests/Rector/FunctionLike/RemoveDuplicatedIfReturnRector/Fixture/skip_modified_value.php.inc b/rules/dead-code/tests/Rector/FunctionLike/RemoveDuplicatedIfReturnRector/Fixture/skip_modified_value.php.inc new file mode 100644 index 00000000000..4b72b7b949a --- /dev/null +++ b/rules/dead-code/tests/Rector/FunctionLike/RemoveDuplicatedIfReturnRector/Fixture/skip_modified_value.php.inc @@ -0,0 +1,19 @@ +doTestFile($file); + } + + public function provideData(): Iterator + { + return $this->yieldFilesFromDirectory(__DIR__ . '/Fixture'); + } + + protected function getRectorClass(): string + { + return RemoveDuplicatedIfReturnRector::class; + } +} diff --git a/src/PhpParser/Node/Manipulator/IfManipulator.php b/src/PhpParser/Node/Manipulator/IfManipulator.php index 217d283eeb9..04872350266 100644 --- a/src/PhpParser/Node/Manipulator/IfManipulator.php +++ b/src/PhpParser/Node/Manipulator/IfManipulator.php @@ -263,6 +263,19 @@ final class IfManipulator return $ifs; } + public function isIfWithOnlyReturn(Node $node): bool + { + if (! $node instanceof If_) { + return false; + } + + if (! $this->isIfWithoutElseAndElseIfs($node)) { + return false; + } + + return $this->hasOnlyStmtOfType($node, Return_::class); + } + private function matchComparedAndReturnedNode(NotIdentical $notIdentical, Return_ $returnNode): ?Expr { if ($this->betterStandardPrinter->areNodesEqual( diff --git a/src/PhpParser/Printer/BetterStandardPrinter.php b/src/PhpParser/Printer/BetterStandardPrinter.php index 8ab3d51231a..a2a5ff934cf 100644 --- a/src/PhpParser/Printer/BetterStandardPrinter.php +++ b/src/PhpParser/Printer/BetterStandardPrinter.php @@ -376,7 +376,7 @@ final class BetterStandardPrinter extends Standard $printerNode = Strings::replace($printerNode, '#\/*\*(.*?)\*\/#'); // remove # ... - $printerNode = Strings::replace($printerNode, '#\#(.*?)$#m'); + $printerNode = Strings::replace($printerNode, '#^(\s+)?\#(.*?)$#m'); // remove // ... return Strings::replace($printerNode, '#\/\/(.*?)$#m');