Expose PEP8, Flake8 and CSSLint engines to .arclint

Summary:
Ref T3186. Ref T2039. Allow these linters to be selected with `.arclint`.

Also allow severities to be set.

Also fix some other minor bugs.

Test Plan:
https://github.com/epriestley/arclint-examples
https://github.com/epriestley/arclint-examples/blob/master/.arclint

Reviewers: btrahan

Reviewed By: btrahan

CC: chad, aran

Maniphest Tasks: T2039, T3186

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

src/lint/engine/ArcanistConfigurationDrivenLintEngine.php

@@ -65,13 +65,14 @@ final class ArcanistConfigurationDrivenLintEngine extends ArcanistLintEngine {
       foreach ($more as $key => $value) {
         if (array_key_exists($key, $spec)) {
           try {
-            $linter->setLinterConfigurationValue($key, $spec);
+            $linter->setLinterConfigurationValue($key, $spec[$key]);
           } catch (Exception $ex) {
             $message = pht(
               'Error in parsing ".arclint" file, in key "%s" for '.
-              'linter "%s".',
+              'linter "%s": %s',
               $key,
-              $name);
+              $name,
+              $ex->getMessage());
             throw new PhutilProxyException($message, $ex);
           }
         }

src/lint/linter/ArcanistCSSLintLinter.php

@@ -19,6 +19,10 @@ final class ArcanistCSSLintLinter extends ArcanistExternalLinter {
     return 'CSSLint';
   }
 
+  public function getLinterConfigurationName() {
+    return 'csslint';
+  }
+
   public function getMandatoryFlags() {
     return '--format=lint-xml';
   }
@@ -65,7 +69,7 @@ final class ArcanistCSSLintLinter extends ArcanistExternalLinter {
 
         $data = $this->getData($path);
         $lines = explode("\n", $data);
-        $name = $this->getLinterName() . ' - ' . $child->getAttribute('reason');
+        $name = $child->getAttribute('reason');
         $severity = ($child->getAttribute('severity') == 'warning')
           ? ArcanistLintSeverity::SEVERITY_WARNING
           : ArcanistLintSeverity::SEVERITY_ERROR;
@@ -74,11 +78,8 @@ final class ArcanistCSSLintLinter extends ArcanistExternalLinter {
         $message->setPath($path);
         $message->setLine($child->getAttribute('line'));
         $message->setChar($child->getAttribute('char'));
-        $message->setCode($child->getAttribute('severity'));
+        $message->setCode('CSSLint');
-        $message->setName($name);
+        $message->setDescription($child->getAttribute('reason'));
-        $message->setDescription(
-          $child->getAttribute('reason').
-          "\nEvidence:".$child->getAttribute('evidence'));
         $message->setSeverity($severity);
 
         if ($child->hasAttribute('line')) {
@@ -93,4 +94,20 @@ final class ArcanistCSSLintLinter extends ArcanistExternalLinter {
 
     return $messages;
   }
+
+  protected function getLintCodeFromLinterConfigurationKey($code) {
+
+    // NOTE: We can't figure out which rule generated each message, so we
+    // can not customize severities. I opened a pull request to add this
+    // ability; see:
+    //
+    // https://github.com/stubbornella/csslint/pull/409
+
+    throw new Exception(
+      pht(
+        "CSSLint does not currently support custom severity levels, because ".
+        "rules can't be identified from messages in output. ".
+        "See Pull Request #409."));
+  }
+
 }

src/lint/linter/ArcanistExternalLinter.php

@@ -393,8 +393,12 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter {
     $messages = $this->parseLinterOutput($path, $err, $stdout, $stderr);
 
     if ($messages === false) {
-      $future->resolvex();
+      if ($err) {
-      return;
+        $future->resolvex();
+      } else {
+        throw new Exception(
+          "Linter failed to parse output!\n\n{$stdout}\n\n{$stderr}");
+      }
     }
 
     foreach ($messages as $message) {
@@ -407,6 +411,7 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter {
     $options = array(
       'bin' => 'optional string | list<string>',
       'flags' => 'optional string',
+      'severity' => 'optional map<string, string>',
     );
 
     if ($this->shouldUseInterpreter()) {
@@ -464,10 +469,50 @@ abstract class ArcanistExternalLinter extends ArcanistFutureLinter {
         if (strlen($value)) {
           $this->setFlags($value);
         }
-        break;
+        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);
   }
 
+
+  /**
+   * 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/ArcanistFlake8Linter.php

@@ -12,6 +12,10 @@ final class ArcanistFlake8Linter extends ArcanistExternalLinter {
     return 'flake8';
   }
 
+  public function getLinterConfigurationName() {
+    return 'flake8';
+  }
+
   public function getDefaultFlags() {
     // TODO: Deprecated.
 
@@ -66,12 +70,6 @@ final class ArcanistFlake8Linter extends ArcanistExternalLinter {
         $matches[$key] = trim($match);
       }
 
-      if (substr($matches[4], 0, 1) == 'E') {
-        $severity = ArcanistLintSeverity::SEVERITY_ERROR;
-      } else {
-        $severity = ArcanistLintSeverity::SEVERITY_WARNING;
-      }
-
       $message = new ArcanistLintMessage();
       $message->setPath($path);
       $message->setLine($matches[2]);
@@ -81,7 +79,7 @@ final class ArcanistFlake8Linter extends ArcanistExternalLinter {
       $message->setCode($matches[4]);
       $message->setName($this->getLinterName().' '.$matches[3]);
       $message->setDescription($matches[5]);
-      $message->setSeverity($severity);
+      $message->setSeverity($this->getLintMessageSeverity($matches[4]));
 
       $messages[] = $message;
     }
@@ -93,4 +91,34 @@ final class ArcanistFlake8Linter extends ArcanistExternalLinter {
     return $messages;
   }
 
+  protected function getDefaultMessageSeverity($code) {
+    if (preg_match('/^C/', $code)) {
+      // "C": Cyclomatic complexity
+      return ArcanistLintSeverity::SEVERITY_ADVICE;
+    } else if (preg_match('/^W/', $code)) {
+      // "W": PEP8 Warning
+      return ArcanistLintSeverity::SEVERITY_WARNING;
+    } else {
+      // "E": PEP8 Error
+      // "F": PyFlakes Error
+      return ArcanistLintSeverity::SEVERITY_ERROR;
+    }
+  }
+
+  protected function getLintCodeFromLinterConfigurationKey($code) {
+    if (!preg_match('/^(E|W|C|F)\d+$/', $code)) {
+      throw new Exception(
+        pht(
+          'Unrecognized lint message code "%s". Expected a valid flake8 '.
+          'lint code like "%s", or "%s", or "%s", or "%s".',
+          $code,
+          "E225",
+          "W291",
+          "F811",
+          "C901"));
+    }
+
+    return $code;
+  }
+
 }

src/lint/linter/ArcanistLinter.php

@@ -121,6 +121,10 @@ abstract class ArcanistLinter {
       return $map[$code];
     }
 
+    return $this->getDefaultMessageSeverity($code);
+  }
+
+  protected function getDefaultMessageSeverity($code) {
     return ArcanistLintSeverity::SEVERITY_ERROR;
   }
 

src/lint/linter/ArcanistPEP8Linter.php

@@ -11,6 +11,10 @@ final class ArcanistPEP8Linter extends ArcanistExternalLinter {
     return 'PEP8';
   }
 
+  public function getLinterConfigurationName() {
+    return 'pep8';
+  }
+
   public function getCacheVersion() {
     list($stdout) = execx('%C --version', $this->getExecutableCommand());
     return $stdout.$this->getCommandFlags();
@@ -76,9 +80,6 @@ final class ArcanistPEP8Linter extends ArcanistExternalLinter {
       foreach ($matches as $key => $match) {
         $matches[$key] = trim($match);
       }
-      if (!$this->isMessageEnabled($matches[4])) {
-        continue;
-      }
       $message = new ArcanistLintMessage();
       $message->setPath($path);
       $message->setLine($matches[2]);
@@ -86,7 +87,7 @@ final class ArcanistPEP8Linter extends ArcanistExternalLinter {
       $message->setCode($matches[4]);
       $message->setName('PEP8 '.$matches[4]);
       $message->setDescription($matches[5]);
-      $message->setSeverity(ArcanistLintSeverity::SEVERITY_WARNING);
+      $message->setSeverity($this->getLintMessageSeverity($matches[4]));
 
       $messages[] = $message;
     }
@@ -98,4 +99,31 @@ final class ArcanistPEP8Linter extends ArcanistExternalLinter {
     return $messages;
   }
 
+  protected function getDefaultMessageSeverity($code) {
+    if (preg_match('/^W/', $code)) {
+      return ArcanistLintSeverity::SEVERITY_WARNING;
+    } else {
+
+      // TODO: Once severities/.arclint are more usable, restore this to
+      // "ERROR".
+      // return ArcanistLintSeverity::SEVERITY_ERROR;
+
+      return ArcanistLintSeverity::SEVERITY_WARNING;
+    }
+  }
+
+  protected function getLintCodeFromLinterConfigurationKey($code) {
+    if (!preg_match('/^(E|W)\d+$/', $code)) {
+      throw new Exception(
+        pht(
+          'Unrecognized lint message code "%s". Expected a valid PEP8 '.
+          'lint code like "%s" or "%s".',
+          $code,
+          "E101",
+          "W291"));
+    }
+
+    return $code;
+  }
+
 }

src/lint/linter/__tests__/flake8/undefined.lint-test

@@ -4,4 +4,4 @@ def hello():
     return foo
 ~~~~~~~~~~
 error:3:1
-warning:4:1
+error:4:1