Add support for pylint error ranges

[cdce8p]

With the next pylint release 2.12.0 (hopefully this weekend), we are adding support for error ranges. I.e. end_line and end_column information for error messages. Tracking issue: pylint-dev/pylint#5336

This PRs adds support for those to allow for better error highlighting when using pylint in VSCode.

Before I continuing to work on it, I wanted to see if there is interest in adding this to vscode-python and if the approach is the correct one.

Related: #18027

--

Background

Starting with 2.12.0, the --msg-template option will accept two new parameters end_line and end_column. Both return either an int (from the AST) or an empty string (if unknown). Unfortunately, we are unable to make this option backwards compatible due to the way it was implemented many years ago. Adding end_line to --msg-template in versions prior to 2.12.0 will raise a KeyError.

[kimadeline]

Hi @cdce8p, thank you for the PR!

We're currently discussing this with the team:

Ideally we would like to have dynamic support for error ranges without having to rely on a setting, but that would require a pylint version check. Since our current support isn't stateful we would have to perform that check for every run of pylint, which is something we want to avoid.

[cdce8p]

Hi @cdce8p, thank you for the PR!

We're currently discussing this with the team:

Ideally we would like to have dynamic support for error ranges without having to rely on a setting, but that would require a pylint version check. Since our current support isn't stateful we would have to perform that check for every run of pylint, which is something we want to avoid.

@kimadeline Thanks for the reply. I though that this would be the case as I don't like too much either. I'll need to check tomorrow but there could be another option. Pylint does have some alternative report options, in particular the builtin json-reporter, that might not have this issue. If that doesn't work, it would be possible to add a custom one as plugin that is backwards compatible. You would then only need to make sure that it's installed in the environment, similar to the requirements for jupyter notebook.

It won't be much help for this PR, but we fixed the backwards incompatibility issue with --msg-template for future updates.

[brettcannon]

@cdce8p sorry for the delayed reply; I wrote out an entire reply and I though It clicked "Comment", but it isn't here so I must have forgotten to post it. 😬

The key thing is we are looking to rewrite out Pylint support in hopefully the next couple of months. As such, we don't want to add another setting that we won't need in a little bit (as we can handle this transparently with the rewrite as we will be putting Pylint behind LSP). But if you wanted to do the work to move our support over to the JSON reporter and see if that can be used to transparently support the new range feature then we can have a look at the PR.

[cdce8p]

I've updated the PR to parse the JSON output for pylint. The pylint PR to add endLine and endColumn to the output has also been merged today (pylint-dev/pylint#5456). Will be included in the next release 2.12.2. Since this change is backwards compatible, it doesn't need to wait for the pylint release (although we hope to release 2.12.2 this weekend).

There is a slight caveat, the json output doesn't include a score. Some users might miss it. IMO though, the benefit of having error ranges far outweigh this. Especially considering that the score for a single file isn't all that useful.

In case this is accepted, we, from the pylint team, would love it if these changes could be included in the next release. Please let me know if I can help make that happen.

Update
2.12.2 was just released and is available on PyPI.

[brettcannon]

Unfortunately the cut-off for features for the next release passed on Monday, so if we accept this it would come out in our 2022.2 release which would be the first Wednesday/Thursday of February.

[cdce8p]

@brettcannon Thanks for letting me know. I understand. I had hoped the cutoff would be next week 😞

[brettcannon]

@cdce8p you can always look up when our feature freeze is by looking at our latest release plan. The one for the release going out this week is #18003.

[kimadeline]

Thank you for amending your PR @cdce8p!

It looks good to me. I left a few linting and nitpicky comments here and there, and we should be good to merge it afterwards.

[cdce8p]

@kimadeline Thanks for the review! I've updated the PR and left some comments on open questions.
Do you have an idea why the venv tests fail? I don't think I've changed anything there.

[brettcannon]

@cdce8p the venv failures are probably due to VS Code 1.63.0 going out (our tests can be rather sensitive around file watching and that got an update in this release).

[kimadeline]

Thank you for your contribution, and for the quick turnaround!

As Brett pointed out, the test failures are not caused by your PR, so I'm going to go ahead and merge it.

[jrom99]

I don't know if this is the right place to ask, but pylint/VSCode highlighting behaviour changed this morning for me and I couldn't find on the documentation (pylint --long-help) how to change this setting.

I'd like to ask if it's possible to only enable it for some message categories (e.g.: fully highlight only errors and warnings, highlight only first column/word of hints/refactors) or to set it to highlight only first column on all message categories.

In this screenshot, the blue highlight is due to the class's missing docstring, which is a desired message for me, but places too much clutter my editor, so I'd find it better to only highlight the first column as was done before.

VS Code information

Version: 1.63.2
Commit: 899d46d82c4c95423fb7e10e68eba52050e30ba3
Date: 2021-12-15T09:39:46.686Z
Electron: 13.5.2
Chromium: 91.0.4472.164
Node.js: 14.16.0
V8: 9.1.269.39-electron.0
OS: Linux x64 5.15.18-200.fc35.x86_64

