pylint-dev / pylint

It's not just a linter that annoys you!
https://pylint.readthedocs.io/en/latest/
GNU General Public License v2.0
5.32k stars 1.14k forks source link

Pylint over Language Server Protocol and VS Code extension #5796

Closed karthiknadig closed 2 years ago

karthiknadig commented 2 years ago

Question

The Python team for VS Code has been working on putting various tools we support behind LSP and breaking them out into their own extensions This work is now far enough along that we wanted to share the extension prototypes for your tool with you and see if you had any interest in owning the extension or the Python-based LSP server backing it yourself?

If you don't want to take on that responsibility that's totally fine and understandable! If you want to leave the extension to us we would then publish the extension on the VS Code Marketplace ourselves. We also plan to publish the extension as open source and support the extension for as long as we felt it made sense to based on usage numbers. But due to the difficulty of migrating extension ownership, we wanted to ask early whether you wanted to own the extension from the start? If you did want to own the extension we would give you the code, help you publish to the VS Code Marketplace, and do anything else we can to help you be successful with this.

We have attached a zip file to this issue containing a prototype extension to give you an idea of what we are planning. You can download it, extract the .vsix file, and then install the VSIX to try it out python-pylint.zip. This prototype version was built with python 3.10 and bundles pylint v2.12.2 (as fallback if nothing is found in your environment).

To be very clear, there's no pressure or expectations from us. We just want to make sure you as a project had an opportunity to own this extension instead of us from the get-go before the extension gained traction.

Documentation for future user

Nothing for now

Additional context

No response

Pierre-Sassoulas commented 2 years ago

Hello, thank you for opening the issue.

I think it's a good thing that the VS code related code is in another repository. I don't have any knowledge about VS code and pylint integration in VS code, or even VS code extension maintenance. Thank you for doing this !

We have a similar issue with emacs integration. It's in the pylint codebase but as none of the active contributors use emacs on the daily it's basically not maintained. We might want to be consistent and be explicit in the documentation about us not maintaining any IDE integration.

I'm looking forward to working together on issues required for VS code in pylint core though :) #5466 in particular comes to mind, without IDE integration it would not be such a pressing issue :)

brettcannon commented 2 years ago

Another option is LSP support which is editor-agnostic. If that's something you may be interested in then just let us know! But as we said, we understand if you are not interested and we can continue to wrap Pylint ourselves at both the LSP and VS Code level.

Pierre-Sassoulas commented 2 years ago

I like the list of supported IDE for LSP ! There's also an issue open for SARIF (https://github.com/PyCQA/pylint/issues/5493) never heard of it before the issue was opened, is it wildly adopted too ?

DanielNoord commented 2 years ago

@brettcannon I scanned through the overview on that site. What do you mean with "LSP support"? Is that returning the output from pylint in a formatter similar to the JSON response on this page or am I misunderstanding you?

DudeNr33 commented 2 years ago

Can you maybe provide (ideally relatively simple) example code of some static analysis tool that already has a LSP server VS Code extension? I think this could make it easier to understand what maintaining such an extension means in terms of required skills and effort.

brettcannon commented 2 years ago

@DanielNoord LSP is a client/server protocol over JSON-RPC. So from Pylint's perspective, supporting LSP is probably something along the lines of an --lsp flag or something to spin up an LSP server that can speak the protocol (from LSP's perspective, the editor is the client). pygls is probably the most popular library in helping to implement an LSP server.

Depending how far you go with this, it would be possible to make the boundaries of Pylint internals communicate to the edges of Pylint via LSP messages and let the "server" part handle communicating to the user. For instance, your CLI would effectively act as a server of your internal LSP service and just react to Python data objects instead of via JSON-RPC. That would then make your CLI and LSP server just a front-end for your Pylint LSP service back-end. Or to try and make this a bit more concrete, you would parse your CLI args, construct LSP messages, send them to your internal LSP service code, get back LSP messages, and then format them for the terminal as appropriate.

