Separate Phutil specific lint rules from XHPAST

Summary: Also provides an example how to build custom linter using XHPAST.

Test Plan: Added debug output to `willLintPaths()`, verified that each path is parsed only once.

Reviewers: epriestley

Reviewed By: epriestley

CC: aran, Korvin

Differential Revision: https://secure.phabricator.com/D4718

src/__phutil_library_map__.php

@@ -21,6 +21,7 @@ phutil_register_library_map(array(
     'ArcanistBaseTestResultParser' => 'unit/engine/ArcanistBaseTestResultParser.php',
     'ArcanistBaseUnitTestEngine' => 'unit/engine/ArcanistBaseUnitTestEngine.php',
     'ArcanistBaseWorkflow' => 'workflow/ArcanistBaseWorkflow.php',
+    'ArcanistBaseXHPASTLinter' => 'lint/linter/ArcanistBaseXHPASTLinter.php',
     'ArcanistBranchWorkflow' => 'workflow/ArcanistBranchWorkflow.php',
     'ArcanistBrowseWorkflow' => 'workflow/ArcanistBrowseWorkflow.php',
     'ArcanistBundle' => 'parser/ArcanistBundle.php',
@@ -106,6 +107,8 @@ phutil_register_library_map(array(
     'ArcanistPhutilTestCaseTestCase' => 'unit/engine/phutil/testcase/ArcanistPhutilTestCaseTestCase.php',
     'ArcanistPhutilTestSkippedException' => 'unit/engine/phutil/testcase/ArcanistPhutilTestSkippedException.php',
     'ArcanistPhutilTestTerminatedException' => 'unit/engine/phutil/testcase/ArcanistPhutilTestTerminatedException.php',
+    'ArcanistPhutilXHPASTLinter' => 'lint/linter/ArcanistPhutilXHPASTLinter.php',
+    'ArcanistPhutilXHPASTLinterTestCase' => 'lint/linter/__tests__/ArcanistPhutilXHPASTLinterTestCase.php',
     'ArcanistPyFlakesLinter' => 'lint/linter/ArcanistPyFlakesLinter.php',
     'ArcanistPyLintLinter' => 'lint/linter/ArcanistPyLintLinter.php',
     'ArcanistRepoUtilsTestCase' => 'repository/util/__tests__/ArcanistRepoUtilsTestCase.php',
@@ -172,6 +175,7 @@ phutil_register_library_map(array(
     'ArcanistArcanistLinterTestCase' => 'ArcanistLinterTestCase',
     'ArcanistBaseCommitParserTestCase' => 'ArcanistTestCase',
     'ArcanistBaseWorkflow' => 'Phobject',
+    'ArcanistBaseXHPASTLinter' => 'ArcanistLinter',
     'ArcanistBranchWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistBrowseWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistBundleTestCase' => 'ArcanistTestCase',
@@ -234,6 +238,8 @@ phutil_register_library_map(array(
     'ArcanistPhutilTestCaseTestCase' => 'ArcanistPhutilTestCase',
     'ArcanistPhutilTestSkippedException' => 'Exception',
     'ArcanistPhutilTestTerminatedException' => 'Exception',
+    'ArcanistPhutilXHPASTLinter' => 'ArcanistBaseXHPASTLinter',
+    'ArcanistPhutilXHPASTLinterTestCase' => 'ArcanistArcanistLinterTestCase',
     'ArcanistPyFlakesLinter' => 'ArcanistLinter',
     'ArcanistPyLintLinter' => 'ArcanistLinter',
     'ArcanistRepoUtilsTestCase' => 'ArcanistTestCase',
@@ -264,7 +270,7 @@ phutil_register_library_map(array(
     'ArcanistUserAbortException' => 'ArcanistUsageException',
     'ArcanistWhichWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistXHPASTLintNamingHookTestCase' => 'ArcanistTestCase',
-    'ArcanistXHPASTLinter' => 'ArcanistLinter',
+    'ArcanistXHPASTLinter' => 'ArcanistBaseXHPASTLinter',
     'ArcanistXHPASTLinterTestCase' => 'ArcanistArcanistLinterTestCase',
     'ComprehensiveLintEngine' => 'ArcanistLintEngine',
     'ExampleLintEngine' => 'ArcanistLintEngine',

src/lint/engine/PhutilLintEngine.php

@@ -46,9 +46,16 @@ class PhutilLintEngine extends ArcanistLintEngine {
     $linters[] = id(new ArcanistTextLinter())->setPaths($text_paths);
     $linters[] = id(new ArcanistSpellingLinter())->setPaths($text_paths);
 
-    $linters[] = id(new ArcanistXHPASTLinter())
+    $php_paths = preg_grep('/\.php$/', $paths);
+
+    $xhpast_linter = id(new ArcanistXHPASTLinter())
       ->setCustomSeverityMap($this->getXHPASTSeverityMap())
-      ->setPaths(preg_grep('/\.php$/', $paths));
+      ->setPaths($php_paths);
+    $linters[] = $xhpast_linter;
+
+    $linters[] = id(new ArcanistPhutilXHPASTLinter())
+      ->setXHPASTLinter($xhpast_linter)
+      ->setPaths($php_paths);
 
     return $linters;
   }
@@ -61,11 +68,9 @@ class PhutilLintEngine extends ArcanistLintEngine {
     return array(
       ArcanistXHPASTLinter::LINT_PHP_53_FEATURES          => $error,
       ArcanistXHPASTLinter::LINT_PHP_54_FEATURES          => $error,
-      ArcanistXHPASTLinter::LINT_PHT_WITH_DYNAMIC_STRING  => $error,
       ArcanistXHPASTLinter::LINT_COMMENT_SPACING          => $error,
       ArcanistXHPASTLinter::LINT_RAGGED_CLASSTREE_EDGE    => $warning,
       ArcanistXHPASTLinter::LINT_TODO_COMMENT             => $advice,
-      ArcanistXHPASTLinter::LINT_ARRAY_COMBINE            => $warning,
     );
   }
 }

src/lint/linter/ArcanistBaseXHPASTLinter.php

@@ -0,0 +1,34 @@
+<?php
+
+/**
+ * @group linter
+ */
+abstract class ArcanistBaseXHPASTLinter extends ArcanistLinter {
+
+  protected function raiseLintAtToken(
+    XHPASTToken $token,
+    $code,
+    $desc,
+    $replace = null) {
+    return $this->raiseLintAtOffset(
+      $token->getOffset(),
+      $code,
+      $desc,
+      $token->getValue(),
+      $replace);
+  }
+
+  protected function raiseLintAtNode(
+    XHPASTNode $node,
+    $code,
+    $desc,
+    $replace = null) {
+    return $this->raiseLintAtOffset(
+      $node->getOffset(),
+      $code,
+      $desc,
+      $node->getConcreteString(),
+      $replace);
+  }
+
+}

src/lint/linter/ArcanistPhutilXHPASTLinter.php

@@ -0,0 +1,149 @@
+<?php
+
+/**
+ * @group linter
+ */
+final class ArcanistPhutilXHPASTLinter extends ArcanistBaseXHPASTLinter {
+
+  const LINT_PHT_WITH_DYNAMIC_STRING = 1;
+  const LINT_ARRAY_COMBINE           = 2;
+
+  private $xhpastLinter;
+
+  public function setXHPASTLinter(ArcanistXHPASTLinter $linter) {
+    $this->xhpastLinter = $linter;
+    return $this;
+  }
+
+  public function setEngine(ArcanistLintEngine $engine) {
+    if (!$this->xhpastLinter) {
+      throw new Exception(
+        'Call setXHPASTLinter() before using ArcanistPhutilXHPASTLinter.');
+    }
+    $this->xhpastLinter->setEngine($engine);
+    return parent::setEngine($engine);
+  }
+
+  public function getLintNameMap() {
+    return array(
+      self::LINT_PHT_WITH_DYNAMIC_STRING => 'Use of pht() on Dynamic String',
+      self::LINT_ARRAY_COMBINE           => 'array_combine() Unreliable',
+    );
+  }
+
+  public function getLintSeverityMap() {
+    return array(
+      self::LINT_ARRAY_COMBINE => ArcanistLintSeverity::SEVERITY_WARNING,
+    );
+  }
+
+  public function getLinterName() {
+    return 'PHLXHP';
+  }
+
+  public function willLintPaths(array $paths) {
+    $this->xhpastLinter->willLintPaths($paths);
+  }
+
+  public function lintPath($path) {
+    $tree = $this->xhpastLinter->getXHPASTTreeForPath($path);
+    if (!$tree) {
+      return;
+    }
+
+    $root = $tree->getRootNode();
+
+    $this->lintPHT($root);
+    $this->lintArrayCombine($root);
+  }
+
+
+  private function lintPHT($root) {
+    $calls = $root->selectDescendantsOfType('n_FUNCTION_CALL');
+    foreach ($calls as $call) {
+      $name = $call->getChildByIndex(0)->getConcreteString();
+      if (strcasecmp($name, 'pht') != 0) {
+        continue;
+      }
+
+      $parameters = $call->getChildOfType(1, 'n_CALL_PARAMETER_LIST');
+      if (!$parameters->getChildren()) {
+        continue;
+      }
+
+      $identifier = $parameters->getChildByIndex(0);
+      if ($this->isConstantString($identifier)) {
+        continue;
+      }
+
+      $this->raiseLintAtNode(
+        $call,
+        self::LINT_PHT_WITH_DYNAMIC_STRING,
+        "The first parameter of pht() can be only a scalar string, ".
+          "otherwise it can't be extracted.");
+    }
+  }
+
+  private function isConstantString(XHPASTNode $node) {
+    $value = $node->getConcreteString();
+
+    switch ($node->getTypeName()) {
+      case 'n_HEREDOC':
+        if ($value[3] == "'") { // Nowdoc: <<<'EOT'
+          return true;
+        }
+        $value = preg_replace('/^.+\n|\n.*$/', '', $value);
+        break;
+
+      case 'n_STRING_SCALAR':
+        if ($value[0] == "'") {
+          return true;
+        }
+        $value = substr($value, 1, -1);
+        break;
+
+      case 'n_CONCATENATION_LIST':
+        foreach ($node->getChildren() as $child) {
+          if ($child->getTypeName() == 'n_OPERATOR') {
+            continue;
+          }
+          if (!$this->isConstantString($child)) {
+            return false;
+          }
+        }
+        return true;
+
+      default:
+        return false;
+    }
+
+    return preg_match('/^((?>[^$\\\\]*)|\\\\.)*$/s', $value);
+  }
+
+
+  private function lintArrayCombine($root) {
+    $function_calls = $root->selectDescendantsOfType('n_FUNCTION_CALL');
+    foreach ($function_calls as $call) {
+      $name = $call->getChildByIndex(0)->getConcreteString();
+      if (strcasecmp($name, 'array_combine') == 0) {
+        $parameter_list = $call->getChildOfType(1, 'n_CALL_PARAMETER_LIST');
+        if (count($parameter_list->getChildren()) !== 2) {
+          // Wrong number of parameters, but raise that elsewhere if we want.
+          continue;
+        }
+
+        $first = $parameter_list->getChildByIndex(0);
+        $second = $parameter_list->getChildByIndex(1);
+
+        if ($first->getConcreteString() == $second->getConcreteString()) {
+          $this->raiseLintAtNode(
+            $call,
+            self::LINT_ARRAY_COMBINE,
+            'Prior to PHP 5.4, array_combine() fails when given empty '.
+            'arrays. Prefer to write array_combine(x, x) as array_fuse(x).');
+        }
+      }
+    }
+  }
+
+}

src/lint/linter/ArcanistXHPASTLinter.php

@@ -5,7 +5,7 @@
  *
  * @group linter
  */
-final class ArcanistXHPASTLinter extends ArcanistLinter {
+final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
 
   protected $trees = array();
 
@@ -40,11 +40,9 @@ final class ArcanistXHPASTLinter extends ArcanistLinter {
   const LINT_IMPLICIT_FALLTHROUGH      = 30;
   const LINT_PHP_53_FEATURES           = 31;
   const LINT_REUSED_AS_ITERATOR        = 32;
-  const LINT_PHT_WITH_DYNAMIC_STRING   = 33;
   const LINT_COMMENT_SPACING           = 34;
   const LINT_PHP_54_FEATURES           = 35;
   const LINT_SLOWNESS                  = 36;
-  const LINT_ARRAY_COMBINE             = 37;
 
 
   public function getLintNameMap() {
@@ -81,10 +79,8 @@ final class ArcanistXHPASTLinter extends ArcanistLinter {
       self::LINT_PHP_53_FEATURES           => 'Use Of PHP 5.3 Features',
       self::LINT_PHP_54_FEATURES           => 'Use Of PHP 5.4 Features',
       self::LINT_REUSED_AS_ITERATOR        => 'Variable Reused As Iterator',
-      self::LINT_PHT_WITH_DYNAMIC_STRING   => 'Use of pht() on Dynamic String',
       self::LINT_COMMENT_SPACING           => 'Comment Spaces',
       self::LINT_SLOWNESS                  => 'Slow Construct',
-      self::LINT_ARRAY_COMBINE             => 'array_combine() Unreliable',
     );
   }
 
@@ -108,7 +104,6 @@ final class ArcanistXHPASTLinter extends ArcanistLinter {
       self::LINT_BINARY_EXPRESSION_SPACING => $warning,
       self::LINT_ARRAY_INDEX_SPACING       => $warning,
       self::LINT_IMPLICIT_FALLTHROUGH      => $warning,
-      self::LINT_PHT_WITH_DYNAMIC_STRING   => $disabled,
       self::LINT_SLOWNESS                  => $warning,
       self::LINT_COMMENT_SPACING           => $advice,
 
@@ -120,24 +115,27 @@ final class ArcanistXHPASTLinter extends ArcanistLinter {
       // a specific minimum version.
       self::LINT_PHP_53_FEATURES           => $disabled,
       self::LINT_PHP_54_FEATURES           => $disabled,
-
-      // This message specifically recommends array_fuse(), a libphutil
-      // function.
-      self::LINT_ARRAY_COMBINE             => $disabled,
     );
   }
 
   public function willLintPaths(array $paths) {
     $futures = array();
     foreach ($paths as $path) {
+      if (array_key_exists($path, $this->trees)) {
+        continue;
+      }
       $futures[$path] = xhpast_get_parser_future($this->getData($path));
     }
     foreach (Futures($futures)->limit(8) as $path => $future) {
       $this->willLintPath($path);
+      $this->trees[$path] = null;
       try {
         $this->trees[$path] = XHPASTTree::newFromDataAndResolvedExecFuture(
           $this->getData($path),
           $future->resolve());
+        $root = $this->trees[$path]->getRootNode();
+        $root->buildSelectCache();
+        $root->buildTokenCache();
       } catch (XHPASTSyntaxErrorException $ex) {
         $this->raiseLintAtLine(
           $ex->getErrorLine(),
@@ -171,15 +169,12 @@ final class ArcanistXHPASTLinter extends ArcanistLinter {
   }
 
   public function lintPath($path) {
-    if (empty($this->trees[$path])) {
+    if (!$this->trees[$path]) {
       return;
     }
 
     $root = $this->trees[$path]->getRootNode();
 
-    $root->buildSelectCache();
-    $root->buildTokenCache();
-
     $this->lintUseOfThisInStaticMethods($root);
     $this->lintDynamicDefines($root);
     $this->lintSurpriseConstructors($root);
@@ -206,10 +201,8 @@ final class ArcanistXHPASTLinter extends ArcanistLinter {
     $this->lintImplicitFallthrough($root);
     $this->lintPHP53Features($root);
     $this->lintPHP54Features($root);
-    $this->lintPHT($root);
     $this->lintStrposUsedForStart($root);
     $this->lintStrstrUsedForCheck($root);
-    $this->lintArrayCombine($root);
   }
 
   public function lintStrstrUsedForCheck($root) {
@@ -297,68 +290,6 @@ final class ArcanistXHPASTLinter extends ArcanistLinter {
     }
   }
 
-  public function lintPHT($root) {
-    $calls = $root->selectDescendantsOfType('n_FUNCTION_CALL');
-    foreach ($calls as $call) {
-      $name = strtolower($call->getChildByIndex(0)->getConcreteString());
-      if ($name != 'pht') {
-        continue;
-      }
-
-      $parameters = $call->getChildOfType(1, 'n_CALL_PARAMETER_LIST');
-      if (!$parameters->getChildren()) {
-        continue;
-      }
-
-      $identifier = $parameters->getChildByIndex(0);
-      if ($this->isConstantString($identifier)) {
-        continue;
-      }
-
-      $this->raiseLintAtNode(
-        $call,
-        self::LINT_PHT_WITH_DYNAMIC_STRING,
-        "The first parameter of pht() can be only a scalar string, ".
-          "otherwise it can't be extracted.");
-    }
-  }
-
-  private function isConstantString(XHPASTNode $node) {
-    $value = $node->getConcreteString();
-
-    switch ($node->getTypeName()) {
-      case 'n_HEREDOC':
-        if ($value[3] == "'") { // Nowdoc: <<<'EOT'
-          return true;
-        }
-        $value = preg_replace('/^.+\n|\n.*$/', '', $value);
-        break;
-
-      case 'n_STRING_SCALAR':
-        if ($value[0] == "'") {
-          return true;
-        }
-        $value = substr($value, 1, -1);
-        break;
-
-      case 'n_CONCATENATION_LIST':
-        foreach ($node->getChildren() as $child) {
-          if ($child->getTypeName() == 'n_OPERATOR') {
-            continue;
-          }
-          if (!$this->isConstantString($child)) {
-            return false;
-          }
-        }
-        return true;
-
-      default:
-        return false;
-    }
-
-    return preg_match('/^((?>[^$\\\\]*)|\\\\.)*$/s', $value);
-  }
-
   public function lintPHP53Features($root) {
 
     $functions = $root->selectTokensOfType('T_FUNCTION');
@@ -1806,32 +1737,6 @@ final class ArcanistXHPASTLinter extends ArcanistLinter {
     }
   }
 
-  protected function lintArrayCombine($root) {
-    $function_calls = $root->selectDescendantsOfType('n_FUNCTION_CALL');
-    foreach ($function_calls as $call) {
-      $name = $call->getChildByIndex(0)->getConcreteString();
-      if (strtolower($name) === 'array_combine') {
-        $parameter_list = $call->getChildOfType(1, 'n_CALL_PARAMETER_LIST');
-        if (count($parameter_list->getChildren()) !== 2) {
-          // Wrong number of parameters, but raise that elsewhere if we want.
-          continue;
-        }
-
-        $first = $parameter_list->getChildByIndex(0);
-        $second = $parameter_list->getChildByIndex(1);
-
-        if ($first->getConcreteString() == $second->getConcreteString()) {
-          $this->raiseLintAtNode(
-            $call,
-            self::LINT_ARRAY_COMBINE,
-            'Prior to PHP 5.4, array_combine() fails when given empty '.
-            'arrays. Prefer to write array_combine(x, x) as array_fuse(x).');
-        }
-      }
-    }
-  }
-
-
   /**
    * Exit is parsed as an expression, but using it as such is almost always
    * wrong. That is, this is valid:
@@ -2058,32 +1963,6 @@ final class ArcanistXHPASTLinter extends ArcanistLinter {
     }
   }
 
-  protected function raiseLintAtToken(
-    XHPASTToken $token,
-    $code,
-    $desc,
-    $replace = null) {
-    return $this->raiseLintAtOffset(
-      $token->getOffset(),
-      $code,
-      $desc,
-      $token->getValue(),
-      $replace);
-  }
-
-  protected function raiseLintAtNode(
-    XHPASTNode $node,
-    $code,
-    $desc,
-    $replace = null) {
-    return $this->raiseLintAtOffset(
-      $node->getOffset(),
-      $code,
-      $desc,
-      $node->getConcreteString(),
-      $replace);
-  }
-
   public function getSuperGlobalNames() {
     return array(
       '$GLOBALS',

src/lint/linter/__tests__/ArcanistPhutilXHPASTLinterTestCase.php

@@ -0,0 +1,20 @@
+<?php
+
+/**
+ * @group testcase
+ */
+final class ArcanistPhutilXHPASTLinterTestCase
+  extends ArcanistArcanistLinterTestCase {
+
+  public function testPhutilXHPASTLint() {
+    $linter = new ArcanistPhutilXHPASTLinter();
+    $linter->setXHPASTLinter(new ArcanistXHPASTLinter());
+
+    $working_copy = ArcanistWorkingCopyIdentity::newFromPath(__FILE__);
+    return $this->executeTestsInDirectory(
+      dirname(__FILE__).'/phlxhp/',
+      $linter,
+      $working_copy);
+  }
+
+}

src/lint/linter/__tests__/phlxhp/array-combine.lint-test

@@ -3,4 +3,4 @@
 array_combine($x, $x);
 array_combine($x, $y);
 ~~~~~~~~~~
-disabled:3:1
+warning:3:1

src/lint/linter/__tests__/phlxhp/pht.lint-test

@@ -20,8 +20,8 @@ $a
 EOT
 );
 ~~~~~~~~~~
-disabled:5:1
+error:5:1
-disabled:7:1
+error:7:1
-disabled:8:1
+error:8:1
-disabled:10:1
+error:10:1
-disabled:18:1
+error:18:1