Allow linters to share resources by shoving them on the Engine

Summary:
Currently, the Phutil XHPAST Linter and vanilla XHPAST Linter reuse the same parse tree, but do this by having explicit knowledge of one another.

Instead, let them synchronize by writing to a glorified array of globals on the Engine. They no longer require knowledge of one another, so this can work under `.arclint`.

(This could probably be a little cleaner by putting more logic in the shared base class, but Facebook has some kind of goofy subclass of this thing and //this// patch won't disrupt it, while a cleaner one might.)

This should unblock D9057.

Test Plan: Ran unit tests and normal lint, got accurate looking results without duplicate invocations showing up in `--trace`.

Reviewers: btrahan, joshuaspence

Reviewed By: joshuaspence

Subscribers: epriestley

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

src/lint/engine/ArcanistLintEngine.php

@@ -65,6 +65,8 @@ abstract class ArcanistLintEngine {
   private $postponedLinters = array();
   private $configurationManager;
 
+  private $linterResources = array();
+
   public function __construct() {
 
   }
@@ -548,4 +550,38 @@ abstract class ArcanistLintEngine {
   }
 
 
+  /**
+   * Get a named linter resource shared by another linter.
+   *
+   * This mechanism allows linters to share arbitrary resources, like the
+   * results of computation. If several linters need to perform the same
+   * expensive computation step, they can use a named resource to synchronize
+   * construction of the result so it doesn't need to be built multiple
+   * times.
+   *
+   * @param string  Resource identifier.
+   * @param wild    Optionally, default value to return if resource does not
+   *                exist.
+   * @return wild   Resource, or default value if not present.
+   */
+  public function getLinterResource($key, $default = null) {
+    return idx($this->linterResources, $key, $default);
+  }
+
+
+  /**
+   * Set a linter resource that other linters can accesss.
+   *
+   * See @{method:getLinterResource} for a description of this mechanism.
+   *
+   * @param string Resource identifier.
+   * @param wild   Resource.
+   * @return this
+   */
+  public function setLinterResource($key, $value) {
+    $this->linterResources[$key] = $value;
+    return $this;
+  }
+
+
 }

src/lint/engine/PhutilLintEngine.php

@@ -58,7 +58,6 @@ class PhutilLintEngine extends ArcanistLintEngine {
     $linters[] = $xhpast_linter;
 
     $linters[] = id(new ArcanistPhutilXHPASTLinter())
-      ->setXHPASTLinter($xhpast_linter)
       ->setPaths($php_paths);
 
     $merge_conflict_linter = id(new ArcanistMergeConflictLinter());

src/lint/linter/ArcanistBaseXHPASTLinter.php

@@ -1,10 +1,14 @@
 <?php
 
 /**
- * @group linter
+ * @task sharing Sharing Parse Trees
  */
 abstract class ArcanistBaseXHPASTLinter extends ArcanistFutureLinter {
 
+  private $futures = array();
+  private $trees = array();
+  private $exceptions = array();
+
   protected final function raiseLintAtToken(
     XHPASTToken $token,
     $code,
@@ -31,4 +35,117 @@ abstract class ArcanistBaseXHPASTLinter extends ArcanistFutureLinter {
       $replace);
   }
 
+  protected function buildFutures(array $paths) {
+    return $this->getXHPASTLinter()->buildSharedFutures($paths);
+  }
+
+
+/* -(  Sharing Parse Trees  )------------------------------------------------ */
+
+
+  /**
+   * Get the linter object which is responsible for building parse trees.
+   *
+   * When the engine specifies that several XHPAST linters should execute,
+   * we designate one of them as the one which will actually build parse trees.
+   * The other linters share trees, so they don't have to recompute them.
+   *
+   * Roughly, the first linter to execute elects itself as the builder.
+   * Subsequent linters request builds and retrieve results from it.
+   *
+   * @return ArcanistBaseXHPASTLinter Responsible linter.
+   * @task sharing
+   */
+  protected function getXHPASTLinter() {
+    $resource_key = 'xhpast.linter';
+
+    // If we're the first linter to run, share ourselves. Otherwise, grab the
+    // previously shared linter.
+
+    $engine = $this->getEngine();
+    $linter = $engine->getLinterResource($resource_key);
+    if (!$linter) {
+      $linter = $this;
+      $engine->setLinterResource($resource_key, $linter);
+    }
+
+    $base_class = __CLASS__;
+    if (!($linter instanceof $base_class)) {
+      throw new Exception(
+        pht(
+          'Expected resource "%s" to be an instance of "%s"!',
+          $resource_key,
+          $base_class));
+    }
+
+    return $linter;
+  }
+
+
+  /**
+   * Build futures on this linter, for use and to share with other linters.
+   *
+   * @param list<string> Paths to build futures for.
+   * @return list<ExecFuture> Futures.
+   * @task sharing
+   */
+  protected function buildSharedFutures(array $paths) {
+    foreach ($paths as $path) {
+      if (!isset($this->futures[$path])) {
+        $this->futures[$path] = xhpast_get_parser_future($this->getData($path));
+      }
+    }
+    return array_select_keys($this->futures, $paths);
+  }
+
+
+  /**
+   * Get a path's tree from the responsible linter.
+   *
+   * @param   string           Path to retrieve tree for.
+   * @return  XHPASTTree|null  Tree, or null if unparseable.
+   * @task sharing
+   */
+  protected function getXHPASTTreeForPath($path) {
+
+    // If we aren't the linter responsible for actually building the parse
+    // trees, go get the tree from that linter.
+    if ($this->getXHPASTLinter() !== $this) {
+      return $this->getXHPASTLinter()->getXHPASTTreeForPath($path);
+    }
+
+    if (!array_key_exists($path, $this->trees)) {
+      $this->trees[$path] = null;
+      try {
+        $this->trees[$path] = XHPASTTree::newFromDataAndResolvedExecFuture(
+          $this->getData($path),
+          $this->futures[$path]->resolve());
+        $root = $this->trees[$path]->getRootNode();
+        $root->buildSelectCache();
+        $root->buildTokenCache();
+      } catch (Exception $ex) {
+        $this->exceptions[$path] = $ex;
+      }
+    }
+
+    return $this->trees[$path];
+  }
+
+
+  /**
+   * Get a path's parse exception from the responsible linter.
+   *
+   * @param   string          Path to retrieve exception for.
+   * @return  Exeption|null   Parse exception, if available.
+   * @task sharing
+   */
+  protected function getXHPASTExceptionForPath($path) {
+    if ($this->getXHPASTLinter() !== $this) {
+      return $this->getXHPASTLinter()->getXHPASTExceptionForPath($path);
+    }
+
+    return idx($this->exceptions, $path);
+  }
+
+
 }

src/lint/linter/ArcanistPhutilXHPASTLinter.php

@@ -1,24 +1,15 @@
 <?php
 
-/**
- * @group linter
- */
 final class ArcanistPhutilXHPASTLinter extends ArcanistBaseXHPASTLinter {
 
   const LINT_ARRAY_COMBINE           = 2;
   const LINT_DEPRECATED_FUNCTION     = 3;
   const LINT_UNSAFE_DYNAMIC_STRING   = 4;
 
-  private $xhpastLinter;
   private $deprecatedFunctions = array();
   private $dynamicStringFunctions = array();
   private $dynamicStringClasses = array();
 
-  public function setXHPASTLinter(ArcanistXHPASTLinter $linter) {
-    $this->xhpastLinter = $linter;
-    return $this;
-  }
-
   public function setDeprecatedFunctions($map) {
     $this->deprecatedFunctions = $map;
     return $this;
@@ -34,15 +25,6 @@ final class ArcanistPhutilXHPASTLinter extends ArcanistBaseXHPASTLinter {
     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_ARRAY_COMBINE           => 'array_combine() Unreliable',
@@ -73,17 +55,8 @@ final class ArcanistPhutilXHPASTLinter extends ArcanistBaseXHPASTLinter {
     return $version;
   }
 
-  protected function buildFutures(array $paths) {
-    return $this->xhpastLinter->buildFutures($paths);
-  }
-
-  public function willLintPath($path) {
-    $this->xhpastLinter->willLintPath($path);
-    return parent::willLintPath($path);
-  }
-
   protected function resolveFuture($path, Future $future) {
-    $tree = $this->xhpastLinter->getXHPASTTreeForPath($path);
+    $tree = $this->getXHPASTLinter()->getXHPASTTreeForPath($path);
     if (!$tree) {
       return;
     }

src/lint/linter/ArcanistXHPASTLinter.php

@@ -2,14 +2,9 @@
 
 /**
  * Uses XHPAST to apply lint rules to PHP.
- *
- * @group linter
  */
 final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
 
-  private $futures = array();
-  private $trees = array();
-
   const LINT_PHP_SYNTAX_ERROR          = 1;
   const LINT_UNABLE_TO_PARSE           = 2;
   const LINT_VARIABLE_VARIABLE         = 3;
@@ -130,38 +125,6 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
     );
   }
 
-  protected function buildFutures(array $paths) {
-    foreach ($paths as $path) {
-      if (!isset($this->futures[$path])) {
-        $this->futures[$path] = xhpast_get_parser_future($this->getData($path));
-      }
-    }
-    return array_select_keys($this->futures, $paths);
-  }
-
-  public function getXHPASTTreeForPath($path) {
-    if (!array_key_exists($path, $this->trees)) {
-      $this->trees[$path] = null;
-      try {
-        $this->trees[$path] = XHPASTTree::newFromDataAndResolvedExecFuture(
-          $this->getData($path),
-          $this->futures[$path]->resolve());
-        $root = $this->trees[$path]->getRootNode();
-        $root->buildSelectCache();
-        $root->buildTokenCache();
-      } catch (XHPASTSyntaxErrorException $ex) {
-        $this->raiseLintAtLine(
-          $ex->getErrorLine(),
-          1,
-          self::LINT_PHP_SYNTAX_ERROR,
-          'This file contains a syntax error: '.$ex->getMessage());
-      } catch (Exception $ex) {
-        $this->raiseLintAtPath(self::LINT_UNABLE_TO_PARSE, $ex->getMessage());
-      }
-    }
-    return $this->trees[$path];
-  }
-
   public function getCacheVersion() {
     $version = '4';
     $path = xhpast_get_binary_path();
@@ -174,6 +137,16 @@ final class ArcanistXHPASTLinter extends ArcanistBaseXHPASTLinter {
   protected function resolveFuture($path, Future $future) {
     $tree = $this->getXHPASTTreeForPath($path);
     if (!$tree) {
+      $ex = $this->getXHPASTExceptionForPath($path);
+      if ($ex instanceof XHPASTSyntaxErrorException) {
+        $this->raiseLintAtLine(
+          $ex->getErrorLine(),
+          1,
+          self::LINT_PHP_SYNTAX_ERROR,
+          'This file contains a syntax error: '.$ex->getMessage());
+      } else if ($ex instanceof Exception) {
+        $this->raiseLintAtPath(self::LINT_UNABLE_TO_PARSE, $ex->getMessage());
+      }
       return;
     }
 

src/lint/linter/__tests__/ArcanistPhutilXHPASTLinterTestCase.php

@@ -8,7 +8,6 @@ final class ArcanistPhutilXHPASTLinterTestCase
 
   public function testPhutilXHPASTLint() {
     $linter = new ArcanistPhutilXHPASTLinter();
-    $linter->setXHPASTLinter(new ArcanistXHPASTLinter());
     $linter->setDeprecatedFunctions(array(
       'deprecated_function' => 'This function is most likely deprecated.',
     ));