From 86cbcb9f564bad6f7000e70eb57c717d127821da Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 11 May 2014 19:28:46 -0700 Subject: [PATCH] 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 | 36 ++++++ src/lint/engine/PhutilLintEngine.php | 1 - src/lint/linter/ArcanistBaseXHPASTLinter.php | 119 +++++++++++++++++- .../linter/ArcanistPhutilXHPASTLinter.php | 29 +---- src/lint/linter/ArcanistXHPASTLinter.php | 47 ++----- .../ArcanistPhutilXHPASTLinterTestCase.php | 1 - 6 files changed, 165 insertions(+), 68 deletions(-) diff --git a/src/lint/engine/ArcanistLintEngine.php b/src/lint/engine/ArcanistLintEngine.php index d634bba9..6f030a13 100644 --- a/src/lint/engine/ArcanistLintEngine.php +++ b/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; + } + + } diff --git a/src/lint/engine/PhutilLintEngine.php b/src/lint/engine/PhutilLintEngine.php index 968f9b20..666ffbc1 100644 --- a/src/lint/engine/PhutilLintEngine.php +++ b/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()); diff --git a/src/lint/linter/ArcanistBaseXHPASTLinter.php b/src/lint/linter/ArcanistBaseXHPASTLinter.php index 192a9443..1fe7d778 100644 --- a/src/lint/linter/ArcanistBaseXHPASTLinter.php +++ b/src/lint/linter/ArcanistBaseXHPASTLinter.php @@ -1,10 +1,14 @@ 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 Paths to build futures for. + * @return list 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); + } + + } diff --git a/src/lint/linter/ArcanistPhutilXHPASTLinter.php b/src/lint/linter/ArcanistPhutilXHPASTLinter.php index 92b8ac6e..b5a59d59 100644 --- a/src/lint/linter/ArcanistPhutilXHPASTLinter.php +++ b/src/lint/linter/ArcanistPhutilXHPASTLinter.php @@ -1,24 +1,15 @@ 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; } diff --git a/src/lint/linter/ArcanistXHPASTLinter.php b/src/lint/linter/ArcanistXHPASTLinter.php index 9cf651d0..dc288412 100644 --- a/src/lint/linter/ArcanistXHPASTLinter.php +++ b/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; } diff --git a/src/lint/linter/__tests__/ArcanistPhutilXHPASTLinterTestCase.php b/src/lint/linter/__tests__/ArcanistPhutilXHPASTLinterTestCase.php index 32e5a4f9..f1036583 100644 --- a/src/lint/linter/__tests__/ArcanistPhutilXHPASTLinterTestCase.php +++ b/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.', ));