Unused tag applied to too big range

[rchl]

In #229 support for DiagnosticTag was introduced for flake8 but the range in those diagnostics is not quite right.

With a file like (requires "pylsp.plugins.flake8.enabled": true):

from typing import Any, List


def foo() -> Any:
    return 1

the whole from typing line is marked with "unused" tag while only the List import is unused.

Here is how it looks with pylsp:

Here is how it looks with pyright:

@krassowski @ccordoba12

[rchl]

Diagnostic payload:

{
  "diagnostics": [
    {
      "code": "F401",
      "message": "F401 'typing.List' imported but unused",
      "range": {
        "end": {
          "character": 29,
          "line": 0
        },
        "start": {
          "character": 0,
          "line": 0
        }
      },
      "severity": 1,
      "source": "flake8",
      "tags": [
        1
      ]
    }
  ],
  "uri": "file:///Users/rafal/Documents/testpy/test.py"
}

[rchl]

Looking at the result, I suppose it might be more flake8 issue that it reports diagnostic for the whole line.

[krassowski]

Yes, nothing with support for diagnostic tags, just the old issue of missing precise positions for diagnostics themselves. Note: flake8 is just a wrapper around pyflakes and friends.

Over a year ago I opened a PR to expose the metadata we need (start and end position) upstream PyCQA/pyflakes#649 for an issue which is even older. It was meat with rather little enthusiasm by the maintainers at the time, suggesting that they would reconsider once they can drop Python 3.7 (EOL is June 2023) but they still support 3.6 (which is EOL for 8 months now). You can try to nudge/rebase that PR with hope they would reconsider earlier - after all this is not a user facing change (not visible in pyflakes reports).

Otherwise, we could just monkeypatch here (being very defensive - wrapping everything in big try-except) if @ccordoba12 agrees.

[ccordoba12]

Otherwise, we could just monkeypatch here (being very defensive - wrapping everything in big try-except) if @ccordoba12 agrees.

Yeah, I think it'd be very nice to have support for this here because it's confusing for users not being able to see the exact position where an error/warning occurs.

So, I'm quite +1 on this @krassowski.

[Wuestengecko]

Hi! I noticed that something very similar applies to the pylint plugin as well, when it marks unused variables and function parameters. It does correctly set the start position in this case, but the highlighting extends until end of line, which can be very confusing:

This one might be easier to fix; considering we already have an accurate start position, we could derive the end position by looking for the end of the "word" – which, if I'm not missing something, should always be an identifier (i.e. word.isidentifier() is True).

[ccordoba12]

@Wuestengecko, we're very busy at the moment but you're welcome to send us a pull request with a possible fix to take a look at it.