[CodingStyle] Improve NewlineBeforeNewAssignSetRector

packages/CodingStyle/src/Rector/ClassMethod/NewlineBeforeNewAssignSetRector.php

@@ -4,9 +4,12 @@
 
 use PhpParser\Node;
 use PhpParser\Node\Expr\Assign;
+use PhpParser\Node\Expr\Closure;
 use PhpParser\Node\Expr\MethodCall;
 use PhpParser\Node\Expr\Variable;
+use PhpParser\Node\Stmt\ClassMethod;
 use PhpParser\Node\Stmt\Expression;
+use PhpParser\Node\Stmt\Function_;
 use PhpParser\Node\Stmt\Nop;
 use Rector\Rector\AbstractRector;
 use Rector\RectorDefinition\CodeSample;
@@ -17,6 +20,16 @@
  */
 final class NewlineBeforeNewAssignSetRector extends AbstractRector
 {
+    /**
+     * @var string|null
+     */
+    private $previousStmtVariableName;
+
+    /**
+     * @var string|null
+     */
+    private $previousPreviousStmtVariableName;
+
     public function getDefinition(): RectorDefinition
     {
         return new RectorDefinition('Add extra space before new assign set', [
@@ -56,15 +69,15 @@ public function run()
      */
     public function getNodeTypes(): array
     {
-        return [Node\Stmt\ClassMethod::class, Node\Stmt\Function_::class, Node\Expr\Closure::class];
+        return [ClassMethod::class, Function_::class, Closure::class];
     }
 
     /**
-     * @param Node\Stmt\ClassMethod|Node\Stmt\Function_|Node\Expr\Closure $node
+     * @param ClassMethod|Function_|Closure $node
      */
     public function refactor(Node $node): ?Node
     {
-        $previousStmtVariableName = null;
+        $this->reset();
 
         if ($node->stmts === null) {
             return null;
@@ -78,32 +91,94 @@ public function refactor(Node $node): ?Node
             }
 
             if ($stmt instanceof Assign || $stmt instanceof MethodCall) {
-                if ($stmt->var instanceof Variable) {
-                    $currentStmtVariableName = $this->getName($stmt->var);
+                if ($this->shouldSkipLeftVariable($stmt)) {
+                    continue;
                 }
+
+                $currentStmtVariableName = $this->getName($stmt->var);
             }
 
-            if ($this->isNewVariableThanBefore($previousStmtVariableName, $currentStmtVariableName)) {
+            if ($this->shouldAddEmptyLine($currentStmtVariableName, $node, $key)) {
                 // insert newline before
                 array_splice($node->stmts, $key, 0, [new Nop()]);
             }
 
-            $previousStmtVariableName = $currentStmtVariableName;
+            $this->previousPreviousStmtVariableName = $this->previousStmtVariableName;
+            $this->previousStmtVariableName = $currentStmtVariableName;
         }
 
         return $node;
     }
 
-    private function isNewVariableThanBefore(?string $previousStmtVariableName, ?string $currentStmtVariableName): bool
+    private function reset(): void
+    {
+        $this->previousStmtVariableName = null;
+        $this->previousPreviousStmtVariableName = null;
+    }
+
+    /**
+     * @param ClassMethod|Function_|Closure $node
+     */
+    private function shouldAddEmptyLine(?string $currentStmtVariableName, Node $node, int $key): bool
+    {
+        if (! $this->isNewVariableThanBefore($currentStmtVariableName)) {
+            return false;
+        }
+
+        // this is already empty line before
+        if ($this->isPreceededByEmptyLine($node, $key)) {
+            return false;
+        }
+
+        return true;
+    }
+
+    private function isNewVariableThanBefore(?string $currentStmtVariableName): bool
     {
-        if ($previousStmtVariableName === null) {
+        if ($this->previousPreviousStmtVariableName === null) {
+            return false;
+        }
+
+        if ($this->previousStmtVariableName === null) {
             return false;
         }
 
         if ($currentStmtVariableName === null) {
             return false;
         }
 
-        return $previousStmtVariableName !== $currentStmtVariableName;
+        if ($this->previousStmtVariableName !== $this->previousPreviousStmtVariableName) {
+            return false;
+        }
+
+        return $this->previousStmtVariableName !== $currentStmtVariableName;
+    }
+
+    /**
+     * @param ClassMethod|Function_|Closure $node
+     */
+    private function isPreceededByEmptyLine(Node $node, int $key): bool
+    {
+        if ($node->stmts === null) {
+            return false;
+        }
+
+        $previousNode = $node->stmts[$key - 1];
+        $currentNode = $node->stmts[$key];
+
+        return abs($currentNode->getLine() - $previousNode->getLine()) >= 2;
+    }
+
+    /**
+     * @param Assign|MethodCall $node
+     */
+    private function shouldSkipLeftVariable(Node $node): bool
+    {
+        if (! $node->var instanceof Variable) {
+            return true;
+        }
+
+        // local method call
+        return $this->isName($node->var, 'this');
     }
 }

packages/CodingStyle/tests/Rector/ClassMethod/NewlineBeforeNewAssignSetRector/Fixture/skip_already_nop.php.inc

@@ -0,0 +1,13 @@
+<?php
+
+namespace Rector\CodingStyle\Tests\Rector\ClassMethod\NewlineBeforeNewAssignSetRector\Fixture;
+
+final class SkipAlreadyNop
+{
+    public function run()
+    {
+        $matches = Strings::matchAll($input, '!([A-Z][A-Z0-9]*(?=$|[A-Z][a-z0-9])|[A-Za-z][a-z0-9]+)!');
+
+        $parts = [];
+    }
+}

packages/CodingStyle/tests/Rector/ClassMethod/NewlineBeforeNewAssignSetRector/Fixture/skip_false.php.inc

@@ -0,0 +1,13 @@
+<?php
+
+namespace Rector\CodingStyle\Tests\Rector\ClassMethod\NewlineBeforeNewAssignSetRector\Fixture;
+
+final class SkipFalse
+{
+    public function run()
+    {
+        $changedContent = $this->someMethod();
+        $this->someMethod();
+
+    }
+}

packages/CodingStyle/tests/Rector/ClassMethod/NewlineBeforeNewAssignSetRector/Fixture/skip_just_one.php.inc

@@ -0,0 +1,12 @@
+<?php
+
+namespace Rector\CodingStyle\Tests\Rector\ClassMethod\NewlineBeforeNewAssignSetRector\Fixture;
+
+final class SkipJustOne
+{
+    public function run()
+    {
+        $matches = 1;
+        $anotherMatches = 5;
+    }
+}

packages/CodingStyle/tests/Rector/ClassMethod/NewlineBeforeNewAssignSetRector/NewlineBeforeNewAssignSetRectorTest.php

@@ -9,7 +9,12 @@ final class NewlineBeforeNewAssignSetRectorTest extends AbstractRectorTestCase
 {
     public function test(): void
     {
-        $this->doTestFiles([__DIR__ . '/Fixture/fixture.php.inc']);
+        $this->doTestFiles([
+            __DIR__ . '/Fixture/fixture.php.inc',
+            __DIR__ . '/Fixture/skip_false.php.inc',
+            __DIR__ . '/Fixture/skip_already_nop.php.inc',
+            __DIR__ . '/Fixture/skip_just_one.php.inc',
+        ]);
     }
 
     protected function getRectorClass(): string

packages/NodeTypeResolver/src/NodeScopeAndMetadataDecorator.php

@@ -104,6 +104,7 @@ public function decorateNodesFromFile(array $nodes, string $filePath): array
         $nodeTraverser->addVisitor($this->parentAndNextNodeVisitor);
         $nodeTraverser->addVisitor($this->classAndMethodNodeVisitor);
         $nodeTraverser->addVisitor($this->namespaceNodeVisitor);
+
         $nodes = $nodeTraverser->traverse($nodes);
 
         // this split is needed, so nodes have names, classes and namespaces

rector.yaml

@@ -17,4 +17,5 @@ parameters:
     php_version_features: '7.1'
 
 services:
+#    Rector\CodingStyle\Rector\ClassMethod\NewlineBeforeNewAssignSetRector: ~
 #    Rector\CodingStyle\Rector\Namespace_\ImportFullyQualifiedNamesRector: ~