Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code action to add/delete imports #685

Open
wants to merge 33 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
33 commits
Select commit Hold shift + click to select a range
e7e7dd2
Auto import actions using importmagic
youben11 Oct 28, 2019
f9ef0a0
import re
youben11 Oct 28, 2019
4c89036
typo
youben11 Oct 28, 2019
4383f1e
test against a specific action
youben11 Oct 28, 2019
7ed8a3b
disable actions test
youben11 Oct 29, 2019
4ae20c8
missing white-space
youben11 Oct 29, 2019
8242066
separate action generation logic: fix linting problem
youben11 Oct 29, 2019
5175028
fix: pass document instead of source
youben11 Oct 29, 2019
f54910d
filter results based on minScore parameter
youben11 Oct 29, 2019
2668de3
update test
youben11 Oct 29, 2019
ecabe85
Merge branch 'develop' into importmagic
gatesn Nov 4, 2019
936f121
private function
youben11 Nov 4, 2019
b0f04a3
change log level
youben11 Nov 4, 2019
2261042
Merge branch 'importmagic' of github.com:youben11/python-language-ser…
youben11 Nov 4, 2019
d57d904
warn about unreferenced import/variable/function
youben11 Nov 6, 2019
c7e5b36
remove unreferenced imports
youben11 Nov 9, 2019
061f0cc
config option to enable importmagic
youben11 Nov 12, 2019
e9996f7
fix camel_to_underscore
youben11 Nov 12, 2019
48d1f01
clarify remove command action title
youben11 Nov 12, 2019
2d12e54
clean message checking
youben11 Nov 12, 2019
71b8385
get diagnostics from pyls_lint
youben11 Nov 12, 2019
f99d33e
non blocking index build
youben11 Nov 12, 2019
6812993
store cache as dict
youben11 Nov 13, 2019
d178670
test remove action
youben11 Nov 14, 2019
ae63149
better linting test
youben11 Nov 14, 2019
a712a73
search symbols in source tokens instead of raw string:
youben11 Nov 15, 2019
80def3f
check for None value
youben11 Nov 15, 2019
0a9d163
fix range
youben11 Nov 15, 2019
e7ee0ca
use backward compatible tokenizer func
youben11 Nov 15, 2019
809a4af
fix indexing and pattern matching
youben11 Nov 15, 2019
1c509f9
typo
youben11 Nov 15, 2019
b82ff8f
new style class
youben11 Nov 15, 2019
107c431
use list comprehension for better perf
youben11 Nov 15, 2019
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions pyls/_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import os
import sys
import threading
import re

import jedi

Expand Down Expand Up @@ -198,3 +199,8 @@ def is_process_alive(pid):
return e.errno == errno.EPERM
else:
return True


def camel_to_underscore(camelcase):
s1 = re.sub('([^_])([A-Z][a-z]+)', r'\1_\2', camelcase)
return re.sub('([a-z0-9])([A-Z])', r'\1_\2', s1).lower()
337 changes: 337 additions & 0 deletions pyls/plugins/importmagic_lint.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,337 @@
# Copyright 2017 Palantir Technologies, Inc.
import logging
import re
import sys
import tokenize
from concurrent.futures import ThreadPoolExecutor
import importmagic
from pyls import hookimpl, lsp, _utils


log = logging.getLogger(__name__)

SOURCE = 'importmagic'
ADD_IMPORT_COMMAND = 'importmagic.addimport'
REMOVE_IMPORT_COMMAND = 'importmagic.removeimport'
MAX_COMMANDS = 4
UNRES_RE = re.compile(r"Unresolved import '(?P<unresolved>[\w.]+)'")
UNREF_RE = re.compile(r"Unreferenced import '(?P<unreferenced>[\w.]+)'")

_index_cache = {}


class _SourceReader(object):
# Used to tokenize python source code
def __init__(self, source):
self.lines = re.findall(r'[^\n]*\n?', source)
# To pop lines later
self.lines.reverse()

def readline(self):
if self.lines:
return self.lines.pop()
return ''


def _build_index(paths):
"""Build index of symbols from python modules.
"""
log.info("Started building importmagic index")
index = importmagic.SymbolIndex()
index.build_index(paths=paths)
log.info("Finished building importmagic index")
return index


def _cache_index_callback(future):
# Cache the index
_index_cache['default'] = future.result()


def _get_index():
"""Get the cached index if built and index project files on each call.
Return an empty index if not built yet.
"""
# Index haven't been built yet
index = _index_cache.get('default')
if index is None:
return importmagic.SymbolIndex()

# Index project files
# TODO(youben) index project files
# index.build_index(paths=[])
return _index_cache['default']


