Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
jfly opened this issue Sep 20, 2024 · 2 comments
Open

Comments

@jfly
Copy link

jfly commented Sep 20, 2024

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.

  • Some tools (Pyright, neodim, VS Code, maybe others) treat the pair (DiagnosticSeverity.Hint, DiagnosticTag.Unnecessary) as "non-actionable unused code".
  • Other tools (notably Neovim, I haven't checked other popular IDEs) choose to treat all diagnostics (including the (DiagnosticSeverity.Hint, DiagnosticTag.Unnecessary) pair) as actionable.

One of Neovim's maintainers sought clarity on this in #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.
  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
Copy link

mfussenegger commented Sep 22, 2024

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 _.

@FatBoyXPC
Copy link

If it isn't actionable it would require lots of special handling on the client side - for what?

Currently, in neovim, unused variables get grayed out and they get this message next to them about being unused, and they show up in the diagnostics list. I think the ask here is "I like it being grayed out, but I don't like it being in the list". Given that clients already do (part of) this, I'm not sure what special handing would be required for the graying out of the variables; and regarding the list I'm sure it's easy enough to filter out that particular type? That said, I'm firmly in the camp that would appreciate if my editor let me store a setting to determine if I see these "unactionable items" in my diagnostic list (such as - maybe I expect some to not be actionable but I also want to ensure I didn't accidentally leave some dangling around. I suppose CI could catch this as well but I suppose I should stay on topic).

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

As stated in the initial post, you can't do this with Python if named parameters are used:

(Furthermore, you often are not even free to rename them to something obviously unused because Python supports invoking functions with named parameters.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants