microsoft / language-server-protocol

Defines a common protocol for language servers.
https://microsoft.github.io/language-server-protocol/
Creative Commons Attribution 4.0 International
11.12k stars 784 forks source link

Feature Request: A mechanism for marking code as unused, but not actionable #2026

Open jfly opened 1 day ago

jfly commented 1 day ago

In a language like Python, there are scenarios where you end up with unused variables that are still very much necessary. For example, if you're implementing a method signature, you can end up defining parameters that are unused, but still must be be there in order to preserve the arity of the function. (Furthermore, you often are not even free to rename them to something obviously unused because Python supports invoking functions with named parameters.)

It's nice for IDEs to be able to give a visual indicator that these variables are unused without telling the user that there's something actionable here.

There seems to be disagreement in the ecosystem about if the Language Server Protocol is capable of expressing this. I've read everything I could on this, and my opinion is that it does not.

One of Neovim's maintainers sought clarity on this in https://github.com/microsoft/language-server-protocol/issues/1696, and the issue was closed with this message:

I will close the issue since I really don't want to enforce UI rendering in the LSP specification.

I totally get that UI rendering is not in scope for the Language Server Protocol. But perhaps the concept of "actionability" is? Is there some way we can clarify or change the Language Server Protocol to support this?

A few proposals, in no particular order:

  1. Update the docstring on DiagnosticTag.Unnecessary to clarify that is it non-actionable.
    • This would be consistent with what Pyright does: it emits this diagnostic at (DiagnosticSeverity.Hint, DiagnosticTag.Unnecessary). Relevant code here.
    • This would be inconsistent with what rust-analyzer does: it emits this diagnostic at (DiagnosticSeverity.Warning, DiagnosticTag.Unnecessary). Relevant code: here's the DiagnosticTag, the severity ultimately comes from cargo check, which emits this as a "warning".
  2. Update the docstring on DiagnosticTag.Unnecessary to clarify that it is non-actionable with DiagnosticSeverity.Hint, but is actionable at other severities.
    • This would be consistent with both Pyright and rust-analyzer. I haven't explored other LSP servers, but would be happy to do so if there's interest in moving this forward.
  3. Add a new DiagnosticTag.Unused (or perhaps DiagnosticTag.Unreferenced) that is clearly documented as non-actionable.
    • If we made this change, ideally we'd also clarify that DiagnosticTag.Unnecessary is actionable. Or we deprecate DiagnosticTag.Unnecessary and add an alternative that is clearly documented as actionable.
    • For the record, I proposed something similar to this in https://github.com/microsoft/language-server-protocol/issues/2025 before I fully understood the issue.
  4. Deprecate DiagnosticTag entirely, instead do this with semantic tokens.

Does this feel like something that could be in scope for the Language Server Protocol? If so, I'd be happy to help move this forward.

mfussenegger commented 11 hours ago

Afaik eclipse.jdt.ls and luals also use DiagnosticTag.Unnecessary for actionable diagnostic. There are probably others too. Seems to me that pyright is the odd duck here.

I'd also question a bit why there'd need to be a diagnostic at all if something is legitimately/intentionally unused. What'd be the point of a DiagnosticTag.Unused over just not emitting any diagnostic? If it isn't actionable it would require lots of special handling on the client side - for what?

there are scenarios where you end up with unused variables that are still very much necessary. For example, if you're implementing a method signature, you can end up defining parameters that are unused, but still must be be there in order to preserve the arity of the function.

This sounds a bit as a more general ambiguity problem. How is a server supposed to know if something is intentionally unused or not, without some additional hint from the person who wrote the code?

E.g. some languages allow to prefix intentionally unused variables with _.