Add ability to underline variables in the editor that are reassigned.

[CyrusNajmabadi]

Some users would like to know if a variable is ever reassigned in some block of code. This is esp. relevant to them as C# lacks a way to mark local variables as 'readonly', so a manual scan of the code (or examination of FindAllReferences) is necessary.

This PR adds the ability to mark these variables (declaration and references) with an underline to clearly demarcate which are even reassignments.

A core intuition here is that this effectively acts like a classification. You can think of non-underlying variables as being 'effectively readonly', while underlined variables are 'effectively mutable'. If we had a lang-feature to make locals readonly, these are the ones that wouldn't be applicable.

For example, without reassignment:

But with reassignment:

Closes #37517

Todo:

• VB side.
• Exposed Tools|Option to control this
• Tests

[CyrusNajmabadi]

this will likely be set to false when this is submitted. it will be an opt-in option.

[mavasani]

So, are we underlining all reads and writes in the block if there is at least one write? Can you expand a bit more on the overall user scenario?

[davidwengier]

This will be interesting to play with, as my gut feeling on reading the PR title was that this would underline the reassignment, and perhaps the reads after that, and I was surprised by the parameter itself being underlined. I can see the logic in that approach too though.

[mavasani]

my gut feeling on reading the PR title was that this would underline the reassignment, and perhaps the reads after that, and I was surprised by the parameter itself being underlined

Exactly, I was surprised as well. My gut feeling on reading the PR was that we have some way to show the def-use chains so a user can look at a read and find all potential writes that can lead to it or vice versa. I would like to understand the motivation for underlying all reads and writes in the block.

[CyrusNajmabadi]

So, are we underlining all reads and writes in the block if there is at least one write? Can you expand a bit more on the overall user scenario?

No. We are underling any variable that ever encounters a reassignment. That way the user can tell if this is an 'assigned-once' varaible, or a reassigned variable.

[CyrusNajmabadi]

This will be interesting to play with, as my gut feeling on reading the PR title was that this would underline the reassignment, and perhaps the reads after that, and I was surprised by the parameter itself being underlined. I can see the logic in that approach too though.

If we only underline hte reassignment, it's easy to miss. The idea here is that you can immediately distinguish (from any location) if it's reassigned or not ever.

It would be akin to knowing at the decl site, if this is read-only or not.

[CyrusNajmabadi]

I would like to understand the motivation for underlying all reads and writes in the block.

Think of it more like a classification. Non-underlined variables are effectively readonly (and could be marked as such if we ever get such a feature). Underlined variables are reassigned and thus can't be readonly.

[CyrusNajmabadi]

@genlu @sharwell This has been updated with the resolution of the IDE design meeting. Specifically, internally, this now runs through the classification codepath. Though we still need the special IDE option to enable this (as standard VS classification cannot underline things).

[sharwell]

@CyrusNajmabadi can you describe the rules for which variables get underlined, and at which locations? We discussed many options in design review and I'm curious which one we ended up with for the PR.

Specifically, internally, this now runs through the classification codepath.

👍

[CyrusNajmabadi]

@CyrusNajmabadi can you describe the rules for which variables get underlined, and at which locations?

Sure!

The general idea is: is this a variable that is assigned after it has already been assigned. The idea being that if we had readonly variables, would this be something the compiler would be likely to error on becuse multiple writes happen to the variable.

Once we detect this has happened for a variable, we then consider this an aspect of the variable itself (similar to how a variable is static is not). One we determine this is an aspect of the variable, then we classify that variable (the declaration and references for it) in that fashion.

[CyrusNajmabadi]

@ryzngard it looks like:

[dibarbet]

there's a 'modification' SemanticTokenModifier in the LSP spec, would that be appropriate?
https://microsoft.github.io/language-server-protocol/specification#textDocument_semanticTokens

Also, @allisonchou do we need custom handling for this in the LSP client?

[CyrusNajmabadi]

i did add some filtering for this in our LSP code (similar to how we handle 'static') :)