Replacing class method visibility doesn't work without previous node' attributes in format-preserving pretty printing

[maks-rafalko]

This finding is related to #1115

(It's a bug-report with test reproducer, not the fix or feature implementation)

at Infection, we do a lot of nodes replacements during the Mutation Testing process.

For example (see tests in this PR), we replace public function ... with protected function ... class method visibility modifier.

However, unlike with other replacements, this one for some reason requires $replacedNode->getAttributes() to be passed when the new Node is created:

Replacement:

$replacedNode = $stmts[0]->stmts[0]->stmts[0];
$stmts[0]->stmts[0]->stmts[0] = new \PhpParser\Node\Stmt\ClassMethod(
    $node->name,
    [
        'flags' => ($node->flags & ~\PhpParser\Modifiers::PUBLIC) | \PhpParser\Modifiers::PROTECTED,
        'byRef' => $node->returnsByRef(),
        'params' => $node->getParams(),
        'returnType' => $node->getReturnType(),
        'stmts' => $node->getStmts(),
        'attrGroups' => $node->getAttrGroups(),
    ],
    $replacedNode->getAttributes(), // <---- pretty-printing does not work properly without replaced node's attributes
);

Please note how we pass $replacedNode->getAttributes() to the the created ClassMethod node. If I omit it, then we have a completely broken formatting:

--- Expected
+++ Actual
@@ @@

 class SourceClass
 {
-    /**
-     * @template T
-     * @param array<T> $values
-     * @return list<T>
-     */
     protected function makeAList(array $values): array
     {
         // some comment
-
         $strings = ['1'];
-
         $ints = array_map(function ($value): int {
             return (int) $value;
         }, $strings);
-
         $nonEmptyArray = ['1'];
-
         $nonEmptyArrayFromMethod = $this->returnNonEmptyArray();
-
         $inlineNonEmpty = ['1'];
-
         return array_values($values);
     }
 }

But when I pass $previousNode->getAttributes(), the diff is clear and correct:

--- Original
+++ New
@@ @@
      * @param array<T> $values
      * @return list<T>
      */
-    public function makeAList(array $values): array
+    protected function makeAList(array $values): array
     {

---

What is not clear:

• why do we need to pass $replacedNode->getAttributes() while in other cases (like here) we don't need to pass it?

[nikic]

This is intended behavior. Formatting preservation requires the origNode attribute to be set. This is intended to be achieved through in-place modification, though creating a new one and copying origNode works as well.