From 61753b677096d411944e8bff42dd3ecb78cfcf8d Mon Sep 17 00:00:00 2001 From: James Rhodes Date: Mon, 18 Nov 2013 11:32:58 -0800 Subject: [PATCH] Upgrade C# linter to support linting multiple files at a time Summary: This upgrades `cslint` to support linting multiple files at a time. As this required a backwards-incompatible to `cslint`, I've added a SUPPORTED_VERSION constant which can be used to detect these kinds of breaking changes in the future (and prompt users to upgrade the `cslint` they have in their repository). The reason for this upgrade is mainly around running `arc lint --everything`, where there are significant performance benefits gained when bulk linting lots of files per command execution. Test Plan: Upgraded `cslint` in the Tychaia repository and ran `arc lint --everything --trace`. Saw a substantially less number of executions happening for linting and all of the results came through as expected. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley CC: Korvin, epriestley, aran, waynea Differential Revision: https://secure.phabricator.com/D7599 --- src/lint/linter/ArcanistCSharpLinter.php | 168 ++++++++++++++++------- 1 file changed, 116 insertions(+), 52 deletions(-) diff --git a/src/lint/linter/ArcanistCSharpLinter.php b/src/lint/linter/ArcanistCSharpLinter.php index 54273362..141365c8 100644 --- a/src/lint/linter/ArcanistCSharpLinter.php +++ b/src/lint/linter/ArcanistCSharpLinter.php @@ -5,13 +5,16 @@ * * @group linter */ -final class ArcanistCSharpLinter extends ArcanistFutureLinter { +final class ArcanistCSharpLinter extends ArcanistLinter { private $runtimeEngine; private $cslintEngine; private $cslintHintPath; private $loaded; private $discoveryMap; + private $futures; + + const SUPPORTED_VERSION = 1; public function getLinterName() { return 'C#'; @@ -101,74 +104,135 @@ final class ArcanistCSharpLinter extends ArcanistFutureLinter { throw new Exception("Unable to locate cslint."); } + // Determine cslint version. + $ver_future = new ExecFuture( + "%C -v", + $this->runtimeEngine.$this->cslintEngine); + list($err, $stdout, $stderr) = $ver_future->resolve(); + if ($err !== 0) { + throw new Exception( + "You are running an old version of cslint. Please ". + "upgrade to version ".self::SUPPORTED_VERSION."."); + } + $ver = (int)$stdout; + if ($ver < self::SUPPORTED_VERSION) { + throw new Exception( + "You are running an old version of cslint. Please ". + "upgrade to version ".self::SUPPORTED_VERSION."."); + } elseif ($ver > self::SUPPORTED_VERSION) { + throw new Exception( + "Arcanist does not support this version of cslint (it is ". + "newer). You can try upgrading Arcanist with `arc upgrade`."); + } + $this->loaded = true; } - protected function buildFutures(array $paths) { + public function lintPath($path) { + } + + public function willLintPaths(array $paths) { $this->loadEnvironment(); $futures = array(); + // Bulk linting up into futures, where the number of files + // is based on how long the command is. + $current_paths = array(); foreach ($paths as $path) { - // %s won't pass through the JSON correctly - // under Windows. This is probably because not only - // does the JSON have quotation marks in the content, - // but because there'll be a lot of escaping and - // double escaping because the JSON also contains - // regular expressions. cslint supports passing the - // settings JSON through base64-encoded to mitigate - // this issue. - $futures[$path] = new ExecFuture( - "%C --settings-base64=%s -r=. %s", + // If the current paths for the command, plus the next path + // is greater than 6000 characters (less than the Windows + // command line limit), then finalize this future and add it. + $total = 0; + foreach ($current_paths as $current_path) { + $total += strlen($current_path) + 3; // Quotes and space. + } + if ($total + strlen($path) > 6000) { + // %s won't pass through the JSON correctly + // under Windows. This is probably because not only + // does the JSON have quotation marks in the content, + // but because there'll be a lot of escaping and + // double escaping because the JSON also contains + // regular expressions. cslint supports passing the + // settings JSON through base64-encoded to mitigate + // this issue. + $futures[] = new ExecFuture( + "%C --settings-base64=%s -r=. %Ls", + $this->runtimeEngine.$this->cslintEngine, + base64_encode(json_encode($this->discoveryMap)), + $current_paths); + $current_paths = array(); + } + + // Append the path to the current paths array. + $current_paths[] = $this->getEngine()->getFilePathOnDisk($path); + } + + // If we still have paths left in current paths, then we need to create + // a future for those too. + if (count($current_paths) > 0) { + $futures[] = new ExecFuture( + "%C --settings-base64=%s -r=. %Ls", $this->runtimeEngine.$this->cslintEngine, base64_encode(json_encode($this->discoveryMap)), - $this->getEngine()->getFilePathOnDisk($path)); + $current_paths); + $current_paths = array(); } - return $futures; + $this->futures = $futures; } - protected function resolveFuture($path, Future $future) { - list($rc, $stdout) = $future->resolve(); - $results = json_decode($stdout); - if ($results === null || $results->Issues === null) { - return; + public function didRunLinters() { + if ($this->futures) { + foreach (Futures($this->futures) as $future) { + $this->resolveFuture($future); + } } - foreach ($results->Issues as $issue) { - $message = new ArcanistLintMessage(); - $message->setPath($path); - $message->setLine($issue->LineNumber); - $message->setCode($issue->Index->Code); - $message->setName($issue->Index->Name); - $message->setChar($issue->Column); - $message->setOriginalText($issue->OriginalText); - $message->setReplacementText($issue->ReplacementText); - $message->setDescription( - vsprintf($issue->Index->Message, $issue->Parameters)); - $severity = ArcanistLintSeverity::SEVERITY_ADVICE; - switch ($issue->Index->Severity) { - case 0: - $severity = ArcanistLintSeverity::SEVERITY_ADVICE; - break; - case 1: - $severity = ArcanistLintSeverity::SEVERITY_AUTOFIX; - break; - case 2: - $severity = ArcanistLintSeverity::SEVERITY_WARNING; - break; - case 3: - $severity = ArcanistLintSeverity::SEVERITY_ERROR; - break; - case 4: - $severity = ArcanistLintSeverity::SEVERITY_DISABLED; - break; + } + + protected function resolveFuture(Future $future) { + list($stdout) = $future->resolvex(); + $all_results = json_decode($stdout); + foreach ($all_results as $results) { + if ($results === null || $results->Issues === null) { + return; } - $severity_override = $this->getLintMessageSeverity($issue->Index->Code); - if ($severity_override !== null) { - $severity = $severity_override; + foreach ($results->Issues as $issue) { + $message = new ArcanistLintMessage(); + $message->setPath($results->FileName); + $message->setLine($issue->LineNumber); + $message->setCode($issue->Index->Code); + $message->setName($issue->Index->Name); + $message->setChar($issue->Column); + $message->setOriginalText($issue->OriginalText); + $message->setReplacementText($issue->ReplacementText); + $message->setDescription( + vsprintf($issue->Index->Message, $issue->Parameters)); + $severity = ArcanistLintSeverity::SEVERITY_ADVICE; + switch ($issue->Index->Severity) { + case 0: + $severity = ArcanistLintSeverity::SEVERITY_ADVICE; + break; + case 1: + $severity = ArcanistLintSeverity::SEVERITY_AUTOFIX; + break; + case 2: + $severity = ArcanistLintSeverity::SEVERITY_WARNING; + break; + case 3: + $severity = ArcanistLintSeverity::SEVERITY_ERROR; + break; + case 4: + $severity = ArcanistLintSeverity::SEVERITY_DISABLED; + break; + } + $severity_override = $this->getLintMessageSeverity($issue->Index->Code); + if ($severity_override !== null) { + $severity = $severity_override; + } + $message->setSeverity($severity); + $this->addLintMessage($message); } - $message->setSeverity($severity); - $this->addLintMessage($message); } }