Page MenuHomePhabricator
Files F13239148

D12501.diff
No OneTemporary
Actions

D12501.diff
View Options

diff --git a/src/lint/engine/ArcanistLintEngine.php b/src/lint/engine/ArcanistLintEngine.php
--- a/src/lint/engine/ArcanistLintEngine.php
+++ b/src/lint/engine/ArcanistLintEngine.php
@@ -212,6 +212,10 @@
throw new ArcanistNoEffectException('No linters to run.');
}
+ foreach ($linters as $key => $linter) {
+ $linter->setLinterID($key);
+ }
+
$linters = msort($linters, 'getLinterPriority');
foreach ($linters as $linter) {
$linter->setEngine($this);
@@ -249,75 +253,13 @@
$this->cacheVersion = crc32(implode("\n", $versions));
- $this->stopped = array();
- $exceptions = array();
- foreach ($linters as $linter_name => $linter) {
- if (!is_string($linter_name)) {
- $linter_name = get_class($linter);
- }
- try {
- if (!$linter->canRun()) {
- continue;
- }
- $paths = $linter->getPaths();
-
- foreach ($paths as $key => $path) {
- // Make sure each path has a result generated, even if it is empty
- // (i.e., the file has no lint messages).
- $result = $this->getResultForPath($path);
- if (isset($this->stopped[$path])) {
- unset($paths[$key]);
- }
- if (isset($this->cachedResults[$path][$this->cacheVersion])) {
- $cached_result = $this->cachedResults[$path][$this->cacheVersion];
-
- $use_cache = $this->shouldUseCache(
- $linter->getCacheGranularity(),
- idx($cached_result, 'repository_version'));
+ $runnable = $this->getRunnableLinters($linters);
- if ($use_cache) {
- unset($paths[$key]);
-
- if (idx($cached_result, 'stopped') == $linter_name) {
- $this->stopped[$path] = $linter_name;
- }
- }
- }
- }
- $paths = array_values($paths);
-
- if ($paths) {
- $profiler = PhutilServiceProfiler::getInstance();
- $call_id = $profiler->beginServiceCall(array(
- 'type' => 'lint',
- 'linter' => $linter_name,
- 'paths' => $paths,
- ));
-
- try {
- $linter->willLintPaths($paths);
- foreach ($paths as $path) {
- $linter->willLintPath($path);
- $linter->lintPath($path);
- if ($linter->didStopAllLinters()) {
- $this->stopped[$path] = $linter_name;
- }
- }
- } catch (Exception $ex) {
- $profiler->endServiceCall($call_id, array());
- throw $ex;
- }
- $profiler->endServiceCall($call_id, array());
- }
-
- } catch (Exception $ex) {
- $exceptions[$linter_name] = $ex;
- }
- }
+ $this->stopped = array();
- $exceptions += $this->didRunLinters($linters);
+ $exceptions = $this->executeLinters($runnable);
- foreach ($linters as $linter) {
+ foreach ($runnable as $linter) {
foreach ($linter->getLintMessages() as $message) {
if (!$this->isSeverityEnabled($message->getSeverity())) {
continue;
@@ -419,33 +361,6 @@
abstract public function buildLinters();
- final protected function didRunLinters(array $linters) {
- assert_instances_of($linters, 'ArcanistLinter');
-
- $exceptions = array();
- $profiler = PhutilServiceProfiler::getInstance();
-
- foreach ($linters as $linter_name => $linter) {
- if (!is_string($linter_name)) {
- $linter_name = get_class($linter);
- }
-
- $call_id = $profiler->beginServiceCall(array(
- 'type' => 'lint',
- 'linter' => $linter_name,
- ));
-
- try {
- $linter->didRunLinters();
- } catch (Exception $ex) {
- $exceptions[$linter_name] = $ex;
- }
- $profiler->endServiceCall($call_id, array());
- }
-
- return $exceptions;
- }
-
final public function setRepositoryVersion($version) {
$this->repositoryVersion = $version;
return $this;
@@ -579,4 +494,161 @@
return $this;
}
+
+ private function getRunnableLinters(array $linters) {
+ assert_instances_of($linters, 'ArcanistLinter');
+
+ // TODO: The canRun() mechanism is only used by one linter, and just
+ // silently disables the linter. Almost every other linter handles this
+ // by throwing `ArcanistMissingLinterException`. Both mechanisms are not
+ // ideal; linters which can not run should emit a message, get marked as
+ // "skipped", and allow execution to continue. See T7045.
+
+ $runnable = array();
+ foreach ($linters as $key => $linter) {
+ if ($linter->canRun()) {
+ $runnable[$key] = $linter;
+ }
+ }
+
+ return $runnable;
+ }
+
+ private function executeLinters(array $runnable) {
+ $all_paths = $this->getPaths();
+ $path_chunks = array_chunk($all_paths, 32, $preserve_keys = true);
+
+ $exception_lists = array();
+ foreach ($path_chunks as $chunk) {
+ $exception_lists[] = $this->executeLintersOnChunk($runnable, $chunk);
+ }
+
+ return array_mergev($exception_lists);
+ }
+
+
+ private function executeLintersOnChunk(array $runnable, array $path_list) {
+ assert_instances_of($runnable, 'ArcanistLinter');
+
+ $path_map = array_fuse($path_list);
+
+ $exceptions = array();
+ $did_lint = array();
+ foreach ($runnable as $linter) {
+ $linter_id = $linter->getLinterID();
+ $paths = $linter->getPaths();
+
+ foreach ($paths as $key => $path) {
+ // If we aren't running this path in the current chunk of paths,
+ // skip it completely.
+ if (empty($path_map[$path])) {
+ unset($paths[$key]);
+ continue;
+ }
+
+ // Make sure each path has a result generated, even if it is empty
+ // (i.e., the file has no lint messages).
+ $result = $this->getResultForPath($path);
+
+ // If a linter has stopped all other linters for this path, don't
+ // actually run the linter.
+ if (isset($this->stopped[$path])) {
+ unset($paths[$key]);
+ continue;
+ }
+
+ // If we have a cached result for this path, don't actually run the
+ // linter.
+ if (isset($this->cachedResults[$path][$this->cacheVersion])) {
+ $cached_result = $this->cachedResults[$path][$this->cacheVersion];
+
+ $use_cache = $this->shouldUseCache(
+ $linter->getCacheGranularity(),
+ idx($cached_result, 'repository_version'));
+
+ if ($use_cache) {
+ unset($paths[$key]);
+ if (idx($cached_result, 'stopped') == $linter_id) {
+ $this->stopped[$path] = $linter_id;
+ }
+ }
+ }
+ }
+
+ $paths = array_values($paths);
+
+ if (!$paths) {
+ continue;
+ }
+
+ try {
+ $this->executeLinterOnPaths($linter, $paths);
+ $did_lint[] = array($linter, $paths);
+ } catch (Exception $ex) {
+ $exceptions[] = $ex;
+ }
+ }
+
+ foreach ($did_lint as $info) {
+ list($linter, $paths) = $info;
+ try {
+ $this->executeDidLintOnPaths($linter, $paths);
+ } catch (Exception $ex) {
+ $exceptions[] = $ex;
+ }
+ }
+
+ return $exceptions;
+ }
+
+ private function beginLintServiceCall(ArcanistLinter $linter, array $paths) {
+ $profiler = PhutilServiceProfiler::getInstance();
+
+ return $profiler->beginServiceCall(
+ array(
+ 'type' => 'lint',
+ 'linter' => $linter->getInfoName(),
+ 'paths' => $paths,
+ ));
+ }
+
+ private function endLintServiceCall($call_id) {
+ $profiler = PhutilServiceProfiler::getInstance();
+ $profiler->endServiceCall($call_id, array());
+ }
+
+ private function executeLinterOnPaths(ArcanistLinter $linter, array $paths) {
+ $call_id = $this->beginLintServiceCall($linter, $paths);
+
+ try {
+ $linter->willLintPaths($paths);
+ foreach ($paths as $path) {
+ $linter->setActivePath($path);
+ $linter->lintPath($path);
+ if ($linter->didStopAllLinters()) {
+ $this->stopped[$path] = $linter->getLinterID();
+ }
+ }
+ } catch (Exception $ex) {
+ $this->endLintServiceCall($call_id);
+ throw $ex;
+ }
+
+ $this->endLintServiceCall($call_id);
+ }
+
+ private function executeDidLintOnPaths(ArcanistLinter $linter, array $paths) {
+ $call_id = $this->beginLintServiceCall($linter, $paths);
+
+ try {
+ $linter->didLintPaths($paths);
+ } catch (Exception $ex) {
+ $this->endLintServiceCall($call_id);
+ throw $ex;
+ }
+
+ $this->endLintServiceCall($call_id);
+ }
+
+
}
diff --git a/src/lint/linter/ArcanistBaseXHPASTLinter.php b/src/lint/linter/ArcanistBaseXHPASTLinter.php
--- a/src/lint/linter/ArcanistBaseXHPASTLinter.php
+++ b/src/lint/linter/ArcanistBaseXHPASTLinter.php
@@ -8,6 +8,7 @@
private $futures = array();
private $trees = array();
private $exceptions = array();
+ private $refcount = array();
final public function getCacheVersion() {
$parts = array();
@@ -52,6 +53,10 @@
return $this->getXHPASTLinter()->buildSharedFutures($paths);
}
+ protected function didResolveLinterFutures(array $futures) {
+ $this->getXHPASTLinter()->releaseSharedFutures(array_keys($futures));
+ }
+
/* -( Sharing Parse Trees )------------------------------------------------ */
@@ -105,11 +110,44 @@
if (!isset($this->futures[$path])) {
$this->futures[$path] = PhutilXHPASTBinary::getParserFuture(
$this->getData($path));
+ $this->refcount[$path] = 1;
+ } else {
+ $this->refcount[$path]++;
}
}
return array_select_keys($this->futures, $paths);
}
+
+ /**
+ * Release futures on this linter which are no longer in use elsewhere.
+ *
+ * @param list<string> Paths to release futures for.
+ * @return void
+ * @task sharing
+ */
+ final protected function releaseSharedFutures(array $paths) {
+ foreach ($paths as $path) {
+ if (empty($this->refcount[$path])) {
+ throw new Exception(
+ pht(
+ 'Imbalanced calls to shared futures: each call to '.
+ 'buildSharedFutures() for a path must be paired with a call to '.
+ 'releaseSharedFutures().'));
+ }
+
+ $this->refcount[$path]--;
+
+ if (!$this->refcount[$path]) {
+ unset($this->refcount[$path]);
+ unset($this->futures[$path]);
+ unset($this->trees[$path]);
+ unset($this->exceptions[$path]);
+ }
+ }
+ }
+
+
/**
* Get a path's tree from the responsible linter.
*
diff --git a/src/lint/linter/ArcanistCSharpLinter.php b/src/lint/linter/ArcanistCSharpLinter.php
--- a/src/lint/linter/ArcanistCSharpLinter.php
+++ b/src/lint/linter/ArcanistCSharpLinter.php
@@ -182,13 +182,14 @@
$this->futures = $futures;
}
- public function didRunLinters() {
+ public function didLintPaths(array $paths) {
if ($this->futures) {
$futures = id(new FutureIterator($this->futures))
->limit(8);
foreach ($futures as $future) {
$this->resolveFuture($future);
}
+ $this->futures = array();
}
}
diff --git a/src/lint/linter/ArcanistFutureLinter.php b/src/lint/linter/ArcanistFutureLinter.php
--- a/src/lint/linter/ArcanistFutureLinter.php
+++ b/src/lint/linter/ArcanistFutureLinter.php
@@ -19,15 +19,38 @@
}
}
- final public function lintPath($path) {}
-
- final public function didRunLinters() {
- if ($this->futures) {
- foreach ($this->futures as $path => $future) {
- $this->willLintPath($path);
- $this->resolveFuture($path, $future);
- }
+ final public function lintPath($path) {
+ return;
+ }
+
+ final public function didLintPaths(array $paths) {
+ if (!$this->futures) {
+ return;
+ }
+
+ $map = array();
+ foreach ($this->futures as $path => $future) {
+ $this->setActivePath($path);
+ $this->resolveFuture($path, $future);
+ $map[$path] = $future;
}
+ $this->futures = array();
+
+ $this->didResolveLinterFutures($map);
+ }
+
+
+ /**
+ * Hook for cleaning up resources.
+ *
+ * This is invoked after a block of futures resolve, and allows linters to
+ * discard or clean up any shared resources they no longer need.
+ *
+ * @param map<string, Future> Map of paths to resolved futures.
+ * @return void
+ */
+ protected function didResolveLinterFutures(array $futures) {
+ return;
}
}
diff --git a/src/lint/linter/ArcanistLinter.php b/src/lint/linter/ArcanistLinter.php
--- a/src/lint/linter/ArcanistLinter.php
+++ b/src/lint/linter/ArcanistLinter.php
@@ -4,6 +4,8 @@
* Implements lint rules, like syntax checks for a specific language.
*
* @task info Human Readable Information
+ * @task state Runtime State
+ * @task exec Executing Linters
* @stable
*/
abstract class ArcanistLinter {
@@ -13,6 +15,7 @@
const GRANULARITY_REPOSITORY = 3;
const GRANULARITY_GLOBAL = 4;
+ private $id;
protected $paths = array();
protected $data = array();
protected $engine;
@@ -27,6 +30,7 @@
/* -( Human Readable Information )---------------------------------------- */
+
/**
* Return an optional informative URI where humans can learn more about this
* linter.
@@ -69,6 +73,163 @@
get_class($this));
}
+
+/* -( Runtime State )------------------------------------------------------ */
+
+
+ /**
+ * @task state
+ */
+ final public function getActivePath() {
+ return $this->activePath;
+ }
+
+
+ /**
+ * @task state
+ */
+ final public function setActivePath($path) {
+ $this->stopAllLinters = false;
+ $this->activePath = $path;
+ return $this;
+ }
+
+
+ /**
+ * @task state
+ */
+ final public function setEngine(ArcanistLintEngine $engine) {
+ $this->engine = $engine;
+ return $this;
+ }
+
+
+ /**
+ * @task state
+ */
+ final protected function getEngine() {
+ return $this->engine;
+ }
+
+
+ /**
+ * Set the internal ID for this linter.
+ *
+ * This ID is assigned automatically by the @{class:ArcanistLintEngine}.
+ *
+ * @param string Unique linter ID.
+ * @return this
+ * @task state
+ */
+ final public function setLinterID($id) {
+ $this->id = $id;
+ return $this;
+ }
+
+
+ /**
+ * Get the internal ID for this linter.
+ *
+ * Retrieves an internal linter ID managed by the @{class:ArcanistLintEngine}.
+ * This ID is a unique scalar which distinguishes linters in a list.
+ *
+ * @return string Unique linter ID.
+ * @task state
+ */
+ final public function getLinterID() {
+ return $this->id;
+ }
+
+
+/* -( Executing Linters )-------------------------------------------------- */
+
+
+ /**
+ * Hook called before a list of paths are linted.
+ *
+ * Parallelizable linters can start multiple requests in parallel here,
+ * to improve performance. They can implement @{method:didLintPaths} to
+ * collect results.
+ *
+ * Linters which are not parallelizable should normally ignore this callback
+ * and implement @{method:lintPath} instead.
+ *
+ * @param list<string> A list of paths to be linted
+ * @return void
+ * @task exec
+ */
+ public function willLintPaths(array $paths) {
+ return;
+ }
+
+
+ /**
+ * Hook called for each path to be linted.
+ *
+ * Linters which are not parallelizable can do work here.
+ *
+ * Linters which are parallelizable may want to ignore this callback and
+ * implement @{method:willLintPaths} and @{method:didLintPaths} instead.
+ *
+ * @param string Path to lint.
+ * @return void
+ * @task exec
+ */
+ public function lintPath($path) {
+ return;
+ }
+
+
+ /**
+ * Hook called after a list of paths are linted.
+ *
+ * Parallelizable linters can collect results here.
+ *
+ * Linters which are not paralleizable should normally ignore this callback
+ * and implement @{method:lintPath} instead.
+ *
+ * @param list<string> A list of paths which were linted.
+ * @return void
+ * @task exec
+ */
+ public function didLintPaths(array $paths) {
+ return;
+ }
+
+
+ /**
+ * Obsolete hook which was invoked before a path was linted.
+ *
+ * WARNING: This is an obsolete hook which is not called. If you maintain
+ * a linter which relies on it, update to use @{method:lintPath} instead.
+ *
+ * @task exec
+ */
+ final public function willLintPath($path) {
+ // TODO: Remove this method after some time. In the meantime, the "final"
+ // will fatal subclasses which implement this hook and point at the API
+ // change so maintainers get fewer surprises.
+ throw new PhutilMethodNotImplementedException();
+ }
+
+
+ /**
+ * Obsolete hook which was invoked after linters ran.
+ *
+ * WARNING: This is an obsolete hook which is not called. If you maintain
+ * a linter which relies on it, update to use @{method:didLintPaths} instead.
+ *
+ * @return void
+ * @task exec
+ */
+ final public function didRunLinters() {
+ // TODO: Remove this method after some time. In the meantime, the "final"
+ // will fatal subclasses which implement this hook and point at the API
+ // change so maintainers get fewer surprises.
+ throw new PhutilMethodNotImplementedException();
+ }
+
+
public function getLinterPriority() {
return 1.0;
}
@@ -86,10 +247,6 @@
return $this;
}
- final public function getActivePath() {
- return $this->activePath;
- }
-
final public function getOtherLocation($offset, $path = null) {
if ($path === null) {
$path = $this->getActivePath();
@@ -172,19 +329,11 @@
return $this->data[$path];
}
- final public function setEngine(ArcanistLintEngine $engine) {
- $this->engine = $engine;
- return $this;
- }
-
- final protected function getEngine() {
- return $this->engine;
- }
-
public function getCacheVersion() {
return 0;
}
+
final public function getLintMessageFullCode($short_code) {
return $this->getLinterName().$short_code;
}
@@ -291,30 +440,16 @@
$replacement);
}
- public function willLintPath($path) {
- $this->stopAllLinters = false;
- $this->activePath = $path;
- }
-
public function canRun() {
return true;
}
- public function willLintPaths(array $paths) {
- return;
- }
-
- abstract public function lintPath($path);
abstract public function getLinterName();
public function getVersion() {
return null;
}
- public function didRunLinters() {
- // This is a hook.
- }
-
final protected function isCodeEnabled($code) {
$severity = $this->getLintMessageSeverity($code);
return $this->getEngine()->isSeverityEnabled($severity);
diff --git a/src/lint/linter/__tests__/ArcanistLinterTestCase.php b/src/lint/linter/__tests__/ArcanistLinterTestCase.php
--- a/src/lint/linter/__tests__/ArcanistLinterTestCase.php
+++ b/src/lint/linter/__tests__/ArcanistLinterTestCase.php
@@ -113,11 +113,12 @@
$engine = new ArcanistUnitTestableLintEngine();
$engine->setWorkingCopy($working_copy);
$engine->setConfigurationManager($configuration_manager);
- $engine->setPaths(array($path));
$engine->setCommitHookMode(idx($config, 'hook', false));
$path_name = idx($config, 'path', $path);
+ $engine->setPaths(array($path_name));
+
$linter->addPath($path_name);
$linter->addData($path_name, $data);
@@ -129,7 +130,6 @@
$engine->addFileData($path_name, $data);
$results = $engine->run();
-
$this->assertEqual(
1,
count($results),

File Metadata

Mime Type
text/plain
Expires
Thu, May 23, 12:36 AM (3 w, 4 d ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6289931
Default Alt Text
D12501.diff (19 KB)

Event Timeline

Phabricator · Built by Phacility