@DudeNr33 not specifically, but you can actually open up the extension we shared with you and see how it works! That zip file we provided you has a .vsix file inside, but that file is actually just a zip file itself. So just rename the .vsix to .zip, open it up, and poke around! The TypeScript/JavaScript code is actually boilerplate (we are also doing this for flake8 and mypy in the linter space and purposefully made it so you don't have to touch TS/JS). But you can look at the Python code and see what we did to wrap Pylint to get an LSP server on top of it.

Otherwise I don't know of any tools in the Python community that natively supports LSP; everyone has been wrapping tools like Pylint themselves for LSP, e.g. https://github.com/python-lsp/python-lsp-server/blob/develop/pylsp/plugins/pylint_lint.py .

karthiknadig commented 2 years ago

@DanielNoord @DudeNr33 You can find the code that does this in extension\bundled\linter\linter_server.py.

In there, we handle on file open, save, and close LSP requests. Part of this is running pylint using runpy, and then parsing the results and sending them as LSP diagnostics messages. Most of the TS/JS code is there to just get the right python interpreter from the python extension and launch linter_server.py. At this point the code in TS/JS in linter or formatter agnostic.

DanielNoord commented 2 years ago

@brettcannon @karthiknadig I use VS Code myself, so I'm certainly inclined to help out here both to thank you for all you're doing for Python support in VSC as well as make my own life easier. However, I think for now adding LSP support internally in pylint might be a little too much. We're actually dealing with quite a bit of technical debt and issues such as migrating away from optparse and fully typing our codebase are things that I believe should be given priority.

That said, when looking at the extension I saw it uses json as output format for pylint. I know that this format is likely to change in 3.0 because of current inconsistencies (see also #4740 and #4741). Therefore, I wonder if it wouldn't be a good idea/compromise to add a new lsp or json-v2 format in an upcoming version. That would allows us to define a format that works well for the extension and prevent future incompatibility for the extension with either 2.x or 3.x.

I am not sure what other contributors/maintainers think of this, but perhaps adding a json-v2 or --output-future flag could also be a good way to allow other plugins or extensions that use pylint's json format to start supporting the new format. We had a discussion about this previously based on https://github.com/PyCQA/pylint/issues/4741#issuecomment-984110669.

brettcannon commented 2 years ago

I use VS Code myself, so I'm certainly inclined to help out here both to thank you for all you're doing for Python support in VSC as well as make my own life easier.

You're quite welcome! And glad you're a happy user!

I think for now adding LSP support internally in pylint might be a little too much.

We completely understand. We just didn't want to publish something and have you feel like you weren't consulted or that we are trying to take ownership of Pylint in the VS Code Marketplace or in the LSP space if we published the LSP server for Pylint as a separate thing on PyPI.

I wonder if it wouldn't be a good idea/compromise to add a new lsp or json-v2 format in an upcoming version.

How discoverable would it be to notice the differences in the JSON payloads between Pylint 2 and 3? If we can examine the results and do the right thing then we should be fine. We can also have separate Pylint 2 and Pylint 3 support by getting the Pylint version somehow -- at worst via importlib.metadata -- and then choose the appropriate code for the version of Pylint being used (that would implicitly version the JSON format).

DanielNoord commented 2 years ago

I wonder if it wouldn't be a good idea/compromise to add a new lsp or json-v2 format in an upcoming version.

How discoverable would it be to notice the differences in the JSON payloads between Pylint 2 and 3? If we can examine the results and do the right thing then we should be fine. We can also have separate Pylint 2 and Pylint 3 support by getting the Pylint version somehow -- at worst via importlib.metadata -- and then choose the appropriate code for the version of Pylint being used (that would implicitly version the JSON format).

I mean I can think of multiple ways to go about doing this. Just a version key should be enough, right? If it exists check which pylint json format it is (which would allow later iterations) and handle appropriately, if it doesn't assume you're dealing with < 2.13 (or whatever version we would add that key).

I think in https://github.com/PyCQA/pylint/issues/4741 we actually discussed this problem and concluded that extensions maintainers probably won't be too happy to do such checks themselves. That's why we discussed adding a second json format type. Instead of having to check which version an extension is dealing with, it can specify the version it wants and let pylint handle giving the correct version to it.

Now that I'm thinking about it: a --json-output-version=? (defaulting to 1) with a new version key might not be a bad solution after all. That would also allow us to release a version 3 easily. I'll let others chip in here, they might have even better solutions.

Pierre-Sassoulas commented 2 years ago

A tuple for the version is available for compatibility reason here: https://github.com/PyCQA/pylint/blob/main/pylint/__pkginfo__.py#L35. See https://github.com/PyCQA/pylint/issues/4399 and https://github.com/PyCQA/pylint/issues/4420 (TLDR : pylint.__pkginfo__.numversion was not available for 2.8.0, 2.8.1 and 2.8.2, but was before and will be after).

brettcannon commented 2 years ago

Just a version key should be enough, right?

That would work. For us, the key thing is knowing how to properly call Pylint and then how to parse the results appropriately. For an API thing like a specific flag or value on the CLI requires checking the Pylint version upfront before we call Pylint using runpy. For processing on the output we either need to have some versioning or an easy way to detect from the shape of the data what we should do.

karthiknadig commented 2 years ago

We now have a repository setup for this: https://github.com/microsoft/vscode-pylint

karthiknadig commented 2 years ago

Pre-release build is now available via marketplace: https://marketplace.visualstudio.com/items?itemName=ms-python.pylint

Pierre-Sassoulas commented 2 years ago

How discoverable would it be to notice the differences in the JSON payloads between Pylint 2 and 3? If we can examine the results and do the right thing then we should be fine.

It's going to be possible to switch before 3.0 as we're going to add another Json reporter and deprecate the old one in #7087.

I'm going to close this issue as I think the initial purpose is met, but don't hesitate to open another issue to discuss issue with the vscode plugin.