Add support for invoking C# inline completions by dibarbet · Pull Request #6012 · dotnet/razor

dibarbet · 2022-01-26T04:50:41Z

Summary of the changes

Add support for the internal inline completions API for C#

Resolves #4322

dibarbet · 2022-01-26T04:53:32Z

                SupportsDiagnosticRequests = true,
+                InlineCompletionOptions = new VSInternalInlineCompletionOptions
+                {
+                    Pattern = new Regex(string.Join("|", InlineCompletionHandler.CSharpKeywords))


this is rather hardcoded, but the plan is to eventually not need the pattern once the client side is fully async. So imho not worth doing anything super complicated here.

Totally agree!

Even though it literallllly means nothing in practice lets also add , RegexOptions.Compiled | RegexOptions.Singleline, TimeSpan.FromSeconds(1)) to the regex just for any future on-lookers it's easy to see the pattern we typically follow for Regex.

I am all for anything and everything we can do to get rid of Regex. Especially in a spot like this where the usage is super simple. Is there an issue filed about the ClientSide async stuff, and does it specifically call out the pattern for removal? If so I'm happy (after Taylors Regex safety changes above).

Actually, on second thought is this pattern really correct? Wouldn't this match things like therefor or format (which I assume it should not)? Should we add \b to the front/and/or/end of each keyword, or does the platform already do that for us?

Would this trigger on:

@code{ var str = "foreach"; }

? Or is that fine because we'll just let the C# LS return no results?

Is there an issue filed about the ClientSide async stuff, and does it specifically call out the pattern for removal? If so I'm happy (after Taylors Regex safety changes above).

Finding it... will link here once I do.
Edit: https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1086001/

Actually, on second thought is this pattern really correct? Wouldn't this match things like therefor or format (which I assume it should not)? Should we add \b to the front/and/or/end of each keyword, or does the platform already do that for us?

It would, I can add \b, but note

Would this trigger on:

Yes, the regex is just an optimization due to the blocking nature of the request. It is up to the language server to make the final decision as to whether or not there is actually a snippet there.

DoctorKrolic · 2022-01-26T05:26:47Z

Finally! 🎉🎉🎉

NTaylorMullen

This is awesome to see light up and is just the beginning for what we can do in this area. Super exciting and awesome work @dibarbet!

Had one big design change on moving where the handler exists but the changes required are mostly boiler plate, hopefully wont be too big.

NTaylorMullen · 2022-01-26T17:24:06Z

                SupportsDiagnosticRequests = true,
+                InlineCompletionOptions = new VSInternalInlineCompletionOptions
+                {
+                    Pattern = new Regex(string.Join("|", InlineCompletionHandler.CSharpKeywords))


Totally agree!

Even though it literallllly means nothing in practice lets also add , RegexOptions.Compiled | RegexOptions.Singleline, TimeSpan.FromSeconds(1)) to the regex just for any future on-lookers it's easy to see the pattern we typically follow for Regex.

NTaylorMullen · 2022-01-26T17:27:34Z

