Add pylint support. (#385)

This also adds an `is_saved` argument to the pyls_lint hookspec, since
pylint doesn't expose an API for operating on in-memory contents, only
files.

Fixes #129

Author: DanAlbert

pyls/hookspecs.py

@@ -88,7 +88,7 @@ def pyls_initialize(config, workspace):
 
 
 @hookspec
-def pyls_lint(config, workspace, document):
+def pyls_lint(config, workspace, document, is_saved):
     pass
 
 

pyls/plugins/pylint_lint.py

@@ -0,0 +1,136 @@
+# Copyright 2018 Google LLC.
+"""Linter plugin for pylint."""
+import collections
+import json
+import sys
+
+from pylint.epylint import py_run
+from pyls import hookimpl, lsp
+
+
+class PylintLinter(object):
+    last_diags = collections.defaultdict(list)
+
+    @classmethod
+    def lint(cls, document, is_saved, flags=''):
+        """Plugin interface to pyls linter.
+
+        Args:
+            document: The document to be linted.
+            is_saved: Whether or not the file has been saved to disk.
+            flags: Additional flags to pass to pylint. Not exposed to
+                pyls_lint, but used for testing.
+
+        Returns:
+            A list of dicts with the following format:
+
+                {
+                    'source': 'pylint',
+                    'range': {
+                        'start': {
+                            'line': start_line,
+                            'character': start_column,
+                        },
+                        'end': {
+                            'line': end_line,
+                            'character': end_column,
+                        },
+                    }
+                    'message': msg,
+                    'severity': lsp.DiagnosticSeverity.*,
+                }
+        """
+        if not is_saved:
+            # Pylint can only be run on files that have been saved to disk.
+            # Rather than return nothing, return the previous list of
+            # diagnostics. If we return an empty list, any diagnostics we'd
+            # previously shown will be cleared until the next save. Instead,
+            # continue showing (possibly stale) diagnostics until the next
+            # save.
+            return cls.last_diags[document.path]
+
+        # py_run will call shlex.split on its arguments, and shlex.split does
+        # not handle Windows paths (it will try to perform escaping). Turn
+        # backslashes into forward slashes first to avoid this issue.
+        path = document.path
+        if sys.platform.startswith('win'):
+            path = path.replace('\\', '/')
+        out, _err = py_run(
+            '{} -f json {}'.format(path, flags), return_std=True
+        )
+
+        # pylint prints nothing rather than [] when there are no diagnostics.
+        # json.loads will not parse an empty string, so just return.
+        json_str = out.getvalue()
+        if not json_str.strip():
+            cls.last_diags[document.path] = []
+            return []
+
+        # Pylint's JSON output is a list of objects with the following format.
+        #
+        #     {
+        #         "obj": "main",
+        #         "path": "foo.py",
+        #         "message": "Missing function docstring",
+        #         "message-id": "C0111",
+        #         "symbol": "missing-docstring",
+        #         "column": 0,
+        #         "type": "convention",
+        #         "line": 5,
+        #         "module": "foo"
+        #     }
+        #
+        # The type can be any of:
+        #
+        #  * convention
+        #  * error
+        #  * fatal
+        #  * refactor
+        #  * warning
+        diagnostics = []
+        for diag in json.loads(json_str):
+            # pylint lines index from 1, pyls lines index from 0
+            line = diag['line'] - 1
+            # But both index columns from 0
+            col = diag['column']
+
+            # It's possible that we're linting an empty file. Even an empty
+            # file might fail linting if it isn't named properly.
+            end_col = len(document.lines[line]) if document.lines else 0
+
+            err_range = {
+                'start': {
+                    'line': line,
+                    'character': col,
+                },
+                'end': {
+                    'line': line,
+                    'character': end_col,
+                },
+            }
+
+            if diag['type'] == 'convention':
+                severity = lsp.DiagnosticSeverity.Information
+            elif diag['type'] == 'error':
+                severity = lsp.DiagnosticSeverity.Error
+            elif diag['type'] == 'fatal':
+                severity = lsp.DiagnosticSeverity.Error
+            elif diag['type'] == 'refactor':
+                severity = lsp.DiagnosticSeverity.Hint
+            elif diag['type'] == 'warning':
+                severity = lsp.DiagnosticSeverity.Warning
+
+            diagnostics.append({
+                'source': 'pylint',
+                'range': err_range,
+                'message': '[{}] {}'.format(diag['symbol'], diag['message']),
+                'severity': severity,
+                'code': diag['message-id']
+            })
+        cls.last_diags[document.path] = diagnostics
+        return diagnostics
+
+
+@hookimpl
+def pyls_lint(document, is_saved):
+    return PylintLinter.lint(document, is_saved)

pyls/python_ls.py

@@ -215,10 +215,13 @@ def hover(self, doc_uri, position):
         return self._hook('pyls_hover', doc_uri, position=position) or {'contents': ''}
 
     @_utils.debounce(LINT_DEBOUNCE_S, keyed_by='doc_uri')
-    def lint(self, doc_uri):
+    def lint(self, doc_uri, is_saved):
         # Since we're debounced, the document may no longer be open
         if doc_uri in self.workspace.documents:
-            self.workspace.publish_diagnostics(doc_uri, flatten(self._hook('pyls_lint', doc_uri)))
+            self.workspace.publish_diagnostics(
+                doc_uri,
+                flatten(self._hook('pyls_lint', doc_uri, is_saved=is_saved))
+            )
 
     def references(self, doc_uri, position, exclude_declaration):
         return flatten(self._hook(
@@ -238,7 +241,7 @@ def m_text_document__did_close(self, textDocument=None, **_kwargs):
     def m_text_document__did_open(self, textDocument=None, **_kwargs):
         self.workspace.put_document(textDocument['uri'], textDocument['text'], version=textDocument.get('version'))
         self._hook('pyls_document_did_open', textDocument['uri'])
-        self.lint(textDocument['uri'])
+        self.lint(textDocument['uri'], is_saved=False)
 
     def m_text_document__did_change(self, contentChanges=None, textDocument=None, **_kwargs):
         for change in contentChanges:
@@ -247,10 +250,10 @@ def m_text_document__did_change(self, contentChanges=None, textDocument=None, **
                 change,
                 version=textDocument.get('version')
             )
-        self.lint(textDocument['uri'])
+        self.lint(textDocument['uri'], is_saved=False)
 
     def m_text_document__did_save(self, textDocument=None, **_kwargs):
-        self.lint(textDocument['uri'])
+        self.lint(textDocument['uri'], is_saved=True)
 
     def m_text_document__code_action(self, textDocument=None, range=None, context=None, **_kwargs):
         return self.code_actions(textDocument['uri'], range, context)
@@ -294,7 +297,7 @@ def m_text_document__signature_help(self, textDocument=None, position=None, **_k
     def m_workspace__did_change_configuration(self, settings=None):
         self.config.update((settings or {}).get('pyls', {}))
         for doc_uri in self.workspace.documents:
-            self.lint(doc_uri)
+            self.lint(doc_uri, is_saved=False)
 
     def m_workspace__did_change_watched_files(self, changes=None, **_kwargs):
         changed_py_files = set(d['uri'] for d in changes if d['uri'].endswith(PYTHON_FILE_EXTENSIONS))
@@ -306,7 +309,7 @@ def m_workspace__did_change_watched_files(self, changes=None, **_kwargs):
         for doc_uri in self.workspace.documents:
             # Changes in doc_uri are already handled by m_text_document__did_save
             if doc_uri not in changed_py_files:
-                self.lint(doc_uri)
+                self.lint(doc_uri, is_saved=False)
 
     def m_workspace__execute_command(self, command=None, arguments=None):
         return self.execute_command(command, arguments)

setup.py

@@ -51,6 +51,7 @@
             'pycodestyle',
             'pydocstyle>=2.0.0',
             'pyflakes>=1.6.0',
+            'pylint',
             'rope>=0.10.5',
             'yapf',
         ],
@@ -59,6 +60,7 @@
         'pycodestyle': ['pycodestyle'],
         'pydocstyle': ['pydocstyle>=2.0.0'],
         'pyflakes': ['pyflakes>=1.6.0'],
+        'pylint': ['pylint'],
         'rope': ['rope>0.10.5'],
         'yapf': ['yapf'],
         'test': ['versioneer', 'pylint', 'pytest', 'mock', 'pytest-cov', 'coverage'],
@@ -85,6 +87,7 @@
             'pycodestyle = pyls.plugins.pycodestyle_lint',
             'pydocstyle = pyls.plugins.pydocstyle_lint',
             'pyflakes = pyls.plugins.pyflakes_lint',
+            'pylint = pyls.plugins.pylint_lint',
             'rope_completion = pyls.plugins.rope_completion',
             'rope_rename = pyls.plugins.rope_rename',
             'yapf = pyls.plugins.yapf_format',

test/plugins/test_pylint_lint.py

@@ -0,0 +1,104 @@
+# Copyright 2018 Google LLC.
+import contextlib
+import os
+import tempfile
+
+from pyls import lsp, uris
+from pyls.workspace import Document
+from pyls.plugins import pylint_lint
+
+DOC_URI = uris.from_fs_path(__file__)
+DOC = """import sys
+
+def hello():
+\tpass
+
+import json
+"""
+
+DOC_SYNTAX_ERR = """def hello()
+    pass
+"""
+
+
+@contextlib.contextmanager
+def temp_document(doc_text):
+    try:
+        temp_file = tempfile.NamedTemporaryFile(mode='w', delete=False)
+        name = temp_file.name
+        temp_file.write(doc_text)
+        temp_file.close()
+        yield Document(uris.from_fs_path(name))
+    finally:
+        os.remove(name)
+
+
+def write_temp_doc(document, contents):
+    with open(document.path, 'w') as temp_file:
+        temp_file.write(contents)
+
+
+def test_pylint():
+    with temp_document(DOC) as doc:
+        diags = pylint_lint.pyls_lint(doc, True)
+
+        msg = '[unused-import] Unused import sys'
+        unused_import = [d for d in diags if d['message'] == msg][0]
+
+        assert unused_import['range']['start'] == {'line': 0, 'character': 0}
+        assert unused_import['severity'] == lsp.DiagnosticSeverity.Warning
+
+
+def test_syntax_error_pylint():
+    with temp_document(DOC_SYNTAX_ERR) as doc:
+        diag = pylint_lint.pyls_lint(doc, True)[0]
+
+        assert diag['message'].startswith('[syntax-error] invalid syntax')
+        # Pylint doesn't give column numbers for invalid syntax.
+        assert diag['range']['start'] == {'line': 0, 'character': 0}
+        assert diag['severity'] == lsp.DiagnosticSeverity.Error
+
+
+def test_lint_free_pylint():
+    # Can't use temp_document because it might give us a file that doesn't
+    # match pylint's naming requirements. We should be keeping this file clean
+    # though, so it works for a test of an empty lint.
+    assert not pylint_lint.pyls_lint(
+        Document(uris.from_fs_path(__file__)), True)
+
+
+def test_lint_caching():
+    # Pylint can only operate on files, not in-memory contents. We cache the
+    # diagnostics after a run so we can continue displaying them until the file
+    # is saved again.
+    #
+    # We use PylintLinter.lint directly here rather than pyls_lint so we can
+    # pass --disable=invalid-name to pylint, since we want a temporary file but
+    # need to ensure that pylint doesn't give us invalid-name when our temp
+    # file has capital letters in its name.
+
+    flags = '--disable=invalid-name'
+    with temp_document(DOC) as doc:
+        # Start with a file with errors.
+        diags = pylint_lint.PylintLinter.lint(doc, True, flags)
+        assert diags
+
+        # Fix lint errors and write the changes to disk. Run the linter in the
+        # in-memory mode to check the cached diagnostic behavior.
+        write_temp_doc(doc, '')
+        assert pylint_lint.PylintLinter.lint(doc, False, flags) == diags
+
+        # Now check the on-disk behavior.
+        assert not pylint_lint.PylintLinter.lint(doc, True, flags)
+
+        # Make sure the cache was properly cleared.
+        assert not pylint_lint.PylintLinter.lint(doc, False, flags)
+
+
+def test_per_file_caching():
+    # Ensure that diagnostics are cached per-file.
+    with temp_document(DOC) as doc:
+        assert pylint_lint.pyls_lint(doc, True)
+
+    assert not pylint_lint.pyls_lint(
+        Document(uris.from_fs_path(__file__)), False)