def _get_imports_list(source, index=None):
"""Get modules, functions and variables that are imported.
"""
if index is None:
index = importmagic.SymbolIndex()
imports = importmagic.Imports(index, source)
imported = [i.name for i in list(imports._imports)]
# Go over from imports
for from_import in list(imports._imports_from.values()):
imported.extend([i.name for i in list(from_import)])
return imported


def _tokenize(source):
"""Tokenize python source code.
Returns only NAME tokens.
"""
readline = _SourceReader(source).readline
return [token for token in tokenize.generate_tokens(readline) if token[0] == tokenize.NAME]


def _search_symbol(source, symbol):
"""Search symbol in python source code.

Args:
source: str object of the source code
symbol: str object of the symbol to search

Returns:
list of locations where the symbol was found. Each element have the following format
{
'start': {
'line': int,
'character': int
},
'end': {
'line': int,
'character': int
}
}
"""
symbol_tokens = _tokenize(symbol)
source_tokens = _tokenize(source)

symbol_tokens_str = [token[1] for token in symbol_tokens]
source_tokens_str = [token[1] for token in source_tokens]

symbol_len = len(symbol_tokens)
locations = []
for i in range(len(source_tokens) - symbol_len + 1):
if source_tokens_str[i:i+symbol_len] == symbol_tokens_str:
location_range = {
'start': {
'line': source_tokens[i][2][0] - 1,
'character': source_tokens[i][2][1],
},
'end': {
'line': source_tokens[i + symbol_len - 1][3][0] - 1,
'character': source_tokens[i + symbol_len - 1][3][1],
}
}
locations.append(location_range)

return locations


@hookimpl
def pyls_initialize():
_index_cache['default'] = None
pool = ThreadPoolExecutor()
builder = pool.submit(_build_index, (sys.path))
builder.add_done_callback(_cache_index_callback)


@hookimpl
def pyls_commands():
return [ADD_IMPORT_COMMAND, REMOVE_IMPORT_COMMAND]


@hookimpl
def pyls_lint(document):
"""Build a diagnostics of unresolved and unreferenced symbols.
Every entry follows this format:
{
'source': 'importmagic',
'range': {
'start': {
'line': start_line,
'character': start_column,
},
'end': {
'line': end_line,
'character': end_column,
},
},
'message': message_to_be_displayed,
'severity': sevirity_level,
}

Args:
document: The document to be linted.
Returns:
A list of dictionaries.
"""
scope = importmagic.Scope.from_source(document.source)
unresolved, unreferenced = scope.find_unresolved_and_unreferenced_symbols()

diagnostics = []

# Annoyingly, we only get the text of an unresolved import, so we'll look for it ourselves
for unres in unresolved:
for location_range in _search_symbol(document.source, unres):
diagnostics.append({
'source': SOURCE,
'range': location_range,
'message': "Unresolved import '%s'" % unres,
'severity': lsp.DiagnosticSeverity.Hint,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this not be a warning / error? Or does it overlap with e.g. pyflakes results

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a hint because we actually don't know if this is a module to be imported or just a non defined variable, so better not to trigger a false positive. The unreferenced symbols are flagged with Warning because we know in advance if the symbol is either an import or a varialbe/function and the error should be consistent.

})

for unref in unreferenced:
for location_range in _search_symbol(document.source, unref):
# TODO(youben) use jedi.names to get the type of unref
# Find out if the unref is an import or a variable/func
imports = _get_imports_list(document.source)
if unref in imports:
message = "Unreferenced import '%s'" % unref
else:
message = "Unreferenced variable/function '%s'" % unref

diagnostics.append({
'source': SOURCE,
'range': location_range,
'message': message,
'severity': lsp.DiagnosticSeverity.Warning,
})

return diagnostics


@hookimpl
def pyls_code_actions(config, document):
"""Build a list of actions to be suggested to the user. Each action follow this format:
{
'title': 'importmagic',
'command': command,
'arguments':
{
'uri': document.uri,
'version': document.version,
'startLine': start_line,
'endLine': end_line,
'newText': text,
}
}
"""
# Update the style configuration
conf = config.plugin_settings('importmagic_lint')
min_score = conf.get('minScore', 1)
log.debug("Got importmagic settings: %s", conf)
importmagic.Imports.set_style(**{_utils.camel_to_underscore(k): v for k, v in conf.items()})

# Get empty index while it's building so we don't block here
index = _get_index()
actions = []

diagnostics = pyls_lint(document)
for diagnostic in diagnostics:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if it's easier to just call pyls_lint(document), then try to find the context diagnostic in the results. That way we don't have to parse the messages, just have to do an equality check.

