From 0f30aca626f2477653062893558c9002aad803cf Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 26 Aug 2013 05:37:10 -0700 Subject: [PATCH] Ready more linters and linter functions for .arclint Summary: Ref T3186. Ref T2039. Continues work on readying linters for `.arclint`. - **Ruby**: Make this an ExternalLinter. - **Priority**: Currently, linters have an implicit "correct" order (notably, the "NoLint" linter needs to run before other linters). Make this explicit by introducing `getLinterPriority()`. - **Binaries**: Currently, linters manually reject binary files. Instead, reject binary files by default (linters can override this if they do want to lint binary files). - **Deleted Files**: Currently, linters manually reject deleted files (usually in engines). Instead, reject deleted files by default (linters can override this). - **Severity**: Move this `.arclint` config option up to top level. - **willLintPaths()**: This method is abstract, but almost all linters provide a trivial implementation. Provide a trivial implementation in the base class. - **getLintSeverityMap()/getLintNameMap()**: A bunch of linters have empty implementations; these are redundant. Remove them. - **Spelling**: clean up some dead / test-only / unconventional code. - **`.arclint`**: Allow the filename, generated, nolint, text, spelling and ruby linters to be configured via `.arclint`. Test Plan: https://github.com/epriestley/arclint-examples/commit/458beca3d65b64d52ed612904ae66eb837118b94 Ran unit tests. Reviewers: btrahan Reviewed By: btrahan CC: Firehed, aran Maniphest Tasks: T2039, T3186 Differential Revision: https://secure.phabricator.com/D6805 --- src/__phutil_library_map__.php | 2 +- src/lint/engine/ArcanistLintEngine.php | 8 +- src/lint/engine/ComprehensiveLintEngine.php | 3 - src/lint/engine/PhutilLintEngine.php | 5 ++ src/lint/linter/ArcanistExternalLinter.php | 28 +----- src/lint/linter/ArcanistFilenameLinter.php | 19 ++-- src/lint/linter/ArcanistGeneratedLinter.php | 18 +--- src/lint/linter/ArcanistLinter.php | 72 +++++++++++++++- src/lint/linter/ArcanistNoLintLinter.php | 18 ++-- src/lint/linter/ArcanistRubyLinter.php | 86 +++++++++---------- src/lint/linter/ArcanistScalaSBTLinter.php | 12 --- src/lint/linter/ArcanistSpellingLinter.php | 38 +++----- src/lint/linter/ArcanistTextLinter.php | 30 +++---- .../ArcanistSpellingLinterTestCase.php | 1 - .../linter/__tests__/spelling/spell.lint-test | 2 +- 15 files changed, 171 insertions(+), 171 deletions(-) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 627d50f7..1808e2e9 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -269,7 +269,7 @@ phutil_register_library_map(array( 'ArcanistRepositoryAPIMiscTestCase' => 'ArcanistTestCase', 'ArcanistRepositoryAPIStateTestCase' => 'ArcanistTestCase', 'ArcanistRevertWorkflow' => 'ArcanistBaseWorkflow', - 'ArcanistRubyLinter' => 'ArcanistLinter', + 'ArcanistRubyLinter' => 'ArcanistExternalLinter', 'ArcanistRubyLinterTestCase' => 'ArcanistArcanistLinterTestCase', 'ArcanistScalaSBTLinter' => 'ArcanistLinter', 'ArcanistScriptAndRegexLinter' => 'ArcanistLinter', diff --git a/src/lint/engine/ArcanistLintEngine.php b/src/lint/engine/ArcanistLintEngine.php index 3cb5dbc5..17ed0f1c 100644 --- a/src/lint/engine/ArcanistLintEngine.php +++ b/src/lint/engine/ArcanistLintEngine.php @@ -167,11 +167,15 @@ abstract class ArcanistLintEngine { public function run() { $linters = $this->buildLinters(); - if (!$linters) { throw new ArcanistNoEffectException("No linters to run."); } + $linters = msort($linters, 'getLinterPriority'); + foreach ($linters as $linter) { + $linter->setEngine($this); + } + $have_paths = false; foreach ($linters as $linter) { if ($linter->getPaths()) { @@ -187,7 +191,6 @@ abstract class ArcanistLintEngine { $versions = array($this->getCacheVersion()); foreach ($linters as $linter) { - $linter->setEngine($this); $version = get_class($linter).':'.$linter->getCacheVersion(); $symbols = id(new PhutilSymbolLoader()) @@ -507,4 +510,5 @@ abstract class ArcanistLintEngine { return '--ignore=E101,E501,W291,W292,W293'; } + } diff --git a/src/lint/engine/ComprehensiveLintEngine.php b/src/lint/engine/ComprehensiveLintEngine.php index e8a104d2..06555014 100644 --- a/src/lint/engine/ComprehensiveLintEngine.php +++ b/src/lint/engine/ComprehensiveLintEngine.php @@ -13,9 +13,6 @@ final class ComprehensiveLintEngine extends ArcanistLintEngine { $paths = $this->getPaths(); foreach ($paths as $key => $path) { - if (!$this->pathExists($path)) { - unset($paths[$key]); - } if (preg_match('@^externals/@', $path)) { // Third-party stuff lives in /externals/; don't run lint engines // against it. diff --git a/src/lint/engine/PhutilLintEngine.php b/src/lint/engine/PhutilLintEngine.php index 6328f68f..968f9b20 100644 --- a/src/lint/engine/PhutilLintEngine.php +++ b/src/lint/engine/PhutilLintEngine.php @@ -26,6 +26,11 @@ class PhutilLintEngine extends ArcanistLintEngine { // against it. unset($paths[$key]); } + if (preg_match('(\\.lint-test$)', $path)) { + // Don't try to lint these, since they're tests for linters and + // often have intentional lint errors. + unset($paths[$key]); + } } $linters[] = id(new ArcanistFilenameLinter())->setPaths($paths); diff --git a/src/lint/linter/ArcanistExternalLinter.php b/src/lint/linter/ArcanistExternalLinter.php index 88cda3ed..b7031b66 100644 --- a/src/lint/linter/ArcanistExternalLinter.php +++ b/src/lint/linter/ArcanistExternalLinter.php @@ -411,14 +411,13 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { $options = array( 'bin' => 'optional string | list', 'flags' => 'optional string', - 'severity' => 'optional map', ); if ($this->shouldUseInterpreter()) { $options['interpreter'] = 'optional string | list'; } - return $options; + return $options + parent::getLinterConfigurationOptions(); } public function setLinterConfigurationValue($key, $value) { @@ -470,31 +469,6 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter { $this->setFlags($value); } return; - case 'severity': - $sev_map = array( - 'error' => ArcanistLintSeverity::SEVERITY_ERROR, - 'warning' => ArcanistLintSeverity::SEVERITY_WARNING, - 'autofix' => ArcanistLintSeverity::SEVERITY_AUTOFIX, - 'advice' => ArcanistLintSeverity::SEVERITY_ADVICE, - 'disabled' => ArcanistLintSeverity::SEVERITY_DISABLED, - ); - - $custom = array(); - foreach ($value as $code => $severity) { - if (empty($sev_map[$severity])) { - $valid = implode(', ', array_keys($sev_map)); - throw new Exception( - pht( - 'Unknown lint severity "%s". Valid severities are: %s.', - $severity, - $valid)); - } - $code = $this->getLintCodeFromLinterConfigurationKey($code); - $custom[$code] = $severity; - } - - $this->setCustomSeverityMap($custom); - return; } return parent::setLinterConfigurationValue($key, $value); diff --git a/src/lint/linter/ArcanistFilenameLinter.php b/src/lint/linter/ArcanistFilenameLinter.php index 48fedf38..9aeffdf5 100644 --- a/src/lint/linter/ArcanistFilenameLinter.php +++ b/src/lint/linter/ArcanistFilenameLinter.php @@ -9,25 +9,21 @@ final class ArcanistFilenameLinter extends ArcanistLinter { const LINT_BAD_FILENAME = 1; - public function willLintPaths(array $paths) { - return; - } - public function getLinterName() { return 'NAME'; } - public function getLintSeverityMap() { - return array(); - } - public function getLinterConfigurationName() { return 'filename'; } + public function shouldLintBinaryFiles() { + return true; + } + public function getLintNameMap() { return array( - self::LINT_BAD_FILENAME => 'Bad Filename', + self::LINT_BAD_FILENAME => pht('Bad Filename'), ); } @@ -35,8 +31,9 @@ final class ArcanistFilenameLinter extends ArcanistLinter { if (!preg_match('@^[a-z0-9./\\\\_-]+$@i', $path)) { $this->raiseLintAtPath( self::LINT_BAD_FILENAME, - 'Name files using only letters, numbers, period, hyphen and '. - 'underscore.'); + pht( + 'Name files using only letters, numbers, period, hyphen and '. + 'underscore.')); } } diff --git a/src/lint/linter/ArcanistGeneratedLinter.php b/src/lint/linter/ArcanistGeneratedLinter.php index b6d49ad6..6efdfb15 100644 --- a/src/lint/linter/ArcanistGeneratedLinter.php +++ b/src/lint/linter/ArcanistGeneratedLinter.php @@ -7,30 +7,20 @@ */ final class ArcanistGeneratedLinter extends ArcanistLinter { - public function willLintPaths(array $paths) { - return; - } - public function getLinterName() { return 'GEN'; } - public function getLintSeverityMap() { - return array(); + public function getLinterPriority() { + return 0.25; } - public function getLintNameMap() { - return array( - ); + public function getLinterConfigurationName() { + return 'generated'; } public function lintPath($path) { - if ($this->isBinaryFile($path)) { - return; - } - $data = $this->getData($path); - if (preg_match('/@'.'generated/', $data)) { $this->stopAllLinters(); } diff --git a/src/lint/linter/ArcanistLinter.php b/src/lint/linter/ArcanistLinter.php index 69636d9b..7cbd309c 100644 --- a/src/lint/linter/ArcanistLinter.php +++ b/src/lint/linter/ArcanistLinter.php @@ -24,6 +24,10 @@ abstract class ArcanistLinter { private $customSeverityMap = array(); private $config = array(); + public function getLinterPriority() { + return 1.0; + } + public function setCustomSeverityMap(array $map) { $this->customSeverityMap = $map; return $this; @@ -77,8 +81,29 @@ abstract class ArcanistLinter { return $this; } + /** + * Filter out paths which this linter doesn't act on (for example, because + * they are binaries and the linter doesn't apply to binaries). + */ + private function filterPaths($paths) { + $engine = $this->getEngine(); + + $keep = array(); + foreach ($paths as $path) { + if (!$this->shouldLintDeletedFiles() && !$engine->pathExists($path)) { + continue; + } + if (!$this->shouldLintBinaryFiles() && $this->isBinaryFile($path)) { + continue; + } + $keep[] = $path; + } + + return $keep; + } + public function getPaths() { - return array_values($this->paths); + return $this->filterPaths(array_values($this->paths)); } public function addData($path, $data) { @@ -218,7 +243,10 @@ abstract class ArcanistLinter { return true; } - abstract public function willLintPaths(array $paths); + public function willLintPaths(array $paths) { + return; + } + abstract public function lintPath($path); abstract public function getLinterName(); @@ -261,11 +289,49 @@ abstract class ArcanistLinter { } public function getLinterConfigurationOptions() { - return array(); + return array( + 'severity' => 'optional map', + ); } public function setLinterConfigurationValue($key, $value) { + switch ($key) { + case 'severity': + $sev_map = array( + 'error' => ArcanistLintSeverity::SEVERITY_ERROR, + 'warning' => ArcanistLintSeverity::SEVERITY_WARNING, + 'autofix' => ArcanistLintSeverity::SEVERITY_AUTOFIX, + 'advice' => ArcanistLintSeverity::SEVERITY_ADVICE, + 'disabled' => ArcanistLintSeverity::SEVERITY_DISABLED, + ); + + $custom = array(); + foreach ($value as $code => $severity) { + if (empty($sev_map[$severity])) { + $valid = implode(', ', array_keys($sev_map)); + throw new Exception( + pht( + 'Unknown lint severity "%s". Valid severities are: %s.', + $severity, + $valid)); + } + $code = $this->getLintCodeFromLinterConfigurationKey($code); + $custom[$code] = $severity; + } + + $this->setCustomSeverityMap($custom); + return; + } + throw new Exception("Incomplete implementation: {$key}!"); } + protected function shouldLintBinaryFiles() { + return false; + } + + protected function shouldLintDeletedFiles() { + return false; + } + } diff --git a/src/lint/linter/ArcanistNoLintLinter.php b/src/lint/linter/ArcanistNoLintLinter.php index d302d2a2..04fac3d3 100644 --- a/src/lint/linter/ArcanistNoLintLinter.php +++ b/src/lint/linter/ArcanistNoLintLinter.php @@ -7,32 +7,24 @@ * @group linter */ final class ArcanistNoLintLinter extends ArcanistLinter { - public function willLintPaths(array $paths) { - return; - } public function getLinterName() { return 'NOLINT'; } - public function getLintSeverityMap() { - return array(); + public function getLinterPriority() { + return 0.25; } - public function getLintNameMap() { - return array( - ); + public function getLinterConfigurationName() { + return 'nolint'; } public function lintPath($path) { - if ($this->isBinaryFile($path)) { - return; - } - $data = $this->getData($path); - if (preg_match('/@'.'nolint/', $data)) { $this->stopAllLinters(); } } + } diff --git a/src/lint/linter/ArcanistRubyLinter.php b/src/lint/linter/ArcanistRubyLinter.php index e6e1e7d5..24a9ec7c 100644 --- a/src/lint/linter/ArcanistRubyLinter.php +++ b/src/lint/linter/ArcanistRubyLinter.php @@ -1,72 +1,64 @@ getEngine()->getWorkingCopy(); $prefix = $working_copy->getConfig('lint.ruby.prefix'); if ($prefix !== null) { - $ruby_bin = $prefix . $ruby_bin; + $ruby_bin = $prefix.'ruby'; } - if (!Filesystem::pathExists($ruby_bin)) { - - list($err) = exec_manual('which %s', $ruby_bin); - if ($err) { - throw new ArcanistUsageException( - "Ruby does not appear to be installed on this system. Install it or ". - "add 'lint.ruby.prefix' in your .arcconfig to point to ". - "the directory where it resides."); - } - } - - return $ruby_bin; + return 'ruby'; } - private function getMessageCodeSeverity($code) { + public function getInstallInstructions() { + return pht('Install `ruby` from .'); + } + + public function supportsReadDataFromStdin() { + return true; + } + + public function shouldExpectCommandErrors() { + return true; + } + + protected function getMandatoryFlags() { + // -w: turn on warnings + // -c: check syntax + return '-w -c'; + } + + protected function getDefaultMessageSeverity($code) { return ArcanistLintSeverity::SEVERITY_ERROR; } - public function lintPath($path) { - $rubyp = $this->getRubyPath(); - $f = new ExecFuture("%s -wc", $rubyp); - $f->write($this->getData($path)); - list($err, $stdout, $stderr) = $f->resolve(); - if ($err === 0 ) { - return; - } + protected function parseLinterOutput($path, $err, $stdout, $stderr) { + $lines = phutil_split_lines($stderr, $retain_endings = false); - $lines = explode("\n", $stderr); $messages = array(); foreach ($lines as $line) { $matches = null; + if (!preg_match("/(.*?):(\d+): (.*?)$/", $line, $matches)) { continue; } + foreach ($matches as $key => $match) { $matches[$key] = trim($match); } @@ -76,11 +68,19 @@ final class ArcanistRubyLinter extends ArcanistLinter { $message = new ArcanistLintMessage(); $message->setPath($path); $message->setLine($matches[2]); - $message->setName($this->getLinterName() . " " . $code); + $message->setCode($this->getLinterName()); + $message->setName(pht('Syntax Error')); $message->setDescription($matches[3]); - $message->setSeverity($this->getMessageCodeSeverity($code)); - $this->addLintMessage($message); + $message->setSeverity($this->getLintMessageSeverity($code)); + + $messages[] = $message; } + + if ($err && !$messages) { + return false; + } + + return $messages; } } diff --git a/src/lint/linter/ArcanistScalaSBTLinter.php b/src/lint/linter/ArcanistScalaSBTLinter.php index 8081d645..10aabbb2 100644 --- a/src/lint/linter/ArcanistScalaSBTLinter.php +++ b/src/lint/linter/ArcanistScalaSBTLinter.php @@ -7,22 +7,10 @@ */ final class ArcanistScalaSBTLinter extends ArcanistLinter { - public function willLintPaths(array $paths) { - return; - } - public function getLinterName() { return 'ScalaSBT'; } - public function getLintSeverityMap() { - return array(); - } - - public function getLintNameMap() { - return array(); - } - public function canRun() { // Check if this looks like a SBT project. If it doesn't, throw, because // we rely fairly heavily on `sbt compile` working, below. We don't want diff --git a/src/lint/linter/ArcanistSpellingLinter.php b/src/lint/linter/ArcanistSpellingLinter.php index 1898e75d..1f940f04 100644 --- a/src/lint/linter/ArcanistSpellingLinter.php +++ b/src/lint/linter/ArcanistSpellingLinter.php @@ -24,35 +24,27 @@ final class ArcanistSpellingLinter extends ArcanistLinter { ArcanistSpellingDefaultData::getPartialWordRules(); } - public function willLintPaths(array $paths) { - return; - } - public function getLinterName() { return 'SPELL'; } - public function removeLintRule($word) { - foreach ($this->partialWordRules as $severity=>&$wordlist) { - unset($wordlist[$word]); - } - - foreach ($this->wholeWordRules as $severity=>&$wordlist) { - unset($wordlist[$word]); - } + public function getLinterConfigurationName() { + return 'spelling'; } public function addPartialWordRule( - $incorrect_word, - $correct_word, - $severity=self::LINT_SPELLING_IMPORTANT) { + $incorrect_word, + $correct_word, + $severity = self::LINT_SPELLING_IMPORTANT) { + $this->partialWordRules[$severity][$incorrect_word] = $correct_word; } public function addWholeWordRule( - $incorrect_word, - $correct_word, - $severity=self::LINT_SPELLING_IMPORTANT) { + $incorrect_word, + $correct_word, + $severity = self::LINT_SPELLING_IMPORTANT) { + $this->wholeWordRules[$severity][$incorrect_word] = $correct_word; } @@ -65,16 +57,12 @@ final class ArcanistSpellingLinter extends ArcanistLinter { public function getLintNameMap() { return array( - self::LINT_SPELLING_PICKY => 'Possible spelling mistake', - self::LINT_SPELLING_IMPORTANT => 'Possible spelling mistake', + self::LINT_SPELLING_PICKY => pht('Possible Spelling Mistake'), + self::LINT_SPELLING_IMPORTANT => pht('Possible Spelling Mistake'), ); } public function lintPath($path) { - if ($this->isBinaryFile($path)) { - return; - } - foreach ($this->partialWordRules as $severity => $wordlist) { if ($severity >= $this->severity) { if (!$this->isCodeEnabled($severity)) { @@ -125,7 +113,7 @@ final class ArcanistSpellingLinter extends ArcanistLinter { $text = $this->getData($path); $matches = array(); $num_matches = preg_match_all( - '#\b' . preg_quote($word, '#') . '\b#i', + '#\b'.preg_quote($word, '#').'\b#i', $text, $matches, PREG_OFFSET_CAPTURE); diff --git a/src/lint/linter/ArcanistTextLinter.php b/src/lint/linter/ArcanistTextLinter.php index fc1425fe..4fdbef48 100644 --- a/src/lint/linter/ArcanistTextLinter.php +++ b/src/lint/linter/ArcanistTextLinter.php @@ -17,19 +17,23 @@ final class ArcanistTextLinter extends ArcanistLinter { private $maxLineLength = 80; + public function getLinterPriority() { + return 0.5; + } + public function setMaxLineLength($new_length) { $this->maxLineLength = $new_length; return $this; } - public function willLintPaths(array $paths) { - return; - } - public function getLinterName() { return 'TXT'; } + public function getLinterConfigurationName() { + return 'text'; + } + public function getLintSeverityMap() { return array( self::LINT_LINE_WRAP => ArcanistLintSeverity::SEVERITY_WARNING, @@ -39,21 +43,17 @@ final class ArcanistTextLinter extends ArcanistLinter { public function getLintNameMap() { return array( - self::LINT_DOS_NEWLINE => 'DOS Newlines', - self::LINT_TAB_LITERAL => 'Tab Literal', - self::LINT_LINE_WRAP => 'Line Too Long', - self::LINT_EOF_NEWLINE => 'File Does Not End in Newline', - self::LINT_BAD_CHARSET => 'Bad Charset', - self::LINT_TRAILING_WHITESPACE => 'Trailing Whitespace', - self::LINT_NO_COMMIT => 'Explicit @no'.'commit', + self::LINT_DOS_NEWLINE => pht('DOS Newlines'), + self::LINT_TAB_LITERAL => pht('Tab Literal'), + self::LINT_LINE_WRAP => pht('Line Too Long'), + self::LINT_EOF_NEWLINE => pht('File Does Not End in Newline'), + self::LINT_BAD_CHARSET => pht('Bad Charset'), + self::LINT_TRAILING_WHITESPACE => pht('Trailing Whitespace'), + self::LINT_NO_COMMIT => pht('Explicit %s', '@no'.'commit'), ); } public function lintPath($path) { - if ($this->isBinaryFile($path)) { - return; - } - if (!strlen($this->getData($path))) { // If the file is empty, don't bother; particularly, don't require // the user to add a newline. diff --git a/src/lint/linter/__tests__/ArcanistSpellingLinterTestCase.php b/src/lint/linter/__tests__/ArcanistSpellingLinterTestCase.php index a8c26970..667058fa 100644 --- a/src/lint/linter/__tests__/ArcanistSpellingLinterTestCase.php +++ b/src/lint/linter/__tests__/ArcanistSpellingLinterTestCase.php @@ -10,7 +10,6 @@ final class ArcanistSpellingLinterTestCase public function testSpellingLint() { $linter = new ArcanistSpellingLinter(); - $linter->removeLintRule('acc'.'out'); $linter->addPartialWordRule('supermn', 'superman'); $linter->addWholeWordRule('batmn', 'batman'); diff --git a/src/lint/linter/__tests__/spelling/spell.lint-test b/src/lint/linter/__tests__/spelling/spell.lint-test index bae2ca2d..aa1a5f1f 100644 --- a/src/lint/linter/__tests__/spelling/spell.lint-test +++ b/src/lint/linter/__tests__/spelling/spell.lint-test @@ -5,7 +5,7 @@ $y = $x->recieveData(); for (i=0; i