Format-preserving bug for multipes NodeVisitors

[TomasVotruba]

With format-perserving printer [#344] I came across weird bug.

I have 2 visitor modifying the code:

• InjectAnnotationToConstructorNodeVisitor
• and NamedServicesToConstructorNodeVistitor

This is error output from Travis Report:

• I make property private - doesn't work 👎
• I add argument to constructor - works 👍
• I add assignment in constuctor method - 👎
• declare (strict_types=1);\n is added instead of it

-    private $property;\n
+    public $property;\n
     /**\n
      * @var DateTimeInterface\n
      */\n
-    private $otherProperty;\n
+    public $otherProperty;\n
     public function __construct(stdClass $property, DateTimeInterface $otherProperty)\n
     {\n
-        $this->property = $property;\n
-        $this->otherProperty = $otherProperty;\n
+        declare (strict_types=1);\n
+        declare (strict_types=1);\n
     }\n
 }\n

return $this->codeStyledPrinter->printToString($newStmts, $oldStmts, $oldTokens);

This would be understandable from some point, but...

But when I switch first 2 arguments, this NodeVisitor works but the other doesn't.

return $this->codeStyledPrinter->printToString($oldStmts, $newStmts, $oldTokens);

In this case, afterTraverse() isn't taken into account.

 class ClassWithNamedService extends Controller\n
 {\n
-    /**\n
-     * @var stdClass\n
-     */\n
-    private $stdClass;\n
-    public function __construct(stdClass $stdClass)\n
-    {\n
-        $this->stdClass = $stdClass;\n
-    }\n
     public function render()\n
     {\n
         $this->stdClass->render();\n
     }\n
 }\n

[TomasVotruba]

I used this as hotfix for time being: rectorphp/rector@9ab6c84

[nikic]

Where is the CloningVisitor being applied? Unfortunately GH can only search master.

[TomasVotruba]

I see.

I use own ApIication for testing, because I don't know how to apply changes on dry-run (without changing actuall content): https://github.com/TomasVotruba/Rector/blob/9ab6c8462988fefe7597808dde214ce0eb8fe4cc/src/Testing/Application/FileReconstructor.php#L54-L67

Here's the printer: https://github.com/TomasVotruba/Rector/blob/new-rector-named-services/src/Printer/CodeStyledPrinter.php

CloningVisitor is added to Dependecy Injection container and loaded to NodeTraverser.

[TomasVotruba]

Does CloningVisitor order matter?

I tried using it fast and using it last and still there are some issues.

[nikic]

@TomasVotruba It should definitely run first. To be on the safe side (especially if enterNode visitors are used), I'd also run it as an independent pass instead of interleaving it with other visitors.

[TomasVotruba]

I tried your solution and it works like a charm 🍾

$cloningNodeTraverser = new PhpParser\NodeTraverser();
$cloningNodeTraverser->addVisitor(new PhpParser\NodeVisitor\CloningVisitor);

$oldStmts = $this->parser->parse($fileContent);
$oldTokens = $this->lexer->getTokens();
$newStmts = $cloningNodeTraverser->traverse($oldStmts);
        
$newStmts = $this->nodeTraverser->traverse($newStmts);

$prettyPrinter->printFormatPreserving($newStmts, $oldStmts, $oldTokens);

Thank you!