Support PHPCS as a .arclint linter

Summary:
Ref T3186. Ref T2039. Ref T3771. A few effects here:

  # Expose PHPCS as a `.arclint` linter.
  # Turn PHPCS into an ArcanistExternalLinter linter.
  # Add test coverage for PHPCS.
  # Add a `severity.rules` option to `.arclint`. Some linters have very explicit builtin severities ("error", "warning") but their meanings are different from how arc interprets these terms. For example, PHPCS raises "wrong indentation level" as an "error". You can already use the "severity" map to adjust individual rules, but if you want to adjust an entire linter it's currently difficult. This rule map makes it easy. There's substantial precedent for this in other linters, notably all the Python linters.

For `severity.rules`, for example, this will turn all PHPCS "errors" into warnings, and all of its warnings into advice:

      "severity.rules" : {
        "(^PHPCS\\.E\\.)" : "warning",
        "(^PHPCS\\.W\\.)" : "advice"
      }

The user can use `severity` (or more rules) to get additional granularity adjustments if they desire.

Test Plan: 5bb919bc3a

Reviewers: btrahan

Reviewed By: btrahan

CC: aran, ajtrichards

Maniphest Tasks: T2039, T3186, T3771

Differential Revision: https://secure.phabricator.com/D6830

src/__phutil_library_map__.php

