Cohosting OnAutoInsert endpoint #10776

alexgav · 2024-08-22T02:35:23Z

Summary of the changes

Cohosting endpoint for auto-insert endpoint.

Fixes:
Part of #9519

Notes:

All auto-insert scenarios should be addressed now (Razor, HTML, C#)
Tests will be added in a separate PR since this one has grown large and long enough as it is.

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/AutoInsert/OnAutoInsertEndpoint.cs

...src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInsert/AutoClosingTagOnAutoInsertProvider.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInsert/AutoInsertService.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInsert/InsertTextEdit.cs

src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/AutoInsert/RemoteAutoInsertService.cs

src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/RemoteAdhocWorkspaceFactory.cs

...soft.VisualStudio.LanguageServices.Razor/LanguageClient/Cohost/CohostOnAutoInsertEndpoint.cs

DustinCampbell

I'm requesting a few changes:

Please don't add extension methods for converting between Roslyn LSP types and VS LSP types directly. It is intentional that the VsLspExtensions and RoslynLspExtensions methods never mix to make it easier to remove VsLspExtensions later in Use Roslyn LSP library instead of Visual Studio #10682. See my comments below for guidance on what to do in each case.
I'm uncomfortable with the change to IDocumentSnapshot and DocumentSnapshot. There is almost certainly a better abstraction then adding a method intended for formatting directly onto IDocumentSnapshot.
The FrozenSet<T> usage in this PR needs a second look. FrozenSet<T> is not a designed to be the new workhorse class for sets. In fact, it is much more expensive to construct a FrozenSet<T> because it performs up-front analysis to determine the best strategy. So, frozen sets should be reserved for use as static data. And, for arrays of 1 or 2 single character strings, a FrozenSet<string> is completely contrived. It's a trivial task to write something faster by hand to perform a Contains(...) test. In addition, there's a lot of code using the frozen sets in this PR that cause unnecessary allocations by boxed their struct enumerators. I tried to add guidance throughout my feedback, but I'm happy to chat about this further offline. Or, we could make it a topic at the next Razor "code talk about" meeting.

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInsert/AutoInsertService.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInsert/IOnAutoInsertProvider.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInsert/IAutoInsertService.cs

.../Microsoft.AspNetCore.Razor.LanguageServer/Hosting/VSInternalServerCapabilitiesExtensions.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentSnapshot.cs

....LanguageServices.Razor/LanguageClient/Cohost/CohostOnAutoInsertTriggerCharacterProviders.cs

.../Microsoft.AspNetCore.Razor.LanguageServer.Test/AutoInsert/OnAutoInsertEndpointTest.NetFx.cs

...r/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/AutoInsert/OnAutoInsertEndpointTest.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInsert/IOnAutoInsertProvider.cs

...crosoft.AspNetCore.Razor.LanguageServer.Test/AutoInsert/RazorOnAutoInsertProviderTestBase.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/AutoInsert/OnAutoInsertEndpoint.cs

DustinCampbell

I did a second review pass through the latest changes.

...src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInsert/AutoClosingTagOnAutoInsertProvider.cs

.../Microsoft.AspNetCore.Razor.LanguageServer/Hosting/VSInternalServerCapabilitiesExtensions.cs

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/AutoInsert/OnAutoInsertEndpoint.cs

...src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInsert/AutoClosingTagOnAutoInsertProvider.cs

...soft.AspNetCore.Razor.LanguageServer.Test/AutoInsert/CloseTextTagOnAutoInsertProviderTest.cs

...ft.AspNetCore.Razor.LanguageServer.Test/AutoInsert/AutoClosingTagOnAutoInsertProviderTest.cs

...r/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/AutoInsert/OnAutoInsertEndpointTest.cs

...crosoft.AspNetCore.Razor.LanguageServer.Test/AutoInsert/RazorOnAutoInsertProviderTestBase.cs

DustinCampbell · 2024-08-23T18:11:59Z

FYI that formatting tests are failing in CI for this PR. Not sure if you've taken a look at them yet.

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ImportDocumentSnapshot.cs

davidwengier

I don't like approving the PR with no tests for cohosting, but as there are tests for ensuring the existing bits don't break, I think it's okay to make an exception. Plus I'm waiting on this to merge so I can get my PR in and finish cohost formatting.

I expect I'll probably end up coming through and cleaning up a few things in this area anyway, since I won't be able to stop myself.

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInsert/AutoInsertService.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInsert/IAutoInsertService.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInsert/IOnAutoInsertProvider.cs

...zor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/FormattingCodeDocumentProvider.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Formatting/FormattingContext.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/DocumentSnapshot.cs

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/IDocumentSnapshot.cs

src/Razor/src/Microsoft.CodeAnalysis.Remote.Razor/AutoInsert/RemoteAutoInsertService.cs

davidwengier

I don't like approving the PR with no tests for cohosting, but as there are tests for ensuring the existing bits don't break, I think it's okay to make an exception. Plus I'm waiting on this to merge so I can get my PR in and finish cohost formatting.

I expect I'll probably end up coming through and cleaning up a few things in this area anyway, since I won't be able to stop myself.

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Mapping/RazorLanguageQueryEndpoint.cs

…ders

…rt/AutoClosingTagOnAutoInsertProvider.cs Co-authored-by: David Wengier <david.wengier@microsoft.com>

…rt/IAutoInsertService.cs Co-authored-by: David Wengier <david.wengier@microsoft.com>

…rt/InsertTextEdit.cs Co-authored-by: David Wengier <david.wengier@microsoft.com>

… CR suggestion

…Server

…tion

…Service

Creating IFormattingCodeDocumentProvider service with seprate LSP and Remote implementations to provide code document appropriate for formatting.

Extension methods can't be used for Mock setups, so since I made GetGeneratedOutputAsync() with no parameter an extension method, I had to switch unit tests to use GeGeneratedOutputAsync(It.Any<bool>())

We still had non-parameter GetGeneratedOutputAsycnc in DocumentSnapshot (even though it wasn't in IDocumentSnapshot) which was getting called internally. That was both messy (since there is now no-parameter extension method on IDocumentSnapshot) and was causing issues in tests

alexgav requested a review from a team as a code owner August 22, 2024 02:35

davidwengier reviewed Aug 22, 2024

View reviewed changes

DustinCampbell requested changes Aug 22, 2024

View reviewed changes

davidwengier reviewed Aug 23, 2024

View reviewed changes

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

...crosoft.AspNetCore.Razor.LanguageServer.Test/AutoInsert/RazorOnAutoInsertProviderTestBase.cs Show resolved Hide resolved

davidwengier reviewed Aug 23, 2024

View reviewed changes

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/AutoInsert/OnAutoInsertEndpoint.cs Outdated Show resolved Hide resolved

DustinCampbell reviewed Aug 23, 2024

View reviewed changes

DustinCampbell approved these changes Aug 27, 2024

View reviewed changes

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

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/ProjectSystem/ImportDocumentSnapshot.cs Show resolved Hide resolved

davidwengier approved these changes Aug 28, 2024

View reviewed changes

DustinCampbell reviewed Aug 28, 2024

View reviewed changes

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Mapping/RazorLanguageQueryEndpoint.cs Show resolved Hide resolved

alexgav and others added 19 commits August 29, 2024 09:49

Factoring out AutoInsertService

b9abac6

Switch non-cohost endpoint to use new AutoInsertService

330a670

Adding Remote and OOB AutoInsertService classes and OnAutoInsertProvi…

fe520b8

…ders

Add common code for capabilities

c5506e6

Add cohost OnAutoInsert endpoint

3f74c03

Parameter rename

85cf1f1

Update src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInse…

be0e85f

…rt/AutoClosingTagOnAutoInsertProvider.cs Co-authored-by: David Wengier <david.wengier@microsoft.com>

Update src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInse…

3ee7479

…rt/IAutoInsertService.cs Co-authored-by: David Wengier <david.wengier@microsoft.com>

Update src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/AutoInse…

5cb62c3

…rt/InsertTextEdit.cs Co-authored-by: David Wengier <david.wengier@microsoft.com>

Fixup after rebase

38c9436

More post-build cleanup

4a6d7f6

Move common HTML and C# auto-insert trigger chars per CR suggestion

09452b5

More cleanup after rebase

447dd54

Add IOnAutoInsertTiggerCharacterProvider per CR suggestion

70430a4

Removing trigger characters property from RemoteAutoInsertService per…

02db4e4

… CR suggestion

Switch to using RemoteResponse

b3ec796

Fixup bad resolve after rebase, and extra whitespace in RazorLangauge…

2813a97

…Server

Complete capabilities check in CohostingOnAutoInsertEndpoing registra…

f19fd7f

…tion

Change input position type to serializable :LinePosition

1dbc2c2

alexgav added 19 commits August 29, 2024 09:49

Renaming interface methods, removing unnecessary usage of FrozenSet

7d61fbf

Use ImmutableArray to store providers

a7df9ef

Switching to bool and out on IAutoInsertService and implementation

34beda3

Cleaned up document position info calculation per PR feedback

a4beaef

Minor cleanup in RemoteAutoInsertService

4613364

Cleanup trigger character calculation in CohostOnAutoInsertEndpoint

bf389fb

Formatting changes, VsLspFactory usage

c9e5130

Revert to MEF DI in RemoteAdhocWorkspaceFactory/RemoteRazorFormatting…

9d339ef

…Service

Removing Rolsyn to VS LSP extension methods

a629cf3

Switch to raw strings in tests and minor whitespace cleanup

46c9356

Rename per PR feedback suggestion to better indicate the purpose

0eab010

Cleanup formatting code document acquisition per PR feedback

7bb3d32

Creating IFormattingCodeDocumentProvider service with seprate LSP and Remote implementations to provide code document appropriate for formatting.

Removing redundant assert as compiler is already doing the check

e53bf51

Unnecessary assignment cleanup

348f76f

Misc PR feedback cleanup

c984b46

Made GetGeneratedOutputAsync an extension method per PR feedback

3f9fe84

Remaining MEF parameter attrivute formatting changes

69105c1

Last of the PR feedback.

137d9ad

Fixing most unit test failures.

7715a4c

Extension methods can't be used for Mock setups, so since I made GetGeneratedOutputAsync() with no parameter an extension method, I had to switch unit tests to use GeGeneratedOutputAsync(It.Any<bool>())

alexgav force-pushed the dev/alexgav/CohostingOnAutoInsertEndpoint branch from 9719aad to 7715a4c Compare August 29, 2024 16:50

alexgav merged commit 12f5194 into main Aug 29, 2024
12 checks passed

alexgav deleted the dev/alexgav/CohostingOnAutoInsertEndpoint branch August 29, 2024 18:27

dotnet-policy-service bot added this to the Next milestone Aug 29, 2024

This was referenced Sep 4, 2024

[Automated] PRs inserted in VS build main-35303.128 #10830

Closed

[Automated] PRs inserted in VS build feature.debugger.main-35305.238 #10849

Closed

dotnet-bot mentioned this pull request Sep 14, 2024

[Automated] PRs inserted in VS build feature.debugger.shadowDebug-35313.170 #10886

Closed

ryzngard mentioned this pull request Oct 1, 2024

Merge main into features/extract-to-component #10948

Merged

phil-allen-msft modified the milestones: Next, 17.12 P3 Oct 31, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cohosting OnAutoInsert endpoint #10776

Cohosting OnAutoInsert endpoint #10776

alexgav commented Aug 22, 2024

DustinCampbell left a comment

DustinCampbell left a comment

DustinCampbell commented Aug 23, 2024

davidwengier left a comment

davidwengier left a comment

Cohosting OnAutoInsert endpoint #10776

Cohosting OnAutoInsert endpoint #10776

Conversation

alexgav commented Aug 22, 2024

Summary of the changes

DustinCampbell left a comment

Choose a reason for hiding this comment

DustinCampbell left a comment

Choose a reason for hiding this comment

DustinCampbell commented Aug 23, 2024

davidwengier left a comment

Choose a reason for hiding this comment

davidwengier left a comment

Choose a reason for hiding this comment