Python and pylint information

#:pylint --version
pylint 2.12.2
astroid 2.9.3
Python 3.9.10 (main, Jan 17 2022, 00:00:00) 
[GCC 11.2.1 20211203 (Red Hat 11.2.1-7)]

#:python -m pylint --version
pylint 2.12.2
astroid 2.9.3
Python 3.10.2 (main, Jan 17 2022, 00:00:00) [GCC 11.2.1 20211203 (Red Hat 11.2.1-7)]

Python and pylint settings on VSCode

    "python.analysis.typeCheckingMode": "basic",
    "python.analysis.stubPath": "/home/junior/Documents/Projetos/Python/typings",
    "python.linting.pylintArgs": [
        "--disable=no-name-in-module,line-too-long,invalid-name,import-error,unused-argument,logging-fstring-interpolation,no-else-return",
        "--max-parents=12"
        // "--extension-pkg-allow-list=PyQt5"
    ],
    "python.linting.pycodestyleArgs": [
        "--ignore=E501,E226,W504"
    ],
    "python.linting.pylintEnabled": true,
    "python.analysis.diagnosticMode": "workspace"

[DanielNoord]

Adding the highlighting only for selected messages is probably something that VSCode would need to implement.

However, you'll be happy to know that we're planning on changing the ranges for class and function definitions in the next version of pylint. See pylint-dev/pylint#5466.
We had hoped to release the next version a little sooner, but it's taking longer than expected.

For those who are interested, we would welcome a PR that targets this exact issue!

[jrom99]

For a start, it would already be an improvement to modify add_message so the error is only emitted for the first line. It won't be perfect but definitely an improvement.

Do you know if VSCode uses the class/function definition or uses this add_message to set the highlighting range? If it is the latter, wouldn't it make more sense to have it as a setting in pylint itself? (Sorry for my lack of knowledge on this topic, I have no idea if this setting in VSCode is even language aware and was caught by surprise with the change)

[DanielNoord]

No, VSCode only parses the output generated by pylint. The only things VSCode has access to after this PR (I think) are:

             code: outputMsg.symbol,
             message: outputMsg.message,
             column: outputMsg.column === null || outputMsg.column <= 0 ? 0 : outputMsg.column - colOffset,
             line: outputMsg.line,
             type: outputMsg.type,
             provider: this.info.id,
             endLine: outputMsg.endLine === null ? undefined : outputMsg.endLine,
             endColumn: outputMsg.endColumn,

See L50 in src/client/linters/pylint.ts.

So if VSCode was ever to change this I think it would only be possible on a per-message basis, without this becoming too complicated.

[DanielNoord]

You can always make a proposal/issue in pylint itself if you want a message to have a different range. For example, if you want the invalid-name message to only display on the first letter of the invalid name you could make a proposal to do so, although for this particular example I think we would deny the request.

The "message ranges" are defined when we call add_message in pylint internally, so we can change stuff there. It's only a matter of reaching an agreement about whether stuff is actually an improvement. For pylint-dev/pylint#5466 the consensus is clearly there.

[jrom99]

You can always make a proposal/issue in pylint itself if you want a message to have a different range.

Thanks, do you recommend I open an issue specific to the Class/function docstring ranges or leave it as it's already mentioned in pylint-dev/pylint#5466?

Would it make sense to have add_message output different endColumn and endLine values based on a per message (value or type) setting that accepts a value between [single column, first word, first line, end/default range]?

I've read the add_message definition in https://github.com/PyCQA/pylint/blob/d7013f6799d28a69dbbf3329da7867a8afacce82/pylint/lint/pylinter.py#L1557 and it just seems to pass through the node settings, so I don't know if it would have enough context about where the first word/token ends, though. And thinking again, maybe "first line" would be ambiguous for multi-line definitions like:

[DanielNoord]

You can always make a proposal/issue in pylint itself if you want a message to have a different range.

Thanks, do you recommend I open an issue specific to the Class/function docstring ranges or leave it as it's already mentioned in PyCQA/pylint#5466?

If it's only about those I think the current issue is good enough.

Would it make sense to have add_message output different endColumn and endLine values based on a per message (value or type) setting that accepts a value between [single column, first word, first line, end/default range]?

I've read the add_message definition in https://github.com/PyCQA/pylint/blob/d7013f6799d28a69dbbf3329da7867a8afacce82/pylint/lint/pylinter.py#L1557 and it just seems to pass through the node settings, so I don't know if it would have enough context about where the first word/token ends, though. And thinking again, maybe "first line" would be ambiguous for multi-line definitions like:

I'm not sure what the best approach is here (and I think this discussion should continue in the pylint repository) but based on the node itself and its type (either ClassDef and FunctionDef) we could try and pass other data to the add_message function. What data this should be and how configurable it should be is something to discuss though!

[brettcannon]

I just wanted to quickly thank @DanielNoord for popping over and helping out with the discussion!