@@ -110,6 +110,7 @@ phutil_register_library_map(array(
     'ArcanistNoLintTestCaseMisnamed' => 'lint/linter/__tests__/ArcanistNoLintTestCase.php',
     'ArcanistPEP8Linter' => 'lint/linter/ArcanistPEP8Linter.php',
     'ArcanistPEP8LinterTestCase' => 'lint/linter/__tests__/ArcanistPEP8LinterTestCase.php',
+    'ArcanistPHPCSLinterTestCase' => 'lint/linter/__tests__/ArcanistPHPCSLinterTestCase.php',
     'ArcanistPasteWorkflow' => 'workflow/ArcanistPasteWorkflow.php',
     'ArcanistPatchWorkflow' => 'workflow/ArcanistPatchWorkflow.php',
     'ArcanistPhpcsLinter' => 'lint/linter/ArcanistPhpcsLinter.php',
@@ -255,9 +256,10 @@ phutil_register_library_map(array(
     'ArcanistNoLintTestCaseMisnamed' => 'ArcanistLinterTestCase',
     'ArcanistPEP8Linter' => 'ArcanistExternalLinter',
     'ArcanistPEP8LinterTestCase' => 'ArcanistArcanistLinterTestCase',
+    'ArcanistPHPCSLinterTestCase' => 'ArcanistArcanistLinterTestCase',
     'ArcanistPasteWorkflow' => 'ArcanistBaseWorkflow',
     'ArcanistPatchWorkflow' => 'ArcanistBaseWorkflow',
-    'ArcanistPhpcsLinter' => 'ArcanistLinter',
+    'ArcanistPhpcsLinter' => 'ArcanistExternalLinter',
     'ArcanistPhutilLibraryLinter' => 'ArcanistLinter',
     'ArcanistPhutilTestCaseTestCase' => 'ArcanistPhutilTestCase',
     'ArcanistPhutilTestSkippedException' => 'Exception',

src/lint/engine/ArcanistLintEngine.php

@@ -150,6 +150,32 @@ abstract class ArcanistLintEngine {
     }
   }
 
+  public function isDirectory($path) {
+    if ($this->getCommitHookMode()) {
+      // TODO: This won't get the right result in every case (we need more
+      // metadata) but should almost always be correct.
+      try {
+        $this->loadData($path);
+        return false;
+      } catch (Exception $ex) {
+        return true;
+      }
+    } else {
+      $disk_path = $this->getFilePathOnDisk($path);
+      return is_dir($disk_path);
+    }
+  }
+
+  public function isBinaryFile($path) {
+    try {
+      $data = $this->loadData($path);
+    } catch (Exception $ex) {
+      return false;
+    }
+
+    return ArcanistDiffUtils::isHeuristicBinaryFile($data);
+  }
+
   public function getFilePathOnDisk($path) {
     return Filesystem::resolvePath(
       $path,

src/lint/linter/ArcanistFilenameLinter.php

@@ -37,4 +37,8 @@ final class ArcanistFilenameLinter extends ArcanistLinter {
     }
   }
 
+  public function shouldLintDirectories() {
+    return true;
+  }
+
 }

src/lint/linter/ArcanistLinter.php

@@ -22,6 +22,7 @@ abstract class ArcanistLinter {
   protected $stopAllLinters = false;
 
   private $customSeverityMap = array();
+  private $customSeverityRules = array();
   private $config = array();
 
   public function getLinterPriority() {
@@ -33,6 +34,11 @@ abstract class ArcanistLinter {
     return $this;
   }
 
+  public function setCustomSeverityRules(array $rules) {
+    $this->customSeverityRules = $rules;
+    return $this;
+  }
+
   public function setConfig(array $config) {
     $this->config = $config;
     return $this;
@@ -93,9 +99,15 @@ abstract class ArcanistLinter {
       if (!$this->shouldLintDeletedFiles() && !$engine->pathExists($path)) {
         continue;
       }
-      if (!$this->shouldLintBinaryFiles() && $this->isBinaryFile($path)) {
+
+      if (!$this->shouldLintDirectories() && $engine->isDirectory($path)) {
         continue;
       }
+
+      if (!$this->shouldLintBinaryFiles() && $engine->isBinaryFile($path)) {
+        continue;
+      }
+
       $keep[] = $path;
     }
 
@@ -146,6 +158,12 @@ abstract class ArcanistLinter {
       return $map[$code];
     }
 
+    foreach ($this->customSeverityRules as $rule => $severity) {
+      if (preg_match($rule, $code)) {
+        return $severity;
+      }
+    }
+
     return $this->getDefaultMessageSeverity($code);
   }
 
@@ -271,11 +289,6 @@ abstract class ArcanistLinter {
     return self::GRANULARITY_FILE;
   }
 
-  public function isBinaryFile($path) {
-    // Note that we need the lint engine set before this can be used.
-    return ArcanistDiffUtils::isHeuristicBinaryFile($this->getData($path));
-  }
-
   /**
    * If this linter is selectable via `.arclint` configuration files, return
    * a short, human-readable name to identify it. For example, `"jshint"` or
@@ -291,20 +304,21 @@ abstract class ArcanistLinter {
   public function getLinterConfigurationOptions() {
     return array(
       'severity' => 'optional map<string, string>',
+      'severity.rules' => 'optional map<string, string>',
     );
   }
 
   public function setLinterConfigurationValue($key, $value) {
+    $sev_map = array(
+      'error' => ArcanistLintSeverity::SEVERITY_ERROR,
+      'warning' => ArcanistLintSeverity::SEVERITY_WARNING,
+      'autofix' => ArcanistLintSeverity::SEVERITY_AUTOFIX,
+      'advice' => ArcanistLintSeverity::SEVERITY_ADVICE,
+      'disabled' => ArcanistLintSeverity::SEVERITY_DISABLED,
+    );
+
     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])) {
@@ -321,6 +335,25 @@ abstract class ArcanistLinter {
 
         $this->setCustomSeverityMap($custom);
         return;
+      case 'severity.rules':
+        foreach ($value as $rule => $severity) {
+          if (@preg_match($rule, '') === false) {
+            throw new Exception(
+              pht(
+                'Severity rule "%s" is not a valid regular expression.',
+                $rule));
+          }
+          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));
+          }
+        }
+        $this->setCustomSeverityRules($value);
+        return;
     }
 
     throw new Exception("Incomplete implementation: {$key}!");
@@ -334,4 +367,22 @@ abstract class ArcanistLinter {
     return false;
   }
 
+  protected function shouldLintDirectories() {
+    return false;
+  }
+
+  /**
+   * Map a configuration lint code to an `arc` lint code. Primarily, this is
+   * intended for validation, but can also be used to normalize case or
+   * otherwise be more permissive in accepted inputs.
+   *
+   * If the code is not recognized, you should throw an exception.
+   *
+   * @param string  Code specified in configuration.
+   * @return string  Normalized code to use in severity map.
+   */
+  protected function getLintCodeFromLinterConfigurationKey($code) {
+    return $code;
+  }
+
 }

src/lint/linter/ArcanistMergeConflictLinter.php

@@ -13,10 +13,6 @@ final class ArcanistMergeConflictLinter extends ArcanistLinter {
   }
 
   public function lintPath($path) {
-    if ($this->isBinaryFile($path)) {
-      return;
-    }
-
     $lines = phutil_split_lines($this->getData($path), false);
 
     foreach ($lines as $lineno => $line) {

src/lint/linter/ArcanistPhpcsLinter.php

@@ -13,7 +13,7 @@
  *
  * @group linter
  */
-final class ArcanistPhpcsLinter extends ArcanistLinter {
+final class ArcanistPhpcsLinter extends ArcanistExternalLinter {
 
   private $reports;
 
@@ -21,15 +21,21 @@ final class ArcanistPhpcsLinter extends ArcanistLinter {
     return 'PHPCS';
   }
 
-  public function getLintSeverityMap() {
-    return array();
+  public function getLinterConfigurationName() {
+    return 'phpcs';
   }
 
-  public function getLintNameMap() {
-    return array();
+  public function getMandatoryFlags() {
+    return '--report=xml';
   }
 
-  public function getPhpcsOptions() {
+  public function getInstallInstructions() {
+    return pht('Install PHPCS with `pear install PHP_CodeSniffer`.');
+  }
+
+  public function getDefaultFlags() {
+    // TODO: Deprecation warnings.
+
     $working_copy = $this->getEngine()->getWorkingCopy();
 
     $options = $working_copy->getConfig('lint.phpcs.options');
@@ -40,83 +46,73 @@ final class ArcanistPhpcsLinter extends ArcanistLinter {
     return $options;
   }
 
-  private function getPhpcsPath() {
+  public function getDefaultBinary() {
+    // TODO: Deprecation warnings.
     $working_copy = $this->getEngine()->getWorkingCopy();
     $bin = $working_copy->getConfig('lint.phpcs.bin');
-
-    if ($bin === null) {
-      $bin = 'phpcs';
+    if ($bin) {
+      return $bin;
     }
 
-    return $bin;
+    return 'phpcs';
   }
 
-  public function willLintPaths(array $paths) {
-    $phpcs_bin = $this->getPhpcsPath();
-    $phpcs_options = $this->getPhpcsOptions();
-    $futures = array();
-
-    foreach ($paths as $path) {
-      $filepath = $this->getEngine()->getFilePathOnDisk($path);
-      $this->reports[$path] = new TempFile();
-      $futures[$path] = new ExecFuture('%C %C --report=xml --report-file=%s %s',
-        $phpcs_bin,
-        $phpcs_options,
-        $this->reports[$path],
-        $filepath);
-    }
-
-    foreach (Futures($futures)->limit(8) as $path => $future) {
-      $this->results[$path] = $future->resolve();
-    }
-
-    libxml_use_internal_errors(true);
+  public function shouldExpectCommandErrors() {
+    return true;
   }
 
-  public function lintPath($path) {
-    list($rc, $stdout) = $this->results[$path];
-
-    $report = Filesystem::readFile($this->reports[$path]);
-
-    if ($report) {
-      $report_dom = new DOMDocument();
-      libxml_clear_errors();
-      $report_dom->loadXML($report);
-    }
-    if (!$report || libxml_get_errors()) {
-      throw new ArcanistUsageException('PHPCS Linter failed to load ' .
-        'reporting file. Something happened when running phpcs. ' .
-        "Output:\n$stdout" .
-        "\nTry running lint with --trace flag to get more details.");
+  protected function parseLinterOutput($path, $err, $stdout, $stderr) {
+    $report_dom = new DOMDocument();
+    $ok = @$report_dom->loadXML($stdout);
+    if (!$ok) {
+      return false;
     }
 
     $files = $report_dom->getElementsByTagName('file');
+    $messages = array();
     foreach ($files as $file) {
       foreach ($file->childNodes as $child) {
         if (!($child instanceof DOMElement)) {
           continue;
         }
 
-        $data = $this->getData($path);
-        $lines = explode("\n", $data);
-        $line = $lines[$child->getAttribute('line') - 1];
-        $text = substr($line, $child->getAttribute('column') - 1);
-        $name = $this->getLinterName() . ' - ' . $child->getAttribute('source');
-        $severity = $child->tagName == 'error' ?
-            ArcanistLintSeverity::SEVERITY_ERROR
-            : ArcanistLintSeverity::SEVERITY_WARNING;
+        if ($child->tagName == 'error') {
+          $prefix = 'E';
+        } else {
+          $prefix = 'W';
+        }
+
+        $code = 'PHPCS.'.$prefix.'.'.$child->getAttribute('source');
 
         $message = new ArcanistLintMessage();
         $message->setPath($path);
         $message->setLine($child->getAttribute('line'));
         $message->setChar($child->getAttribute('column'));
-        $message->setCode($child->getAttribute('severity'));
-        $message->setName($name);
+        $message->setCode($code);
         $message->setDescription($child->nodeValue);
-        $message->setSeverity($severity);
-        $message->setOriginalText($text);
-        $this->addLintMessage($message);
+        $message->setSeverity($this->getLintMessageSeverity($code));
+
+        $messages[] = $message;
       }
     }
+
+    return $messages;
   }
+
+  protected function getDefaultMessageSeverity($code) {
+    if (preg_match('/^PHPCS\\.W\\./', $code)) {
+      return ArcanistLintSeverity::SEVERITY_WARNING;
+    } else {
+      return ArcanistLintSeverity::SEVERITY_ERROR;
+    }
+  }
+
+  protected function getLintCodeFromLinterConfigurationKey($code) {
+    if (!preg_match('/^PHPCS\\.(E|W)\\./', $code)) {
+      throw new Exception(
+        "Invalid severity code '{$code}', should begin with 'PHPCS.'.");
+    }
+    return $code;
+  }
+
 }

src/lint/linter/__tests__/ArcanistPHPCSLinterTestCase.php

@@ -0,0 +1,11 @@
+<?php
+
+final class ArcanistPHPCSLinterTestCase extends ArcanistArcanistLinterTestCase {
+
+  public function testPHPCSLint() {
+    return $this->executeTestsInDirectory(
+      dirname(__FILE__).'/phpcs/',
+      new ArcanistPhpcsLinter());
+  }
+
+}

src/lint/linter/__tests__/phpcs/basics.lint-test

@@ -0,0 +1,13 @@
+<?php
+
+function f() {
+  $$g;
+
+  $h++;
+}
+~~~~~~~~~~
+error:2:1
+error:3:1
+error:3:14
+error:4:3
+error:6:3