[LSP] Utilize list of all classification types + expose types to Razor#61395
[LSP] Utilize list of all classification types + expose types to Razor#61395allisonchou merged 6 commits intodotnet:mainfrom
Conversation
|
@sharwell I just saw your comment in the original issue about avoiding reflection, can you elaborate on why reflection is not preferred vs. source generators? |
dibarbet
left a comment
There was a problem hiding this comment.
can probably remove this comment here - https://sourceroslyn.io/#Microsoft.CodeAnalysis.Workspaces/Classification/ClassificationTypeNames.cs,16
Source generator is an interesting idea - I do like that. Reflection here feels icky, but I can't explain a good reason why, so I'll leave that to Sam
src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensHelpers.cs
Outdated
Show resolved
Hide resolved
ryanbrandenburg
left a comment
There was a problem hiding this comment.
Looks good AFAIK.
src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensHelpers.cs
Outdated
Show resolved
Hide resolved
|
Not a big fan of runtime reflection. It can cause a bunch of perf-related items to show up. Source generator would be able to automate the mapping without introducing any runtime overhead. |
|
Hey all, thanks for the comments so far. I updated the PR based on feedback + talking to @davidwengier who gave some good suggestions. Could I get another look at this PR? I changed the approach so we no longer use reflection or source generators. Instead, we add a new
I added a reflection test to ensure |
Very cool approach. I like! |
| { | ||
| internal class RazorSemanticTokensAccessor | ||
| { | ||
| public static ImmutableArray<string> RoslynTokenTypes => SemanticTokensHelpers.AllTokenTypes; |
There was a problem hiding this comment.
One bit we'll want to keep in mind is that once Razor consumes this & wants to insert into VS4Mac we'll need to ensure that the corresponding Roslyn bits are there first. Just bringing it up because there's a lot of movement in that space recently 😄
There was a problem hiding this comment.
Great point. I'm unfamiliar with VS4Mac insertions, is there a regular insertion schedule that I can keep up with to ensure the Roslyn bits go in before the Razor bits?
There was a problem hiding this comment.
No regular schedule, no. The build validation on VS Mac is good though, and will report a failure if there is an incompatibility between inserted versions of roslyn and razor.
| /// Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensHelpers.cs | ||
| /// </remarks> | ||
| public static ImmutableArray<string> AdditiveTypeNames { get; } = ImmutableArray.Create(StaticSymbol); | ||
| public static ImmutableArray<string> AdditiveTypeNames { get; } = ImmutableArray.Create(StaticSymbol, ReassignedVariable); |
There was a problem hiding this comment.
Curious: was ReassignedVariable a piece we had previously missed on the LSP side that we noticed / added here after the test caught the mismatch?
There was a problem hiding this comment.
The new version of RoslynCustomTokenTypes basically takes the full list of classifications but subtracts the ones already mapped to LSP token types and the ones in AdditiveTypeNames. Previously, since we hardcoded RoslynCustomTokenTypes we just explicitly left out StaticSymbol/ReassignedVariable/etc. from the list. However, now that we're doing things more programmatically, we have to intentionally specify which classifications to not include.
| [ClassificationTypeNames.StringLiteral] = LSP.SemanticTokenTypes.String, | ||
| }; | ||
|
|
||
| public static readonly ImmutableArray<string> RoslynCustomTokenTypes = ClassificationTypeNames.AllTypeNames |
There was a problem hiding this comment.
I probably missed the context, is there a specific reason why this is more preferable than the prior (where we re-specified everything)? Asking only because I could also see a world where the "verification test" also goes as far as to ensure this SemanticTokensHelpers class is specifying everything in ClassificationTypeNames
There was a problem hiding this comment.
Good question! IMO there's a couple of reasons why I think this is more preferable than prior:
AllTypeNamesspecifies every classification type and therefore can be easily re-used in non-LSP contexts. The previousRoslynCustomTokenTypesonly specified a subset of classification types.- Before, if we wanted to add a new mapping in
ClassificationTypeToSemanticTokenTypeMap, it would be a two-step process: we'd have to add the mapping, and then delete the entry inRoslynCustomTokenTypes. With the updated code, we can just add a new entry toClassificationTypeToSemanticTokenTypeMapand be done (besides needing to update the LSP client/Razor)
Essentially, I think it's just cleaner and allows for easier future maintainability 😄
AllTypeNamesarray to theClassificationTypeNamesclass. We make sure the array is kept up-to-date with the fields in the class via a test that utilizes reflection.Fixes #61237