[LSP] Add Razor options provider to Roslyn #53879

allisonchou · 2021-06-04T08:46:22Z

Roslyn isn't respecting Razor's tabs/spaces settings when generating override and partial method completion TextEdits in the resolve handler.
This PR adds a document options provider that allows Razor to communicate its options to Roslyn.
The Razor side of this also needs to be merged for this bug to be resolved: Respond to Roslyn adding Razor options service razor#3719

Fixes (once Razor side is also merged):
https://github.com/dotnet/aspnetcore/issues/32555

…ervice

src/Features/Core/Portable/Completion/CompletionProvider.cs

src/Features/LanguageServer/Protocol/Handler/Completion/CompletionResolveHandler.cs

dibarbet

Have one main piece of feedback before I look at the rest

src/Features/LanguageServer/Protocol/Handler/Completion/CompletionResolveHandler.cs

allisonchou · 2021-06-05T01:28:42Z

src/Features/Core/Portable/Completion/CompletionProvider.cs

-            return Task.FromResult(CompletionChange.Create(new TextChange(item.Span, item.DisplayText)));
-        }
+        public virtual Task<CompletionChange> GetChangeAsync(Document document, CompletionItem item, char? commitKey, CancellationToken cancellationToken)
+            => Task.FromResult(CompletionChange.Create(new TextChange(item.Span, item.DisplayText)));


I just changed this to expression body to be more consistent with the style of the other methods in this class

allisonchou · 2021-06-05T01:35:11Z

