diff --git a/pyls/hookspecs.py b/pyls/hookspecs.py index d094dcb5..880be1ff 100644 --- a/pyls/hookspecs.py +++ b/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 diff --git a/pyls/plugins/pylint_lint.py b/pyls/plugins/pylint_lint.py new file mode 100644 index 00000000..f836cd1a --- /dev/null +++ b/pyls/plugins/pylint_lint.py @@ -0,0 +1,140 @@ +# Copyright 2018 Google LLC. +"""Linter plugin for pylint.""" +import collections +import json +import logging +import sys + +from pylint.epylint import py_run +from pyls import hookimpl, lsp + + +log = logging.getLogger(__name__) + + +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.*, + } + """ + log.debug('Running pylint on %s', document.path) + 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. + log.debug('Cannot run pylint, file has not been saved to disk.') + 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, + }) + cls.last_diags[document.path] = diagnostics + return diagnostics + + +@hookimpl +def pyls_lint(document, is_saved): + return PylintLinter.lint(document, is_saved) diff --git a/pyls/python_ls.py b/pyls/python_ls.py index da9d8f48..fffd53ef 100644 --- a/pyls/python_ls.py +++ b/pyls/python_ls.py @@ -211,10 +211,12 @@ 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( @@ -234,7 +236,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=True) def m_text_document__did_change(self, contentChanges=None, textDocument=None, **_kwargs): for change in contentChanges: @@ -243,10 +245,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) @@ -290,12 +292,12 @@ 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, **_kwargs): # Externally changed files may result in changed diagnostics for doc_uri in self.workspace.documents: - 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) diff --git a/setup.py b/setup.py index 2d09e9dc..485ad090 100755 --- a/setup.py +++ b/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': ['tox', 'versioneer', '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', diff --git a/test/plugins/test_pylint_lint.py b/test/plugins/test_pylint_lint.py new file mode 100644 index 00000000..90d40025 --- /dev/null +++ b/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)