"Fix" LSP semantic token modifiers #68744
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
davidwengier
changed the title
|
Also, noticed in the code that converts classifications to semantic tokens that Roslyn will only ever apply one token modifier to a span. Is that intentional? Or a side-effect of the fact that the current two are mutually exclusive? @CyrusNajmabadi do you remember? |
There was a problem hiding this comment.
Also, noticed in the code that converts classifications to semantic tokens that Roslyn will only ever apply one token modifier to a span. Is that intentional? Or a side-effect of the fact that the current two are mutually exclusive?
I'm guessing we just never implemented it because we only returned one at the time (think this predates the existence of the reassigned variable modifier).
The spec supports multiple, we'd just have to do the work to do the proper encoding for it.
| { | ||
| // This must be in the same order as SemanticTokens.TokenModifiers, but skip the "None" item | ||
| SemanticTokenModifiers.Static, | ||
| nameof(SemanticTokens.TokenModifiers.ReassignedVariable) |
There was a problem hiding this comment.
why nameof for reassignedvariable but not the other?
There was a problem hiding this comment.
Static is coming from the LSP protocol DLL, as its part of the standard set. I could change it to nameof(SemanticTokens.TokenModifiers.Static) for consistency, but ReassignedVariable can't come from anywhere else, as its entirely a Roslyn construct. Between Roslyn, Razor and VS, I don't think anyone actually implements this part of the spec properly (or nobody knows how it should be implemented - myself included!)
ghost
added this to the 
For purley selfish reasons, I'm trying to add support to VS for semantic token modifiers, and noticed that Roslyns legend was a bit wrong (you weren't returning the "Reassigned Variable" modifier in the legend, but are returning it in the semantic token range LSP request). Additionally, Razors legend is wrong too, so exposing the fixed one so we can reuse it fixes that too.
Sneak peak!