I totally overhauled this PR based on your super helpful feedback @davidwengier and @dibarbet. The check for Razor options is now performed in the formatter, and the specific tabs/spaces options logic is now done on the Razor side (via dotnet/razor#3719). If either of you could take another look whenever you have time, that would be appreciated. Thanks 😀

davidwengier

So much cleaner 😁

dibarbet

🎉 totally agree with David, I much prefer this iteration

NTaylorMullen · 2021-06-07T18:43:42Z

src/Workspaces/Core/Portable/Formatting/Formatter.cs

+            var documentOptionSetProvider = document.Services.GetService<IDocumentOptionSetProvider>();
+            if (documentOptionSetProvider is not null)
+            {
+                options = await documentOptionSetProvider.GetOptionsForDocumentAsync(document, cancellationToken).ConfigureAwait(false);


Hmmm, one interesting implication is that if you pass in options to this method they'll be ignored because the document options will win. With that in mind the options on line 157 account for this; in an ideal world all call sites to this method wouldn't actually pass options in "within" Roslyn and would then fall back to the appropriate document options services. All that can be done in the future though, just my 2cents

Yeah this seems really weird here. Couldn't the LSP layer or something deal with coupling this at that layer versus something else?

Currently Razor is the only implementer of IDocumentOptionSetProvider so in practice I don't think there's currently an issue, however I totally see both of your points about the check being weird here. I'm exploring alternate options, will hold off on merging in the meantime.

jasonmalinowski

This approach feels really funky and it feels like we still don't have this wired up at the right layer....?

jasonmalinowski · 2021-06-07T19:14:01Z

src/Workspaces/Core/Portable/Formatting/Formatter.cs

+            // Languages such as Razor can specify their own options instead of using the document's options.
+            var documentOptionSetProvider = document.Services.GetService<IDocumentOptionSetProvider>();
+            if (documentOptionSetProvider is not null)


So why is this having to be checked here rather than in the core document options logic we have, like through https://github.com/dotnet/roslyn/blob/main/src/Workspaces/Core/Portable/Options/IDocumentOptionsProvider.cs?

allisonchou · 2021-06-08T06:38:36Z

@jasonmalinowski @NTaylorMullen Based on your feedback, I refactored out the Razor options logic from the formatter over to the indentation service. This required once again overhauling a large portion of the PR, namely due to having to change the return type of the added Razor external access method. If either of you (or anyone else reading this) have any concerns with the updated approach, please let me know!

src/Tools/ExternalAccess/Razor/RazorDocumentOptionsServiceWrapper.cs

src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs

jasonmalinowski · 2021-06-08T21:56:45Z

...Features/Core/Implementation/Formatting/InferredIndentationDocumentOptionsProviderFactory.cs

-                => Task.FromResult<IDocumentOptions?>(new DocumentOptions(document.Project.Solution.Workspace, document.Id, _indentationManagerService));
+            public async Task<IDocumentOptions?> GetOptionsForDocumentAsync(Document document, CancellationToken cancellationToken)
+            {
+                // Languages such as Razor can specify their own formatting options.


So it's a bit odd that we are having to modify the InferredIndentationDocumentOptionsProviderFactory here when we could have a new MEF export. The Razor ExternalAccess could directly export it's own IDocumentOptionsProviderFactory that imports the Razor service directly. This would mean you can keep almost the entire change in the Razor layer and not have to introduce the extra layer in the middle: just rename IDocumentOptionsService to IRazorDocumentOptionsService and you can get away with bit less wrapping.

(it's also otherwise odd we'd be having this at an EditorFeatures layer, since what happens for VS Code?)

allisonchou · 2021-06-09T03:06:33Z

src/Features/Core/Portable/Shared/Extensions/DocumentExtensions.cs

@@ -179,5 +179,13 @@ public static ImmutableArray<AbstractFormattingRule> GetFormattingRules(this Doc

            return rules.AddRange(Formatter.GetDefaultFormattingRules(document));
        }
+
+        public static bool IsRazorDocument(this Document document)


Method was moved

You moved it here for ExternalAccess.Razor right? If you all are "ok" with this living here then I don't have an issue but I also wouldn't be opposed to duplicating this in the ExternalAccess.Razor layer

I think I would prefer leaving in external access if possible - while currently 'true' the presence of a span mapping service means it's razor, there's nothing saying that spans can't be mapped in other cases. So I think I'd prefer to not let it leak too far.

Also, not sure if checking for this might be more appropriate? - https://sourceroslyn.io/#Microsoft.CodeAnalysis.ExternalAccess.Razor/RazorDocumentPropertiesServiceWrapper.cs,23 because span mappers are used in legacy razor as well and I'm guessing we don't want to override options there

Created a new ExternalAccess class for Razor extensions - https://github.com/dotnet/roslyn/blob/b9eb126fc9c7539f6e7cd3ee31dd1da318ac2f1c/src/Tools/ExternalAccess/Razor/Extensions.cs

Hope this is a little cleaner!

allisonchou · 2021-06-09T03:18:43Z

@jasonmalinowski I moved to a MEF-based approach in response to your feedback. One thing I was unsure about was whether there's a way to exclusively activate the Razor options factory in Razor scenarios, as right now it activates with every type of project/solution. To mitigate this, I added a IsRazorDocument check to the Razor options provider, but I'm not sure if there's a better approach.

src/VisualStudio/Core/Def/Implementation/LanguageClient/VisualStudioInProcLanguageServer.cs

dibarbet · 2021-06-09T18:58:51Z

src/Tools/ExternalAccess/Razor/RazorDocumentOptionsProviderFactory.cs

+namespace Microsoft.CodeAnalysis.ExternalAccess.Razor
+{
+    [Shared]
+    [ExportMetadata("Extensions", new string[] { "cshtml", "razor", })]


What does this bit do exactly? Only trigger when there is a cshtml or razor file in the project? And you still need the razor check down below since generated documetns don't fit here, is that right?

Great point, I checked with Taylor just now and it seems the line is actually unnecessary so I removed it - seems like it's required for Razor's dynamic file info provider but not here, it just tells Roslyn to ask Razor for dynamic file info when a .cshtml or .razor file is added.

dibarbet · 2021-06-09T19:07:38Z

src/Features/Core/Portable/Shared/Extensions/DocumentExtensions.cs

@@ -179,5 +179,13 @@ public static ImmutableArray<AbstractFormattingRule> GetFormattingRules(this Doc

            return rules.AddRange(Formatter.GetDefaultFormattingRules(document));
        }
+
+        public static bool IsRazorDocument(this Document document)


I think I would prefer leaving in external access if possible - while currently 'true' the presence of a span mapping service means it's razor, there's nothing saying that spans can't be mapped in other cases. So I think I'd prefer to not let it leak too far.

Also, not sure if checking for this might be more appropriate? - https://sourceroslyn.io/#Microsoft.CodeAnalysis.ExternalAccess.Razor/RazorDocumentPropertiesServiceWrapper.cs,23 because span mappers are used in legacy razor as well and I'm guessing we don't want to override options there

ryanbrandenburg

One nit-pick, otherwise looks OK, though I lack context.

src/Tools/ExternalAccess/Razor/IRazorDocumentOptionsService.cs

allisonchou · 2021-06-09T20:46:30Z

Thanks everyone for the reviews! I think I've gotten to all the feedback, if there's anything else please let me know. If everything looks good, I plan to merge this by EOD so we can start getting the Razor side PR going as well.

allisonchou added 6 commits June 3, 2021 01:27

Preliminary changes

279d026

Merge branch 'main' of github.com:dotnet/roslyn into AddRazorOptionsS…

f29ba3c

…ervice

Completion provider changes

f24711f

More edits

b500d28

Cleanup

7896ed2

Cleanup

881510a

allisonchou added the Area-IDE label Jun 4, 2021

allisonchou requested review from a team as code owners June 4, 2021 08:46

allisonchou added the LSP issues related to the roslyn language server protocol implementation label Jun 4, 2021

Youssef1313 reviewed Jun 4, 2021

View reviewed changes

src/Features/Core/Portable/Completion/CompletionProvider.cs Outdated Show resolved Hide resolved

allisonchou mentioned this pull request Jun 4, 2021

Respond to Roslyn adding Razor options service dotnet/razor#3719

Closed

allisonchou requested review from NTaylorMullen, dibarbet, genlu and davidwengier June 4, 2021 09:06

davidwengier reviewed Jun 4, 2021

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/Completion/CompletionResolveHandler.cs Outdated Show resolved Hide resolved

dibarbet reviewed Jun 4, 2021

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/Completion/CompletionResolveHandler.cs Outdated Show resolved Hide resolved

allisonchou added 2 commits June 4, 2021 18:19

Code review feedback

856a282

Cleanup

ce31655

allisonchou commented Jun 5, 2021

View reviewed changes

allisonchou requested review from dibarbet and davidwengier June 5, 2021 01:36

davidwengier approved these changes Jun 7, 2021

View reviewed changes

dibarbet approved these changes Jun 7, 2021

View reviewed changes

NTaylorMullen approved these changes Jun 7, 2021

View reviewed changes

Fix up tests

6f744d4

jasonmalinowski reviewed Jun 7, 2021

View reviewed changes

Move options logic from formatter to options service

b8c8446

remove unnecessary using

179c811

NTaylorMullen approved these changes Jun 8, 2021

View reviewed changes

src/Tools/ExternalAccess/Razor/RazorDocumentOptionsServiceWrapper.cs Outdated Show resolved Hide resolved

Simplify based on code review feedback, update tests

04fee30

jasonmalinowski reviewed Jun 8, 2021

View reviewed changes

allisonchou added 2 commits June 8, 2021 20:05

Switch to export

c7b7ad4

Add file

df5f2e4

allisonchou commented Jun 9, 2021

View reviewed changes

allisonchou requested a review from jasonmalinowski June 9, 2021 03:18

dibarbet reviewed Jun 9, 2021

View reviewed changes

ryanbrandenburg approved these changes Jun 9, 2021

View reviewed changes

src/Tools/ExternalAccess/Razor/IRazorDocumentOptionsService.cs Outdated Show resolved Hide resolved

allisonchou added 2 commits June 9, 2021 13:39

Code review feedback

b9eb126

Remove added using statement

a87c6aa

dibarbet approved these changes Jun 9, 2021

View reviewed changes

allisonchou merged commit 0e94007 into dotnet:main Jun 9, 2021

ghost added this to the Next milestone Jun 9, 2021

allisonchou deleted the AddRazorOptionsService branch June 9, 2021 22:51

This was referenced Jun 11, 2021

Add Razor options service in response to Roslyn changes dotnet/razor#3768

Merged

Remove IRazorDocumentOptionsService inheritance interface #54047

Merged

RikkiGibson modified the milestones: Next, 17.0.P2 Jun 29, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[LSP] Add Razor options provider to Roslyn #53879

[LSP] Add Razor options provider to Roslyn #53879

allisonchou commented Jun 4, 2021 •

edited

Loading

dibarbet left a comment

allisonchou Jun 5, 2021

allisonchou commented Jun 5, 2021

davidwengier left a comment

dibarbet left a comment

NTaylorMullen Jun 7, 2021

jasonmalinowski Jun 7, 2021

allisonchou Jun 7, 2021 •

edited

Loading

jasonmalinowski left a comment

jasonmalinowski Jun 7, 2021

allisonchou commented Jun 8, 2021 •

edited

Loading

jasonmalinowski Jun 8, 2021

jasonmalinowski Jun 8, 2021

allisonchou Jun 9, 2021

NTaylorMullen Jun 9, 2021

dibarbet Jun 9, 2021

allisonchou Jun 9, 2021

allisonchou commented Jun 9, 2021

dibarbet Jun 9, 2021 •

edited

Loading

allisonchou Jun 9, 2021

dibarbet Jun 9, 2021

ryanbrandenburg left a comment

allisonchou commented Jun 9, 2021

[LSP] Add Razor options provider to Roslyn #53879

[LSP] Add Razor options provider to Roslyn #53879

Conversation

allisonchou commented Jun 4, 2021 • edited Loading

dibarbet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonchou commented Jun 5, 2021

davidwengier left a comment

Choose a reason for hiding this comment

dibarbet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonchou Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

jasonmalinowski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonchou commented Jun 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonchou commented Jun 9, 2021

dibarbet Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanbrandenburg left a comment

Choose a reason for hiding this comment

allisonchou commented Jun 9, 2021

allisonchou commented Jun 4, 2021 •

edited

Loading

allisonchou Jun 7, 2021 •

edited

Loading

allisonchou commented Jun 8, 2021 •

edited

Loading

dibarbet Jun 9, 2021 •

edited

Loading