Intercept C# semantic tokens refresh messages by allisonchou · Pull Request #6298 · dotnet/razor

allisonchou · 2022-04-19T19:42:01Z

Summary of the changes

Adds code to support intercepting C# semantic tokens refresh messages.
Can't be fully tested yet due to a bug on the VS client side where refresh messages aren't being sent through the middle layer properly.
Depends on [LSP] Implement support for workspace/semanticTokens/refresh notifications roslyn#60824 before merging.
Planning on adding some tests before merging - just wanted to make sure this was the correct general approach.

...azor/src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/SemanticTokensRefreshEndpoint.cs

ryanbrandenburg

Mostly just nits, though the _ = Task stuff concerns me. If there's a specific reason we need it lets go ahead and add an explanation comment about why.

...rc/Microsoft.VisualStudio.LanguageServerClient.Razor/RazorCSharpSemanticTokensInterceptor.cs

eng/Versions.props

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/RazorSemanticTokensEndpoint.cs

...t.AspNetCore.Razor.LanguageServer/Semantic/Services/DefaultRazorSemanticTokensInfoService.cs

allisonchou · 2022-04-26T06:34:36Z

...Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Formatting/CSharpOnTypeFormattingPass.cs

                var documentOptions = await GetDocumentOptionsAsync(context).ConfigureAwait(false);

                // Ask C# for formatting changes.
