Put flake8 behind LSP

[karthiknadig]

See what it will take to put the linters behind LSP. As a part of the experiment put flake8 behind LSP.

[karthiknadig]

Did some initial investigation and looks like we have to discuss some UX when it comes to getting linters on user machine. Things to discuss:

1. Currently we provide a command to set linters. Upon selecting a linter we install the said linter into the user's selected python environment. With the new approach we don't have to install linter into user's environment. We have it already installed in the linting extension. We may have to point the user to installed linting extension. The command itself still has purpose in providing more details, like it could show the already installed linting extensions and allow users to choose from that.
2. Respecting existing settings, we have several settings specific to linters and we need to decide which ones we want to support.
   • python.linting.enabled: Global switch for linters. Can be used to turn off all linters.
   • python.linting.<linter>Enabled: Switch to turn on/off individual linters.
   • python.linting.<linter>Path: Path to specific linting binary. With the new linting extension, this can be a path to binary (in which case we will attempt to find the importable library), path to dir containing the importable binary, or <linter> in which case it will wither use the linter installed in the environment, or use the one shipped with the extension.
   • python.linting.<linter>Args: Additional arguments to linters.
   • python.linting.<linter>CategorySeverity: Translation between linter severity and VS Code's severity.
3. We may need to provide a way for users to control when they get linting, onType, onSave, or manual. We need to decide if this is enough or we need more. For manual we can use the existing Run Linter command to do linting on demand.
4. Logging for linting via LSP, and linters will have their own Output panel.

[karthiknadig]

We have a prototype for flake8 behind LSP that adds ability to control severity by error code. Instructions to get it and use it are here: PyCQA/flake8#1467 (comment)

[jaklan]

just the copy-paste from the flake8 thread, as it was hidden there

@karthiknadig @brettcannon I see the new extension has exactly the same problem as the current built-in implementation - python.flake8Path doesn't accept the name of the executable to run (FileNotFoundError: [Errno 2] No such file or directory: 'flake8'), so you can't just specify e.g. flake8 - you need to specify the full path instead of just allowing PATH to do the job.

What's the actual use-case? All the tools like black, mypy, tox, pytest, isort, coverage etc. provide support for pyproject.toml-based configuration, as it becomes a standard approach nowadays to maintain only one file. All, but not flake8, as @asottile continues his fight against TOML (and yes, I know the thread, but sorry - other maintainers were able to add the support, in a native way or the legacy one like tox, but @asottile is the only one who prefers to stay against the community just because of his personal crusade, even when the PR was already done).

It's extremely annoying attitude, but anyway - there's a solution for that: pflake8, so the flake8 wrapper, which adds the support for pyproject.toml. It works pretty well also with VS Code, but because of the above issue - I need to provide the full path like ./.venv/bin/pflake8, which makes the config tied to the very concrete venv, so I can't set it globally as pflake8 to allow finding that on PATH, I also can't keep the workspace-level settings.json in a generic form, shareable with other teammates.

[karthiknadig]

@jaklan We have not manually added any restriction to python.flake8Path. We pass whatever is set in python.flake8Path to subprocess.run. There might be some bug in the prototype where it is not doing the right thing. The point of the prototype was to demo flake8 over LSP and how the integration will look like. That build is not a final product that we will be shipping.

[jaklan]

@karthiknadig the problem is it's not passed to subprocess.run. When you specify the path as flake8 / pflake8 and look at logs, you will see the command doesn't include -p flake8 (p)flake8 but still -m flake8. When we look at the old linter script, we can see invoke = sys.argv[1] line - it should be -p in that case, but it's still -m. It looks like the setting is completely ignored and doesn't reach the linter script.

What determines the value of sys.argv[1]? It has to come from some setting parsing logic on the VSCode side imho.

[karthiknadig]

You can see the entire source code in the extension directory. It should be under ~/.vscode/extensions/python-flake8<version>/bundled/linter/linter_server.py. No there is no VS Code logic that does anything here. When the linter server is launched all the settings are sent, as is, to linter_server.py. There is likely a bug in linter_server.py where it is not constructing the args as needed by subprocess.run.

You are missing the point of this exercise. This issue and the prototype were created to look at how linting over LSP will look like and what we want to do about the ownership of the LSP part and the extension part. Feedback is definitely welcome, but I don't want that to detract from the actual discussion.

We designed this specifically with python community in mind. So, most of the real work in the python code. We don't do anything other than hooking up the LSP in the TS code. You should be able to change anything, detect any additional paths, change environment variables, multiplex multiple linters, or literally anything else from the python code. As long as the messages between VS Code and linter_sertver.py are conforming to LSP it will work. The TS code is tooling agnostic.

[jaklan]

@karthiknadig I understand the goal, but in the flake8 repository you just mentioned this thread as a place to share feedback about the PoC, so I thought it's worth to mention this problem as it's also related to the new extension, but ofc I can create a separate issue for that, whatever you prefer 😉

Yes, I was reading the code of both old and new linter implementation, but I'm just still missing the one thing I mentioned above - sys.argv[1] is either -m or -p, but you don't specify that in the config directly - it's somewhere deducted based on the value of python.flake8Path (or lack of it). So my question is - where does it come from, where's the logic responsible just for selecting this flag?

[karthiknadig]

I am confused, which logs are you talking about? Output > Python logs or output > flake8 logs? I just tested with "python.flake8Path": ["flake8"] and it seems to work well for me:

