FPPP: Add heuristic for multi-line lists

nikic · Dec 26, 2017 · 1c7fd31 · 1c7fd31
1 parent fb81755
commit 1c7fd31
Show file tree

Hide file tree

Showing 4 changed files with 96 additions and 13 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,17 +1,20 @@
 Version 4.0.0-dev
 -----------------
 
-Nothing yet.
+### Added
+
+* In formatting-preserving pretty printer:
+  * Improved formatting of elements inserted into multi-line arrays.
 
 Version 4.0.0-alpha3 (2017-12-26)
 ---------------------------------
 
 ### Fixed
 
 * In the formatting-preserving pretty printer:
- * Fixed comment indentation.
- * Fixed handling of inline HTML in the fallback case.
- * Fixed insertion into list nodes that require creation of a code block.
+  * Fixed comment indentation.
+  * Fixed handling of inline HTML in the fallback case.
+  * Fixed insertion into list nodes that require creation of a code block.
 
 ### Added
 

diff --git a/lib/PhpParser/PrettyPrinterAbstract.php b/lib/PhpParser/PrettyPrinterAbstract.php
@@ -701,7 +701,13 @@ protected function pArray(
         $beforeFirstKeepOrReplace = true;
         $delayedAdd = [];
 
-        if ($subNodeName === 'stmts' && count($origNodes) === 1 && count($nodes) !== 1) {
+        $insertNewline = false;
+        if ($insertStr === "\n") {
+            $insertStr = '';
+            $insertNewline = true;
+        }
+
+        if ($subNodeName === 'stmts' && \count($origNodes) === 1 && \count($nodes) !== 1) {
             $startPos = $origNodes[0]->getStartTokenPos();
             $endPos = $origNodes[0]->getEndTokenPos();
             \assert($startPos >= 0 && $endPos >= 0);
@@ -769,7 +775,7 @@ protected function pArray(
 
                     /** @var Node $delayedAddNode */
                     foreach ($delayedAdd as $delayedAddNode) {
-                        if ($insertStr === "\n") {
+                        if ($insertNewline) {
                             $delayedAddComments = $delayedAddNode->getComments();
                             if ($delayedAddComments) {
                                 $result .= $this->pComments($delayedAddComments) . $this->nl;
@@ -778,8 +784,8 @@ protected function pArray(
 
                         $this->safeAppend($result, $this->p($delayedAddNode, true));
 
-                        if ($insertStr === "\n") {
-                            $result .= $this->nl;
+                        if ($insertNewline) {
+                            $result .= $insertStr . $this->nl;
                         } else {
                             $result .= $insertStr;
                         }
@@ -804,6 +810,11 @@ protected function pArray(
                     return null;
                 }
 
+                if ($insertStr === ', ' && $this->isMultiline($origNodes)) {
+                    $insertStr = ',';
+                    $insertNewline = true;
+                }
+
                 if ($beforeFirstKeepOrReplace) {
                     // Will be inserted at the next "replace" or "keep" element
                     $delayedAdd[] = $arrItem;
@@ -816,12 +827,12 @@ protected function pArray(
                 $origIndentLevel = $this->indentLevel;
                 $this->setIndentLevel($this->origTokens->getIndentationBefore($itemStartPos) + $indentAdjustment);
 
-                if ($insertStr === "\n") {
+                if ($insertNewline) {
                     $comments = $arrItem->getComments();
                     if ($comments) {
                         $result .= $this->nl . $this->pComments($comments);
                     }
-                    $result .= $this->nl;
+                    $result .= $insertStr . $this->nl;
                 } else {
                     $result .= $insertStr;
                 }
@@ -1006,6 +1017,40 @@ protected function pModifiers(int $modifiers) {
              . ($modifiers & Stmt\Class_::MODIFIER_FINAL     ? 'final '     : '');
     }
 
+    /**
+     * Determine whether a list of nodes uses multiline formatting.
+     *
+     * @param (Node|null)[] $nodes Node list
+     *
+     * @return bool Whether multiline formatting is used
+     */
+    protected function isMultiline(array $nodes) : bool {
+        if (\count($nodes) < 2) {
+            return false;
+        }
+
+        $pos = -1;
+        foreach ($nodes as $node) {
+            if (null === $node) {
+                continue;
+            }
+
+            $endPos = $node->getEndTokenPos() + 1;
+            if ($pos >= 0) {
+                $text = $this->origTokens->getTokenCode($pos, $endPos, 0);
+                if (false === strpos($text, "\n")) {
+                    // We require that a newline is present between *every* item. If the formatting
+                    // is inconsistent, with only some items having newlines, we don't consider it
+                    // as multiline
+                    return false;
+                }
+            }
+            $pos = $endPos;
+        }
+
+        return true;
+    }
+
     /**
      * Lazily initializes label char map.
      *

diff --git a/test/code/formatPreservation/listInsertion.test b/test/code/formatPreservation/listInsertion.test
@@ -276,4 +276,38 @@ function test()
 {
     $a;
     $b;
-}
+}
+-----
+<?php
+$array = [
+    1,
+    2,
+    3,
+];
+-----
+array_unshift($stmts[0]->expr->expr->items, new Expr\ArrayItem(new Scalar\LNumber(42)));
+$stmts[0]->expr->expr->items[] = new Expr\ArrayItem(new Scalar\LNumber(24));
+-----
+<?php
+$array = [
+    42,
+    1,
+    2,
+    3,
+    24,
+];
+-----
+<?php
+$array = [
+    1, 2,
+    3,
+];
+-----
+// TODO Replace [3] workaround
+$stmts[0]->expr->expr->items[3] = new Expr\ArrayItem(new Scalar\LNumber(24));
+-----
+<?php
+$array = [
+    1, 2,
+    3, 24,
+];
diff --git a/test/code/formatPreservation/listRemoval.test b/test/code/formatPreservation/listRemoval.test
@@ -31,10 +31,11 @@ function foo(
 array_pop($stmts[0]->params);
 $stmts[0]->params[] = new Node\Param(new Expr\Variable('x'));
 $stmts[0]->params[] = new Node\Param(new Expr\Variable('y'));
-/* TODO The insertion here should try to to honor the style */
 -----
 <?php
 function foo(
     $a,
-    $b, $x, $y
+    $b,
+    $x,
+    $y
 ) {}