Modernize ArcanistPylintLinter

Summary:
Ref T2039. Convert the `ArcanistPylintLinter` to an `ArcanistExternalLinter` and make it compatible with `.arclint`. In doing so, it was necessary to drop support of older versions of `pylint` by setting the minimum version to be 1.0.0. I think that dropping support for older versions is reasonable because version 1.0.0 was released ~18 months ago. In the case than an incompatible version is detected, an `ArcanistMissingLinterException` is thrown.

One caveat here is that support for `lint.pylint.codes.(error|warning|advice)` is dropped. Any installs that are relying on this configuration will need to migrate to using the `.arclint` file for specifying linter severity. We could potentially continue to support this deprecated configuration, but I'm not sure if this is worthwhile.

Test Plan: Wrote and executed unit tests with `arc unit`. I ran the unit tests on all `pylint` releases after (and including) 1.0.0. Specifically, the tests were run on v1.0.0, v1.1.0, v1.2.0, v1.3.0 and v1.4.0.

Reviewers: epriestley, #blessed_reviewers

Reviewed By: epriestley, #blessed_reviewers

Subscribers: epriestley, Korvin

Maniphest Tasks: T2039

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

src/__phutil_library_map__.php

@@ -333,7 +333,7 @@ phutil_register_library_map(array(
     'ArcanistPuppetLintLinterTestCase' => 'ArcanistExternalLinterTestCase',
     'ArcanistPyFlakesLinter' => 'ArcanistExternalLinter',
     'ArcanistPyFlakesLinterTestCase' => 'ArcanistExternalLinterTestCase',
-    'ArcanistPyLintLinter' => 'ArcanistLinter',
+    'ArcanistPyLintLinter' => 'ArcanistExternalLinter',
     'ArcanistPyLintLinterTestCase' => 'ArcanistExternalLinterTestCase',
     'ArcanistRepositoryAPIMiscTestCase' => 'ArcanistTestCase',
     'ArcanistRepositoryAPIStateTestCase' => 'ArcanistTestCase',

src/lint/linter/ArcanistPyLintLinter.php

@@ -1,272 +1,199 @@
 <?php
 
 /**
- * Uses "PyLint" to detect various errors in Python code. To use this linter,
+ * Uses "PyLint" to detect various errors in Python code.
- * you must install pylint and configure which codes you want to be reported as
- * errors, warnings and advice.
- *
- * You should be able to install pylint with ##sudo easy_install pylint##. If
- * your system is unusual, you can manually specify the location of pylint and
- * its dependencies by configuring these keys in your .arcconfig:
- *
- *   lint.pylint.prefix
- *   lint.pylint.logilab_astng.prefix
- *   lint.pylint.logilab_common.prefix
- *
- * You can specify additional command-line options to pass to PyLint by
- * setting ##lint.pylint.options##. You may also specify a list of additional
- * entries for PYTHONPATH with ##lint.pylint.pythonpath##. Those can be
- * absolute or relative to the project root.
- *
- * If you have a PyLint rcfile, specify its path with
- * ##lint.pylint.rcfile##. It can be absolute or relative to the project
- * root. Be sure not to define ##output-format##, or if you do, set it to
- * ##text##.
- *
- * Specify which PyLint messages map to which Arcanist messages by defining
- * the following regular expressions:
- *
- *   lint.pylint.codes.error
- *   lint.pylint.codes.warning
- *   lint.pylint.codes.advice
- *
- * The regexps are run in that order; the first to match determines which
- * Arcanist severity applies, if any. For example, to capture all PyLint
- * "E...." errors as Arcanist errors, set ##lint.pylint.codes.error## to:
- *
- *    ^E.*
- *
- * You can also match more granularly:
- *
- *    ^E(0001|0002)$
- *
- * According to ##man pylint##, there are 5 kind of messages:
- *
- *   (C) convention, for programming standard violation
- *   (R) refactor, for bad code smell
- *   (W) warning, for python specific problems
- *   (E) error, for probable bugs in the code
- *   (F) fatal, if an error occurred which prevented pylint from
- *       doing further processing.
  */
-final class ArcanistPyLintLinter extends ArcanistLinter {
+final class ArcanistPyLintLinter extends ArcanistExternalLinter {
 
-  private function getMessageCodeSeverity($code) {
+  private $config;
-    $config = $this->getEngine()->getConfigurationManager();
 
-    $error_regexp   = $config->getConfigFromAnySource(
+  public function getInfoName() {
-      'lint.pylint.codes.error');
+    return 'PyLint';
-    $warning_regexp = $config->getConfigFromAnySource(
-      'lint.pylint.codes.warning');
-    $advice_regexp  = $config->getConfigFromAnySource(
-      'lint.pylint.codes.advice');
-
-    if (!$error_regexp && !$warning_regexp && !$advice_regexp) {
-      throw new ArcanistUsageException(
-        pht(
-          "You are invoking the PyLint linter but have not configured any of ".
-          "'%s', '%s', or '%s'. Consult the documentation for %s.",
-          'lint.pylint.codes.error',
-          'lint.pylint.codes.warning',
-          'lint.pylint.codes.advice',
-          __CLASS__));
-    }
-
-    $code_map = array(
-      ArcanistLintSeverity::SEVERITY_ERROR    => $error_regexp,
-      ArcanistLintSeverity::SEVERITY_WARNING  => $warning_regexp,
-      ArcanistLintSeverity::SEVERITY_ADVICE   => $advice_regexp,
-    );
-
-    foreach ($code_map as $sev => $codes) {
-      if ($codes === null) {
-        continue;
-      }
-      if (!is_array($codes)) {
-        $codes = array($codes);
-      }
-      foreach ($codes as $code_re) {
-        if (preg_match("/{$code_re}/", $code)) {
-          return $sev;
-        }
-      }
-    }
-
-    // If the message code doesn't match any of the provided regex's,
-    // then just disable it.
-    return ArcanistLintSeverity::SEVERITY_DISABLED;
   }
 
-  private function getPyLintPath() {
+  public function getInfoURI() {
-    $pylint_bin = 'pylint';
+    return 'http://www.pylint.org/';
-
-    // Use the PyLint prefix specified in the config file
-    $config = $this->getEngine()->getConfigurationManager();
-    $prefix = $config->getConfigFromAnySource('lint.pylint.prefix');
-    if ($prefix !== null) {
-      $pylint_bin = $prefix.'/bin/'.$pylint_bin;
-    }
-
-    if (!Filesystem::pathExists($pylint_bin)) {
-
-      list($err) = exec_manual('which %s', $pylint_bin);
-      if ($err) {
-        throw new ArcanistMissingLinterException(
-          pht(
-            "PyLint does not appear to be installed on this system. Install ".
-            "it (e.g., with '%s') or configure '%s' in your %s to point to ".
-            "the directory where it resides.",
-            'sudo easy_install pylint',
-            'lint.pylint.prefix',
-            '.arcconfig'));
-      }
-    }
-
-    return $pylint_bin;
   }
 
-  private function getPyLintPythonPath() {
+  public function getInfoDescription() {
-    // Get non-default install locations for pylint and its dependencies
+    return pht(
-    // libraries.
+      'PyLint is a Python source code analyzer which looks for '.
-    $config = $this->getEngine()->getConfigurationManager();
+      'programming errors, helps enforcing a coding standard and '.
-    $prefixes = array(
+      'sniffs for some code smells.');
-      $config->getConfigFromAnySource('lint.pylint.prefix'),
-      $config->getConfigFromAnySource('lint.pylint.logilab_astng.prefix'),
-      $config->getConfigFromAnySource('lint.pylint.logilab_common.prefix'),
-    );
-
-    // Add the libraries to the python search path
-    $python_path = array();
-    foreach ($prefixes as $prefix) {
-      if ($prefix !== null) {
-        $python_path[] = $prefix.'/lib/python2.7/site-packages';
-        $python_path[] = $prefix.'/lib/python2.7/dist-packages';
-        $python_path[] = $prefix.'/lib/python2.6/site-packages';
-        $python_path[] = $prefix.'/lib/python2.6/dist-packages';
-      }
-    }
-
-    $working_copy = $this->getEngine()->getWorkingCopy();
-    $config_paths = $config->getConfigFromAnySource('lint.pylint.pythonpath');
-    if ($config_paths !== null) {
-      foreach ($config_paths as $config_path) {
-        if ($config_path !== null) {
-          $python_path[] = Filesystem::resolvePath(
-            $config_path,
-            $working_copy->getProjectRoot());
-        }
-      }
-    }
-
-    $python_path[] = '';
-    return implode(':', $python_path);
-  }
-
-  private function getPyLintOptions() {
-    // '-rn': don't print lint report/summary at end
-    $options = array('-rn');
-
-    // Version 0.x.x include the pylint message ids in the output
-    if (version_compare($this->getLinterVersion(), '1', 'lt')) {
-      array_push($options, '-iy', '--output-format=text');
-    }
-    // Version 1.x.x set the output specifically to the 0.x.x format
-    else {
-      array_push($options, "--msg-template='{msg_id}:{line:3d}: {obj}: {msg}'");
-    }
-
-    $working_copy = $this->getEngine()->getWorkingCopy();
-    $config = $this->getEngine()->getConfigurationManager();
-
-    // Specify an --rcfile, either absolute or relative to the project root.
-    // Stupidly, the command line args above are overridden by rcfile, so be
-    // careful.
-    $rcfile = $config->getConfigFromAnySource('lint.pylint.rcfile');
-    if ($rcfile !== null) {
-      $rcfile = Filesystem::resolvePath(
-        $rcfile,
-        $working_copy->getProjectRoot());
-      $options[] = csprintf('--rcfile=%s', $rcfile);
-    }
-
-    // Add any options defined in the config file for PyLint
-    $config_options = $config->getConfigFromAnySource('lint.pylint.options');
-    if ($config_options !== null) {
-      $options = array_merge($options, $config_options);
-    }
-
-    return implode(' ', $options);
   }
 
   public function getLinterName() {
     return 'PyLint';
   }
 
-  private function getLinterVersion() {
+  public function getLinterConfigurationName() {
-    $pylint_bin = $this->getPyLintPath();
+    return 'pylint';
-    $options = '--version';
-
-    list($stdout) = execx('%s %s', $pylint_bin, $options);
-
-    $lines = phutil_split_lines($stdout, false);
-    $matches = null;
-
-    // If the version command didn't return anything or the regex didn't match
-    // Assume a future version that at least is compatible with 1.x.x
-    if (count($lines) == 0 ||
-        !preg_match('/pylint\s((?:\d+\.?)+)/', $lines[0], $matches)) {
-      return '999';
-    }
-
-    return $matches[1];
   }
 
-  public function lintPath($path) {
+  public function getDefaultBinary() {
-    $pylint_bin = $this->getPyLintPath();
+    $prefix = $this->getDeprecatedConfiguration('lint.pylint.prefix');
-    $python_path = $this->getPyLintPythonPath();
+    $bin = $this->getDeprecatedConfiguration('lint.pylint.bin', 'pylint');
-    $options = $this->getPyLintOptions();
-    $path_on_disk = $this->getEngine()->getFilePathOnDisk($path);
 
-    try {
+    if ($prefix) {
-      list($stdout, $_) = execx(
+      return $prefix.'/bin/'.$bin;
-        '/usr/bin/env PYTHONPATH=%s$PYTHONPATH %s %C %s',
+    } else {
-        $python_path,
+      return $bin;
-        $pylint_bin,
+    }
-        $options,
+  }
-        $path_on_disk);
+
-    } catch (CommandException $e) {
+  public function getVersion() {
-      if ($e->getError() == 32) {
+    list($stdout) = execx('%C --version', $this->getExecutableCommand());
-        // According to ##man pylint## the exit status of 32 means there was a
+
-        // usage error. That's bad, so actually exit abnormally.
+    $matches = array();
-        throw $e;
+    $regex = '/^pylint (?P<version>\d+\.\d+\.\d+),/';
-      } else {
+    if (preg_match($regex, $stdout, $matches)) {
-        // The other non-zero exit codes mean there were messages issued,
+      return $matches['version'];
-        // which is expected, so don't exit.
+    } else {
-        $stdout = $e->getStdout();
+      return false;
-      }
+    }
+  }
+
+  public function getInstallInstructions() {
+    return pht(
+      'Install PyLint using `%s`.',
+      'pip install pylint');
+  }
+
+  public function shouldExpectCommandErrors() {
+    return true;
+  }
+
+  public function getLinterConfigurationOptions() {
+    $options = array(
+      'pylint.config' => array(
+        'type' => 'optional string',
+        'help' => pht('Pass in a custom configuration file path.'),
+      ),
+    );
+
+    return $options + parent::getLinterConfigurationOptions();
+  }
+
+  public function setLinterConfigurationValue($key, $value) {
+    switch ($key) {
+      case 'pylint.config':
+        $this->config = $value;
+        return;
+
+      default:
+        return parent::setLinterConfigurationValue($key, $value);
+    }
+  }
+
+  protected function getMandatoryFlags() {
+    $options = array();
+
+    $options[] = '--reports=no';
+    $options[] = '--msg-template="{line}|{column}|{msg_id}|{symbol}|{msg}"';
+
+    // Specify an `--rcfile`, either absolute or relative to the project root.
+    // Stupidly, the command line args above are overridden by rcfile, so be
+    // careful.
+    $config = $this->config;
+    if (!$config) {
+      $config = $this->getDeprecatedConfiguration('lint.pylint.rcfile');
+    }
+
+    if ($config !== null) {
+      $options[] = '--rcfile='.$config;
+    }
+
+    return $options;
+  }
+
+  protected function getDefaultFlags() {
+    $options = array();
+
+    // Add any options defined in the config file for PyLint.
+    $config_options = $this->getDeprecatedConfiguration(
+      'lint.pylint.options',
+      array());
+    $options = array_merge($options, $config_options);
+
+    $installed_version = $this->getVersion();
+    $minimum_version = '1.0.0';
+    if (version_compare($installed_version, $minimum_version, '<')) {
+      throw new ArcanistMissingLinterException(
+        pht(
+          '%s is not compatible with the installed version of pylint. '.
+          'Minimum version: %s; installed version: %s.',
+          __CLASS__,
+          $minimum_version,
+          $installed_version));
+    }
+
+    return $options;
+  }
+
+  protected function parseLinterOutput($path, $err, $stdout, $stderr) {
+    if ($err === 32) {
+      // According to `man pylint` the exit status of 32 means there was a
+      // usage error. That's bad, so actually exit abnormally.
+      return false;
     }
 
     $lines = phutil_split_lines($stdout, false);
     $messages = array();
+
     foreach ($lines as $line) {
-      $matches = null;
+      $matches = explode('|', $line, 5);
-      $regex = '/([A-Z]\d+): *(\d+)(?:|,\d*): *(.*)$/';
+
-      if (!preg_match($regex, $line, $matches)) {
+      if (count($matches) < 5) {
         continue;
       }
-      foreach ($matches as $key => $match) {
-        $matches[$key] = trim($match);
-      }
 
-      $message = new ArcanistLintMessage();
+      $message = id(new ArcanistLintMessage())
-      $message->setPath($path);
+        ->setPath($path)
-      $message->setLine($matches[2]);
+        ->setLine($matches[0])
-      $message->setCode($matches[1]);
+        ->setChar($matches[1])
-      $message->setName($this->getLinterName().' '.$matches[1]);
+        ->setCode($matches[2])
-      $message->setDescription($matches[3]);
+        ->setSeverity($this->getLintMessageSeverity($matches[2]))
-      $message->setSeverity($this->getMessageCodeSeverity($matches[1]));
+        ->setName(ucwords(str_replace('-', ' ', $matches[3])))
-      $this->addLintMessage($message);
+        ->setDescription($matches[4]);
+
+      $messages[] = $message;
+    }
+
+    if ($err && !$messages) {
+      return false;
+    }
+
+    return $messages;
+  }
+
+  protected function getDefaultMessageSeverity($code) {
+    switch (substr($code, 0, 1)) {
+      case 'R':
+      case 'C':
+        return ArcanistLintSeverity::SEVERITY_ADVICE;
+      case 'W':
+        return ArcanistLintSeverity::SEVERITY_WARNING;
+      case 'E':
+      case 'F':
+        return ArcanistLintSeverity::SEVERITY_ERROR;
+      default:
+        return ArcanistLintSeverity::SEVERITY_DISABLED;
     }
   }
 
+  protected function getLintCodeFromLinterConfigurationKey($code) {
+    if (!preg_match('/^(R|C|W|E|F)\d{4}$/', $code)) {
+      throw new Exception(
+        pht(
+          'Unrecognized lint message code "%s". Expected a valid Pylint '.
+          'lint code like "%s", or "%s", or "%s".',
+          $code,
+          'C0111',
+          'E0602',
+          'W0611'));
+    }
+
+    return $code;
+  }
+
 }