e.g. action_diags = [diag for diag in pyls_lint(document) if diag in context.get('diagnostics', [])]

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True that the diagnostics list can grow bigger than the local diagnostics and we will just pass most of them. Also we don't need to do the equality check if we call pyls_lint() as all the diagnostics will be processed.

message = diagnostic.get('message', '')
unref_match = UNREF_RE.match(message)
unres_match = UNRES_RE.match(message)

if unref_match:
unref = unref_match.group('unreferenced')
actions.append(_generate_remove_action(document, index, unref))
elif unres_match:
unres = unres_match.group('unresolved')
actions.extend(_get_actions_for_unres(document, index, min_score, unres))

return actions


def _get_actions_for_unres(document, index, min_score, unres):
"""Get the list of possible actions to be applied to solve an unresolved symbol.
Get a maximun of MAX_COMMANDS actions with the highest score, also filter low score actions
using the min_score value.
"""
actions = []
for score, module, variable in sorted(index.symbol_scores(unres)[:MAX_COMMANDS], reverse=True):
if score < min_score:
# Skip low score results
continue
actions.append(_generate_add_action(document, index, module, variable))

return actions


def _generate_add_action(document, index, module, variable):
"""Generate the patch we would need to apply to import a module.
"""
imports = importmagic.Imports(index, document.source)
if variable:
imports.add_import_from(module, variable)
else:
imports.add_import(module)
start_line, end_line, text = imports.get_update()

action = {
'title': _add_command_title(variable, module),
'command': ADD_IMPORT_COMMAND,
'arguments': [{
'uri': document.uri,
'version': document.version,
'startLine': start_line,
'endLine': end_line,
'newText': text
}]
}
return action


def _generate_remove_action(document, index, unref):
"""Generate the patch we would need to apply to remove an import.
"""
imports = importmagic.Imports(index, document.source)
imports.remove(unref)
start_line, end_line, text = imports.get_update()

action = {
'title': _remove_command_title(unref),
'command': REMOVE_IMPORT_COMMAND,
'arguments': [{
'uri': document.uri,
'version': document.version,
'startLine': start_line,
'endLine': end_line,
'newText': text
}]
}
return action


@hookimpl
def pyls_execute_command(workspace, command, arguments):
if command not in [ADD_IMPORT_COMMAND, REMOVE_IMPORT_COMMAND]:
return

args = arguments[0]

edit = {'documentChanges': [{
'textDocument': {
'uri': args['uri'],
'version': args['version']
},
'edits': [{
'range': {
'start': {'line': args['startLine'], 'character': 0},
'end': {'line': args['endLine'], 'character': 0},
},
'newText': args['newText']
}]
}]}
workspace.apply_edit(edit)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think newer versions of clients support putting the edit in the code action:

/**
 * A code action represents a change that can be performed in code, e.g. to fix a problem or
 * to refactor code.
 *
 * A CodeAction must set either `edit` and/or a `command`. If both are supplied, the `edit` is applied first, then the `command` is executed.
 */
export interface CodeAction {

	/**
	 * A short, human-readable, title for this code action.
	 */
	title: string;

	/**
	 * The kind of the code action.
	 *
	 * Used to filter code actions.
	 */
	kind?: CodeActionKind;

	/**
	 * The diagnostics that this code action resolves.
	 */
	diagnostics?: Diagnostic[];

	/**
	 * The workspace edit this code action performs.
	 */
	edit?: WorkspaceEdit;

	/**
	 * A command this code action executes. If a code action
	 * provides an edit and a command, first the edit is
	 * executed and then the command.
	 */
	command?: Command;
}

But I'm not sure which client capability we should check. What you've got now is probably safer



def _add_command_title(variable, module):
if not variable:
return 'Import "%s"' % module
return 'Import "%s" from "%s"' % (variable, module)


def _remove_command_title(import_name):
return 'Remove unused import of "%s"' % import_name
3 changes: 3 additions & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@
'all': [
'autopep8',
'flake8',
'importmagic',
'mccabe',
'pycodestyle',
'pydocstyle>=2.0.0',
Expand All @@ -58,6 +59,7 @@
],
'autopep8': ['autopep8'],
'flake8': ['flake8'],
'importmagic': ['importmagic'],
'mccabe': ['mccabe'],
'pycodestyle': ['pycodestyle'],
'pydocstyle': ['pydocstyle>=2.0.0'],
Expand All @@ -80,6 +82,7 @@
'autopep8 = pyls.plugins.autopep8_format',
'folding = pyls.plugins.folding',
'flake8 = pyls.plugins.flake8_lint',
'importmagic = pyls.plugins.importmagic_lint',
'jedi_completion = pyls.plugins.jedi_completion',
'jedi_definition = pyls.plugins.definition',
'jedi_hover = pyls.plugins.hover',
Expand Down
Loading