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:
458beca3d6

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

@@ -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',

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';
   }
 
+
 }

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.

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);

src/lint/linter/ArcanistExternalLinter.php

@@ -411,14 +411,13 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter {
     $options = array(
       'bin' => 'optional string | list<string>',
       'flags' => 'optional string',
-      'severity' => 'optional map<string, string>',
     );
 
     if ($this->shouldUseInterpreter()) {
       $options['interpreter'] = 'optional string | list<string>';
     }
 
-    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);

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.'));
     }
   }
 

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();
     }

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<string, string>',
+    );
   }
 
   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;
+  }
+
 }

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();
     }
   }
+
 }

src/lint/linter/ArcanistRubyLinter.php

@@ -1,72 +1,64 @@
 <?php
 
 /**
- * Uses "Ruby" to detect various errors in Ruby code.
+ * Uses `ruby` to detect various errors in Ruby code.
  *
  * @group linter
  */
-final class ArcanistRubyLinter extends ArcanistLinter {
-
-  public function willLintPaths(array $paths) {
-    return;
-  }
+final class ArcanistRubyLinter extends ArcanistExternalLinter {
 
   public function getLinterName() {
-    return 'Ruby';
+    return 'RUBY';
   }
 
-  public function getLintSeverityMap() {
-    return array();
+  public function getLinterConfigurationName() {
+    return 'ruby';
   }
 
-  public function getLintNameMap() {
-    return array();
-  }
-
-  private function getRubyPath() {
-    $ruby_bin = "ruby";
-
-    // Use the Ruby prefix specified in the config file
+  public function getDefaultBinary() {
+    // TODO: Deprecation warning.
     $working_copy = $this->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 <http://www.ruby-lang.org/>.');
+  }
+
+  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;
   }
 
 }

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

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);

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.

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');
 

src/lint/linter/__tests__/spelling/spell.lint-test

@@ -5,7 +5,7 @@ $y = $x->recieveData();
 for (i=0; i<y.lenght; i++) {
 }
 Check uFT8 ufT8 (<-- repeated)
-removed accout d
+
 didn't remove acording
 Added ZZZZsupermnZZZZ
 Added full batmn batmnZZZZ