+        private VSInternalInlineCompletionOptions GetMergedInlineCompletionProvider()
+        {
+            var regexes = _serverCapabilities.Where(s => s.Capabilities.InlineCompletionOptions != null).Select(s => s.Capabilities.InlineCompletionOptions.Pattern.ToString());
+            return new VSInternalInlineCompletionOptions { Pattern = new Regex(string.Join("|", regexes)) };


NTaylorMullen · 2022-01-26T17:32:46Z

+    [ExportLspMethod(VSInternalMethods.TextDocumentInlineCompletionName)]
+    internal class InlineCompletionHandler : IRequestHandler<VSInternalInlineCompletionRequest, VSInternalInlineCompletionList?>
+    {
+        internal static readonly ImmutableHashSet<string> CSharpKeywords = ImmutableHashSet.Create(


lmao for a few of these I went into a C# file and typed them to see what was up. Had no clue what many of em did.

Digging through these does bring up an interesting question on if we want to truncate the list of available snippet keywords so only ones that apply to Razor are available. For instance I could imagine us excluding

mbox

namespace

sim

svm

What's your view here? Not sure how excluding some might interact with bits in the completion list but I don't have the entire end-to-end in my head.

Also kind of curious how many of these plan to be revived as part of the semantic snippet work we're doing @jmarolf / @akhera99 is that known yet?

Excluding some will have slightly weird behavior - the snippet will show up in the list but not be expandable. I would lean towards just leaving them - semantic snippets should be the proper solution here.

And custom snippets would have the same issue.

This is called out in this section of the spec: https://github.com/dotnet/roslyn/blob/feature/semantic-snippets/docs/ide/specs/semantic_snippets.md#scenario-2-snippets-only-show-up-where-they-are-valid

The intent is that something like mbox would only show up if the required types are present and its in the right context (i.e. inside a method or top level statement). We'll need to decide what valid means for razor contexts but I assume its similar.

NTaylorMullen · 2022-01-26T17:43:00Z

+                var range = item.Range ?? new Range { Start = projectionResult.Position, End = projectionResult.Position };
+
+                var textEdit = new TextEdit { NewText = item.Text, Range = range };
+                var remappedEdit = await _documentMappingProvider.RemapFormattedTextEditsAsync(projectionResult.Uri, new[] { textEdit }, formattingOptions, containsSnippet, cancellationToken).ConfigureAwait(false);


Do you think instead of having this overarching class as a handler in the HtmlCSharp language server we could have this as an endpoint in the Razor language server? I imagine it'd be similar to our CodeAction endpoint that delegates back to the client to acquire code actions on the C# language server. That being said the CodeAction bits are a first-class endpoint in O# BUT we could treat this inline completion endpoint similar to the OnAutoInsertEndpoint where we declare all the necessary bits to interop with VS. I guess even a better example which is a custom request that delegates over to C# is @davidwengier's breakpoint request / addition.

Some benefits of having it in the Razor language server:

This formatting call does a StreamJsonRpc request for every response edit in order to format it. If it's all in the language server that wouldn't require any StreamJsonRpc over-the-wire hops

We'll be able to add functionality in a single location for any future snippet expansion (I'm imagining expanding things like @inject in Razor

Sets us up for adopting the new embedded language spec re-design (it'd basically be following that model exactly)

Allows us greater control on when we want to allow C# snippet expansion. For instance, we probably don't want to allow any snippet expansion in Razor directive attributes (don't have to code this for now but it's an example of a benefit in having the endpoint in the Razor language server)

NTaylorMullen · 2022-01-26T18:12:33Z

Oooo and with this we can totalllly nuke the code in our completion handler that attempts to polyfill C# keywords. This will be a masssssssive win

ryanbrandenburg

I largely agree with @NTaylorMullen's feedback but there are some edge-cases around the Regex's which seem like they could lead to weird behavior. If I'm off-base there it's probably worth adding some comments with explanations.

ryanbrandenburg · 2022-01-26T18:08:55Z

                SupportsDiagnosticRequests = true,
+                InlineCompletionOptions = new VSInternalInlineCompletionOptions
+                {
+                    Pattern = new Regex(string.Join("|", InlineCompletionHandler.CSharpKeywords))


I am all for anything and everything we can do to get rid of Regex. Especially in a spot like this where the usage is super simple. Is there an issue filed about the ClientSide async stuff, and does it specifically call out the pattern for removal? If so I'm happy (after Taylors Regex safety changes above).

ryanbrandenburg · 2022-01-26T18:15:11Z

                SupportsDiagnosticRequests = true,
+                InlineCompletionOptions = new VSInternalInlineCompletionOptions
+                {
+                    Pattern = new Regex(string.Join("|", InlineCompletionHandler.CSharpKeywords))


Actually, on second thought is this pattern really correct? Wouldn't this match things like therefor or format (which I assume it should not)? Should we add \b to the front/and/or/end of each keyword, or does the platform already do that for us?

ryanbrandenburg · 2022-01-26T18:24:00Z


+        private VSInternalInlineCompletionOptions GetMergedInlineCompletionProvider()
+        {
+            var regexes = _serverCapabilities.Where(s => s.Capabilities.InlineCompletionOptions != null).Select(s => s.Capabilities.InlineCompletionOptions.Pattern.ToString());


I'm not super up to date on this class, do we expect (now or in the future) there to be more than one _serverCapability that matches this? If not we should change this to .First(). Also what happens if InlineCompletionOptions.Pattern is null?

yeah there definitely could be - for example C# and html both providing snippets and we would want to trigger on keywords from both. If the pattern is null it just means it provides no keywords.

If the pattern is null it just means it provides no keywords.

I more meant that since Pattern might be null or un-assigned that calling Pattern.ToString() might cause an exception.

ryanbrandenburg · 2022-01-26T18:29:49Z

                SupportsDiagnosticRequests = true,
+                InlineCompletionOptions = new VSInternalInlineCompletionOptions
+                {
+                    Pattern = new Regex(string.Join("|", InlineCompletionHandler.CSharpKeywords))


Would this trigger on:

@code{ var str = "foreach"; }

? Or is that fine because we'll just let the C# LS return no results?

…errors in snippets

NTaylorMullen

Getting super close and really loving the move to the language server!

NTaylorMullen · 2022-01-27T17:36:30Z

+    // by the platform. Similar to DefaultCSharpCodeActionResolver we do not have any, so use defaults.
+    private static readonly FormattingOptions s_defaultFormattingOptions = new FormattingOptions()
+    {
+        TabSize = 4,


@allisonchou IIRC we had an options store on the Razor language server side that was regularly updated. Am I remembering wrong? Wondering if there's a way @dibarbet can hook into that

if we do get the razor formatting options though, I think they still need to be passed to C# so C# formats the snippet with the right settings. So regardless I think the best way is to just include them on the request.

Ahhhh good point. Agreed as a good follow up!

NTaylorMullen

Hell ya! This is going to be amazing. 👏

NTaylorMullen · 2022-01-28T02:15:26Z

+    // by the platform. Similar to DefaultCSharpCodeActionResolver we do not have any, so use defaults.
+    private static readonly FormattingOptions s_defaultFormattingOptions = new FormattingOptions()
+    {
+        TabSize = 4,


Ahhhh good point. Agreed as a good follow up!

NTaylorMullen · 2022-01-28T02:17:24Z

+        var snippetSourceText = SourceText.From(snippetText);
+        List<TextChange> indentationChanges = new();
+        // Adjust each line, skipping the first since it must start at the snippet keyword.
+        foreach (var line in snippetSourceText.Lines.Skip(1))


I think combining the logic for formatting snippets (at least for C#) is a good idea - however I would like to battle test this formatting code before we unify things

Totally fair!

Add support for invoking C# inline completions

86459fd

dibarbet requested a review from NTaylorMullen January 26, 2022 04:50

dibarbet requested review from a team, allisonchou, davidwengier, ryanbrandenburg and sharwell as code owners January 26, 2022 04:50

dibarbet commented Jan 26, 2022

View reviewed changes

dibarbet mentioned this pull request Jan 26, 2022

Razor snippets dotnet/roslyn#58857

Merged

davidwengier approved these changes Jan 26, 2022

View reviewed changes

NTaylorMullen suggested changes Jan 26, 2022

View reviewed changes

ryanbrandenburg reviewed Jan 26, 2022

View reviewed changes

dibarbet added 2 commits January 26, 2022 13:09

small feedback

059ad97

move impl to razor ls

12e617b

dibarbet force-pushed the razor_snippets branch 2 times, most recently from 689bc13 to 5daccc9 Compare January 27, 2022 10:29

dibarbet commented Jan 27, 2022

View reviewed changes

Comment thread ...r/src/Microsoft.AspNetCore.Razor.LanguageServer/InlineCompletion/InlineCompletionEndPoint.cs

dibarbet force-pushed the razor_snippets branch from 5daccc9 to ace071f Compare January 27, 2022 11:03

dibarbet commented Jan 27, 2022

View reviewed changes

Comment thread ...r/src/Microsoft.AspNetCore.Razor.LanguageServer/InlineCompletion/InlineCompletionEndPoint.cs

dibarbet force-pushed the razor_snippets branch 3 times, most recently from bb24af2 to fa9af10 Compare January 27, 2022 11:16

Switch to alternate method of formatting to avoid issues with syntax …

87e5a91

…errors in snippets

dibarbet force-pushed the razor_snippets branch from fa9af10 to 87e5a91 Compare January 27, 2022 11:29

NTaylorMullen suggested changes Jan 27, 2022

View reviewed changes

davidwengier reviewed Jan 27, 2022

View reviewed changes

Comment thread ...r/src/Microsoft.AspNetCore.Razor.LanguageServer/InlineCompletion/InlineCompletionEndPoint.cs

dibarbet added 2 commits January 27, 2022 15:37

small fixes

9db9ad2

remove more unnecessary completion code

78a159e

dibarbet added 2 commits January 27, 2022 16:46

Use razor type for middle layer bit

a56c82b

Merge remote-tracking branch 'upstream/main' into razor_snippets

8a16091

dibarbet requested a review from NTaylorMullen January 28, 2022 01:04

dibarbet added 3 commits January 27, 2022 17:11

fix warnings

c2f64b7

add missing file

f50497b

Fixup/remove unneeded completion tests

2f477d2

NTaylorMullen approved these changes Jan 28, 2022

View reviewed changes

dibarbet merged commit 3e9454d into dotnet:main Jan 28, 2022

dibarbet deleted the razor_snippets branch January 28, 2022 19:13

DoctorKrolic mentioned this pull request Jan 28, 2022

Fixed wrong completion icons #5999

Closed

DoctorKrolic mentioned this pull request Feb 17, 2022

Adding snippets via post processing is no longer needed #6106

Closed

Conversation

dibarbet commented Jan 26, 2022

Summary of the changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dibarbet Jan 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!

DoctorKrolic commented Jan 26, 2022

Uh oh!

NTaylorMullen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NTaylorMullen commented Jan 26, 2022

Uh oh!

ryanbrandenburg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NTaylorMullen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

NTaylorMullen left a comment

Choose a reason for hiding this comment

Uh oh!

dibarbet Jan 26, 2022 •

edited

Loading