From be428e32070097a11d77df9bd3fdebfa5baae71d Mon Sep 17 00:00:00 2001
From: Joshua Spence <josh@joshuaspence.com>
Date: Thu, 25 Sep 2014 07:38:37 +1000
Subject: [PATCH] Add a linter rule for array separators

Summary:
Adds a rule to `ArcanistXHPASTLinter` which ensures that:

# Single-lined arrays //do not// contain an unnecessary trailing comma separator after the final array value.
# Multi-lined arrays //do// contain a trailing comma seperator after the final value value.

Depends on D10534.

Test Plan: Wrote and executed unit tests.

Reviewers: #blessed_reviewers, epriestley

Reviewed By: #blessed_reviewers, epriestley

Subscribers: epriestley, Korvin

Differential Revision: https://secure.phabricator.com/D10535
---
 src/lint/linter/ArcanistXHPASTLinter.php      | 48 ++++++++++++++++++-
 .../__tests__/xhpast/array-comma.lint-test    | 30 ++++++++++++
 2 files changed, 77 insertions(+), 1 deletion(-)
 create mode 100644 src/lint/linter/__tests__/xhpast/array-comma.lint-test

diff --git a/src/lint/linter/ArcanistXHPASTLinter.php b/src/lint/linter/ArcanistXHPASTLinter.php
index b005ea48..5d443745 100644
--- a/src/lint/linter/ArcanistXHPASTLinter.php
+++ b/src/lint/linter/ArcanistXHPASTLinter.php
@@ -47,6 +47,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
   const LINT_PHP_COMPATIBILITY         = 45;
   const LINT_LANGUAGE_CONSTRUCT_PAREN  = 46;
   const LINT_EMPTY_STATEMENT           = 47;
+  const LINT_ARRAY_SEPARATOR           = 48;
 
   private $naminghook;
   private $switchhook;
@@ -105,6 +106,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       self::LINT_PHP_COMPATIBILITY         => 'PHP Compatibility',
       self::LINT_LANGUAGE_CONSTRUCT_PAREN  => 'Language Construct Parentheses',
       self::LINT_EMPTY_STATEMENT           => 'Empty Block Statement',
+      self::LINT_ARRAY_SEPARATOR           => 'Array Separator',
     );
   }
 
@@ -144,6 +146,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       self::LINT_CONCATENATION_OPERATOR    => $warning,
       self::LINT_LANGUAGE_CONSTRUCT_PAREN  => $warning,
       self::LINT_EMPTY_STATEMENT           => $advice,
+      self::LINT_ARRAY_SEPARATOR           => $advice,
     );
   }
 
@@ -193,7 +196,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
 
   public function getVersion() {
     // The version number should be incremented whenever a new rule is added.
-    return '8';
+    return '9';
   }
 
   protected function resolveFuture($path, Future $future) {
@@ -263,6 +266,7 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
       'lintPHPCompatibility' => self::LINT_PHP_COMPATIBILITY,
       'lintLanguageConstructParentheses' => self::LINT_LANGUAGE_CONSTRUCT_PAREN,
       'lintEmptyBlockStatements' => self::LINT_EMPTY_STATEMENT,
+      'lintArraySeparator' => self::LINT_ARRAY_SEPARATOR,
     );
 
     foreach ($method_codes as $method => $codes) {
@@ -2714,6 +2718,48 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
     }
   }
 
+  protected function lintArraySeparator(XHPASTNode $root) {
+    $arrays = $root->selectDescendantsOfType('n_ARRAY_LITERAL');
+
+    foreach ($arrays as $array) {
+      $commas = $array->selectTokensOfType(',');
+      $values = $array->selectDescendantsOfType('n_ARRAY_VALUE');
+
+      if ($values->count() == 0) {
+        // There is no need to check an empty array.
+        continue;
+      }
+
+      // This is a little messy... we want to do `last($values)` but
+      // `$values` is an `AASTNodeList`.
+      $last = null;
+      foreach ($values as $value) {
+        $last = $value;
+      }
+
+      $multiline = $array->getLineNumber() != $array->getEndLineNumber();
+
+      if ($multiline && count($commas) != count($values)) {
+        $this->raiseLintAtNode(
+          $last,
+          self::LINT_ARRAY_SEPARATOR,
+          pht('Multi-lined arrays should have trailing commas.'),
+          $last->getConcreteString().',');
+      } else if (!$multiline && count($commas) == count($values)) {
+        $last_comma = null;
+        foreach ($commas as $comma) {
+          $last_comma = $comma;
+        }
+
+        $this->raiseLintAtToken(
+          $last_comma,
+          self::LINT_ARRAY_SEPARATOR,
+          pht('Single lined arrays should not have a trailing comma.'),
+          '');
+      }
+    }
+  }
+
   public function getSuperGlobalNames() {
     return array(
       '$GLOBALS',
diff --git a/src/lint/linter/__tests__/xhpast/array-comma.lint-test b/src/lint/linter/__tests__/xhpast/array-comma.lint-test
new file mode 100644
index 00000000..26096d29
--- /dev/null
+++ b/src/lint/linter/__tests__/xhpast/array-comma.lint-test
@@ -0,0 +1,30 @@
+<?php
+array(1, 2, 3);
+array(1, 2, 3,);
+array(
+  1,
+  2,
+  3,
+);
+array(
+  1,
+  2,
+  3
+);
+~~~~~~~~~~
+advice:3:14
+advice:12:3
+~~~~~~~~~~
+<?php
+array(1, 2, 3);
+array(1, 2, 3);
+array(
+  1,
+  2,
+  3,
+);
+array(
+  1,
+  2,
+  3,
+);