[LSP] Semantic tokens: Utilize frozen partial compilations#55976
[LSP] Semantic tokens: Utilize frozen partial compilations#55976allisonchou merged 11 commits intodotnet:mainfrom
Conversation
dibarbet
left a comment
There was a problem hiding this comment.
So after seeing #55910 we will definitely need that change since LSP solutions will always be branched solutions.
Would also request a test to verify that we set the flag properly when we only have partial, and that it is not set when the full compilation is available (can see the above PR, I think there are tests you might be able to reference there).
src/Features/LanguageServer/Protocol/Handler/SemanticTokens/RoslynSemanticTokens.cs
Outdated
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensHelpers.cs
Outdated
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Handler/SemanticTokens/RoslynSemanticTokens.cs
Outdated
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Handler/SemanticTokens/RoslynSemanticTokensDelta.cs
Outdated
Show resolved
Hide resolved
src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensEditsHandler.cs
Outdated
Show resolved
Hide resolved
| await VerifyNoMultiLineTokens(testLspServer, rangeResults.Data!).ConfigureAwait(false); | ||
| Assert.Equal(expectedRangeResults.Data, rangeResults.Data); | ||
| Assert.Equal(expectedRangeResults.ResultId, rangeResults.ResultId); | ||
| Assert.True(rangeResults is RoslynSemanticTokens); |
There was a problem hiding this comment.
Can we get a test/check for the IsPartial == true scenario? Dono how hard it is to simulate a FrozenSemanticDocument but seems useful.
|
#55910 has been merged, which should clear this PR. I spent quite a bit of time today trying to figure out how feasible adding a test for partial semantics would be + checked with Jason, and in summary it seems more difficult than expected. 😕 It appears the test infrastructure by default gets the full compilation and trying to figure out how to get a weak compilation in the LSP layer will probably involve some time. The test Jason added is in a different layer than LSP and the constructs it uses aren't available in the LSP layer currently. Considering Cyrus' PR to add frozen semantics to non-LSP scenarios didn't include tests either, I'm wondering if we can table this for the future (potentially P5) so we can include this in time for P4. Thoughts @ryanbrandenburg @dibarbet? I can file an issue to add tests if we decide to go this route. I tested manually and confirmed things work, although I can def see how not having tests is risky. |
src/Features/LanguageServer/Protocol/Handler/SemanticTokens/SemanticTokensEditsHandler.cs
Outdated
Show resolved
Hide resolved
|
Merging. Filed #56038 to add a test in the future. |
Allows Roslyn's LSP semantic token handlers to utilize frozen compilations to compute tokens. The switch to using frozen compilations is fairly straightforward, however Razor needs to know if we're returning partial tokens or not so they know whether to keep asking us for tokens. To accomplish this, we return a
IsFinalizedproperty along with our results that Razor can deserialize.Also modified the handlers' logic to not cache 0-length tokens as it is unnecessary.
Likely should be dual-inserted with the Razor-side PR: dotnet/razor#4156