Python extension (i.e, specifically ms-python.python extension) linter implementation has the logic where it checks flake8 and uses -m. The flake8 LSP prototype does not do that.

[brettcannon]

@jaklan all of our work will end up being open source, so if you want an extension for pflake8 it should be very straightforward for you to create.

[jaklan]

@brettcannon sure, but if it's only a matter of specifying the name of the executable then there's no sense to create an additional extension doing exactly the same.

@karthiknadig yeah, I was referring to the old extension above, sorry for the confusion. Regarding the new one - I still have some strange issues with python.flake8Path:

• I've created a new, clean project
• I've created a new Python venv and installed both flake8 and pflake8

(.venv) bash-5.1$ which python
/Users/lanskij/Repositories/test/.venv/bin/python
(.venv) bash-5.1$ pyenv which python
/Users/lanskij/.pyenv/versions/3.9.9/bin/python
(.venv) bash-5.1$ pip freeze
flake8==4.0.1
mccabe==0.6.1
pycodestyle==2.8.0
pyflakes==2.4.0
pyproject-flake8==0.0.1a2
toml==0.10.2

• I've created a settings.json file in .vscode with the following content:

{
    "python.linting.flake8Enabled": false,
    "python.flake8Path": [
        "flake8"
    ]
}

• the selected interpreter is also the proper one:

• but when I try to save & format a sample file, I get:

Traceback (most recent call last):
  File "/Users/lanskij/.vscode/extensions/kanadig.python-flake8-0.0.1/bundled/libs/pygls/protocol.py", line 316, in _handle_notification
    self._execute_notification(handler, params)
  File "/Users/lanskij/.vscode/extensions/kanadig.python-flake8-0.0.1/bundled/libs/pygls/protocol.py", line 223, in _execute_notification
    handler(*params)
  File "/Users/lanskij/.vscode/extensions/kanadig.python-flake8-0.0.1/bundled/linter/linter_server.py", line 214, in did_save
    _lint_and_publish_diagnostics(server, params)
  File "/Users/lanskij/.vscode/extensions/kanadig.python-flake8-0.0.1/bundled/linter/linter_server.py", line 183, in _lint_and_publish_diagnostics
    diagnostics = _get_diagnostics_using_linter_path(text_document)
  File "/Users/lanskij/.vscode/extensions/kanadig.python-flake8-0.0.1/bundled/linter/linter_server.py", line 154, in _get_diagnostics_using_linter_path
    raise ex
  File "/Users/lanskij/.vscode/extensions/kanadig.python-flake8-0.0.1/bundled/linter/linter_server.py", line 148, in _get_diagnostics_using_linter_path
    result = subprocess.run(
  File "/Users/lanskij/.pyenv/versions/3.9.9/lib/python3.9/subprocess.py", line 505, in run
    with Popen(*popenargs, **kwargs) as process:
  File "/Users/lanskij/.pyenv/versions/3.9.9/lib/python3.9/subprocess.py", line 951, in __init__
    self._execute_child(args, executable, preexec_fn, close_fds,
  File "/Users/lanskij/.pyenv/versions/3.9.9/lib/python3.9/subprocess.py", line 1821, in _execute_child
    raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'flake8'

I have verified the PATH value with linting_lsp.show_message_log(str(os.environ["PATH"])) and I got:

/Users/lanskij/.pyenv/shims:/Users/lanskij/.poetry/bin:/Users/lanskij/.local/bin:/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin:/opt/X11/bin:/usr/local/opt/fzf/bin

so it seems the venv is not taken into account (== not added to the PATH), so then being able to specify the executable works only for global packages. Is it a bug or is it expected? Personally I would expect the activated venv to be on the PATH.

[karthiknadig]

@jaklan If you have a python virtual environment with flake8 installed then you should not be using python.flake8path. The linting server will automatically use the flake8 installed in the environment.

With the prototype extension python.flake8path should only be used if you really need do. the prototype extension itself ships with flake8, and it picks up flake8 from selected environments. The path should be used as an exception not the norm. For cases where people have global flake8, will in those cases you have it in global PATH so it should work there as well.

For now, the server is just using exec instead of shell exec (which will allow activated launches). we avoided those to reduce load times. some scenarios the activation time for environments can exceed 30 seconds.

[jaklan]

@karthiknadig okay, so we have a different logic for using module and path, where the first one tries to find the executable in the current venv and falls back to the shipped one if missing, but the latter doesn't deal with the current venv at all. So it seems to be a deadlock for the pflake8 scenario, when you would like to go with the module flow, just with another executable name 😕

Just out of curiosity - if activating the venv would be problematic for the path scenario due to time, how is it different in the module scenario, where we actually take that into consideration?

[karthiknadig]

In the module scenario we are not activating. we are directly launching python executable from the virtual env folder. That python binary often does the work of adding the right site-packages directory to the sys.path. So, we really don't need to activate by running the activate script.

There is another way, launch vscode from an activated terminal. Also, note once we create a repository for the prototype, community can make suggestions. IF the community feels that the LSP should run from an activate environment then we may do that. Again, the point of this whole thing was that it is going to be easier to make contribution because most of the code will sit in python, with TS part just doing the LSP hook up.

[jaklan]

@karthiknadig sure, waiting for the official release then to re-raise the issue, thanks for all the explanations, looking forward to contribute!