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

Proposal: deprecate DiagnosticTag.Unnecessary, replace with a new DiagnosticTag.Unused #2025

Closed
jfly opened this issue Sep 20, 2024 · 1 comment

Comments

@jfly
Copy link

jfly commented Sep 20, 2024

Tangentially related to #2024

First off, I understand that might not be a practical proposal: it would probably trigger a lot of churn in the LSP ecosystem, and I totally understand if you reject this proposal just because the juice is not worth the squeeze. That said, I haven't been able to find this discussed anywhere, so I still think it's useful to have a conversation.

The LSP Spec v3.18 defines DiagnosticTag.Unnecessary as:

Unused or unnecessary code.

Clients are allowed to render diagnostics with this tag faded out
instead of having an error squiggle.

"Unnecessary" implies "you can get rid of this". "Unused" less so. "Unreferenced" might be an even better word. (I'll use the word "unused" for the rest of this proposal.)

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

IMO, this naming issue is a small piece of all the confusion that resulted in this (terrifyingly long) discussion thread over on Pyright: microsoft/pyright#1118. Don't read it. There's a good summary by @erictraut near the bottom.

I dug into the history of `DiagnosticTag.Unnecessary` to see if this had been discussed before. I don't think it has.
  1. First, @mjbvz introduced the word "Unnecessary" and "Unused" to vscode in
    microsoft/vscode@1a820a0.
    • I can't tell if there was an intentionality between using both words in this commit. It doesn't feel like there's a semantic difference between them, as highlighted by this line of code, which treats them the same:

       const reportUnnecessary = config.get<boolean>('showUnused.enabled', true);
  2. That got reworked into a "proposed" DiagnosticTag enum with a single
    Unnecessary field in
    microsoft/vscode@8bb27cd#diff-298da9f1fcf1413ec8668791d137b170183f304bbd534403e1c04ae952f80050R630.
  3. Next, a vigorous discussion occurred here:
    Add official API for fading out unused code vscode#51104 about making this API
    official.
    • The discussion mostly seems to be about if tags are the right way to represent this, and some discussion about "push" vs "pull" semantics and the corresponding performance implications. I don't see any discussion about the word "unecessary" vs "unused".
  4. Next, @mjbvz promoted that DiagnosticTag to an official API:
    microsoft/vscode@2e5253d#diff-f127724f8c5dbf0c8371ad0a100f8a9bc0a398b6b8ec29aa6cd7f265bd01a096R4001
  5. Next, @mjbvz filed this issue to add DiagnosticTag to the LSP spec: Add DiagnosticTag Type #500
    • I see some discussion here about two way flow of information, but not about the actual name of the tag values.
  6. Next, I see @dbaeumer added DiagnosticTag.Unnecessary and DiagnosticTag.Deprecated (I guess this got added to vscode in the mean time, I didn't dig through the history) to microsoft/language-server-protocol in e88d661#diff-6f4b0542b9cf28997cdd34763ec3b808d91ed64a4756a8ca9671a7769534b10dR10.
    • This is a pretty large diff without a lot of explanation. It feels like we just "lifted and shifted" DiagnosticTag out of vscode and into the LSP spec.
@jfly
Copy link
Author

jfly commented Sep 20, 2024

Nevermind! I wrote this up before I fully understood the situation.

I'm closing this in favor of #2026.

@jfly jfly closed this as completed Sep 20, 2024
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

1 participant