+                var indentationOptions = new RazorIndentationOptions(


@davidwengier would you mind confirming that this change looks correct here? I had to upgrade the Roslyn package version which resulted in one of the overloads being rendered obsolete. I was going to leave it for later, but then the formatting tests started failing so I just attempted to fix it up in this PR😅

I think if the tests pass then its fine, though @tmat used a different approach for the auto formatting options in https://github.com/dotnet/razor-tooling/pull/6166/files#diff-1fcf96ce7b4faaa3af72db165764d43a107a153f7040dc46169bd4d562f1b173 and got them from the global options service.

I'm not sure how much it matters for razor scenarios, as there isn't a way for anyone to configure the options anyway.

Thanks! Didn't know Tomas opened a PR. I tried out his changes but was getting some weird casting errors when running the formatting tests. I'm going to take the current approach for now so we can merge this PR, but maybe we can address in a follow up

👍 His PR might depend on un-merged Razor EA changes in Roslyn.

NTaylorMullen

Have one big race, but we're getting so close!!!

...azor/src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/SemanticTokensRefreshEndpoint.cs

...t.AspNetCore.Razor.LanguageServer/Semantic/Services/DefaultRazorSemanticTokensInfoService.cs

...osoft.AspNetCore.Razor.LanguageServer/Semantic/Services/DefaultSemanticTokensCacheService.cs

...rc/Microsoft.VisualStudio.LanguageServerClient.Razor/RazorCSharpSemanticTokensInterceptor.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/RazorSemanticTokensEndpoint.cs

...t.AspNetCore.Razor.LanguageServer/Semantic/Services/DefaultRazorSemanticTokensInfoService.cs

...osoft.AspNetCore.Razor.LanguageServer/Semantic/Services/DefaultSemanticTokensCacheService.cs

...rc/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/Services/SemanticTokensCacheService.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/MemoryCache.cs

...rc/Microsoft.VisualStudio.LanguageServerClient.Razor/RazorCSharpSemanticTokensInterceptor.cs

...azor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Semantic/SemanticTokensCacheTest.cs

NTaylorMullen

!

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/MemoryCache.cs

ghost · 2022-04-26T23:46:03Z

Hello @allisonchou!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

allisonchou · 2022-04-27T00:30:49Z

@ryanbrandenburg it looks like integration tests are hanging, any ideas? Could this be related to the Roslyn package upgrade?
edit: found the logs, investigating

NTaylorMullen · 2022-04-27T04:54:53Z

@ryanbrandenburg it looks like integration tests are hanging, any ideas? Could this be related to the Roslyn package upgrade? edit: found the logs, investigating

Try rebasing on main to get this fix: #6322

…anticTokens

ryanbrandenburg

It was a good cache. Perhaps some day it will be needed again.

ryanbrandenburg · 2022-04-27T17:46:55Z

any ideas

Almost certainly a transient issue, retry with the rebase that @NTaylorMullen recommended. I filed #6324 to follow up. Thankfully we were blessed with LogHubs on this issue, so investigation should be relatively simple.

NTaylorMullen · 2022-04-27T17:47:50Z

any ideas

Almost certainly a transient issue, retry with the rebase that @NTaylorMullen recommended. I filed #6324 to follow up. Thankfully we were blessed with LogHubs on this issue, so investigation should be relatively simple.

Also, do we have dump capturing by chance? That'd make it super clear to understand what was hanging

allisonchou added 3 commits April 19, 2022 12:28

Initial changes

8533a5e

Cleanup

f735f22

Cleanup

3bec4d9

allisonchou requested review from a team, NTaylorMullen, davidwengier, ryanbrandenburg and sharwell as code owners April 19, 2022 19:42

NTaylorMullen reviewed Apr 19, 2022

View reviewed changes

...azor/src/Microsoft.AspNetCore.Razor.LanguageServer/Semantic/SemanticTokensRefreshEndpoint.cs Outdated Show resolved Hide resolved

ryanbrandenburg approved these changes Apr 19, 2022

View reviewed changes

Review feedback

f321de1

This was referenced Apr 19, 2022

[LSP] Implement support for workspace/semanticTokens/refresh notifications dotnet/roslyn#60824

Merged

C# formatting method GetFormattingChanges is obsolete #6313

Closed

allisonchou added 3 commits April 22, 2022 21:40

Upgrade Roslyn package version

e9697b7

Add external init

c1bd7eb

Resolve merge conflcit

d61ffd6

ryanbrandenburg approved these changes Apr 25, 2022

View reviewed changes

allisonchou mentioned this pull request Apr 25, 2022

RazorBrokeredServiceImplementation.GetSolutionAsync is obsolete #6316

Closed

allisonchou added 9 commits April 25, 2022 14:51

Update package versions

c250072

Review feedback

b2e2b12

Review feedback

4b2b21a

Review feedback

5ff36dc

Add tests + fixup tests

40ddc68

Resolve merge conflicts

36424a5

Resolve build errors

47dfaa1

Resolve build errors

70f43d8

Update comemnt

351be3f

allisonchou commented Apr 26, 2022

View reviewed changes

NTaylorMullen suggested changes Apr 26, 2022

View reviewed changes

ryanbrandenburg approved these changes Apr 26, 2022

View reviewed changes

allisonchou added 3 commits April 26, 2022 12:57

Partial review feedback

3f174c5

Farewell cache

1231e45

Additional cleanup

d4319fb

NTaylorMullen approved these changes Apr 26, 2022

View reviewed changes

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/MemoryCache.cs Outdated Show resolved Hide resolved

allisonchou added 2 commits April 26, 2022 16:38

Naming

f5283e2

Cleanup

65a1afa

allisonchou added the auto-merge Squash merge once all PR checks are complete and reviewers have approved label Apr 26, 2022

Merge remote-tracking branch 'origin/main' into allichou/InterceptSem…

f739b6b

…anticTokens

ryanbrandenburg approved these changes Apr 27, 2022

View reviewed changes

allisonchou mentioned this pull request Apr 27, 2022

Disable integration tests #6325

Merged

allisonchou added 3 commits April 27, 2022 11:57

Disable integration tests

b596a06

Update azure-pipelines.yml

703b80d

Update azure-pipelines.yml

73b5850

allisonchou merged commit 751db1e into main Apr 27, 2022

allisonchou deleted the allichou/InterceptSemanticTokens branch April 27, 2022 19:32

allisonchou mentioned this pull request Apr 29, 2022

Remove leftover semantic tokens isFinalized references #6340

Merged

Conversation

allisonchou commented Apr 19, 2022

Summary of the changes

Uh oh!

Uh oh!

ryanbrandenburg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

allisonchou Apr 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwengier Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

allisonchou Apr 26, 2022

Choose a reason for hiding this comment

Uh oh!

davidwengier Apr 27, 2022

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NTaylorMullen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Apr 26, 2022

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

Uh oh!

allisonchou commented Apr 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NTaylorMullen commented Apr 27, 2022

Uh oh!

ryanbrandenburg left a comment

Choose a reason for hiding this comment

Uh oh!

ryanbrandenburg commented Apr 27, 2022

Uh oh!

NTaylorMullen commented Apr 27, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

allisonchou Apr 26, 2022 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

allisonchou commented Apr 27, 2022 •

edited

Loading