Get snippets working in Razor #9327

ryzngard · 2023-09-27T00:50:25Z

Adds support for the razor LSP to get snippets defined in VS under the following conditions:

They are not already being provided by the C# langauge server. This list
They are an expansion snippet. LSP doesn't support SurroundsWith because of selection needed.
We have some way of translating them.

Caveats here:

Right now we haven't added custom delimeter support.
Only HTML and CSharp snippets are used. We may need to add Javascript/Typescript, but we have no notion of that as a context right now.
The "Kind" is ignored. This treats all snippets like "any". We could potentially add support for this syntactically, but right now it's not added.
No Function, Declarations, Id, Import(s), Keyword(s), Literal, Namespace, Object, Reference(s), or Command support. These are in the snippet documentation. If needed we could address each of these individually.

ryzngard · 2023-09-27T00:51:14Z

Converting to Draft as there are known test breaks, and need to add unit tests. Finalizing the list of snippets we need to test with PM.

ryzngard · 2023-09-27T00:53:49Z

src/Razor/src/Microsoft.VisualStudio.LanguageServerClient.Razor/RazorCustomMessageTarget.cs

@@ -117,6 +127,8 @@ internal partial class RazorCustomMessageTarget
        _telemetryReporter = telemetryReporter;
        _languageServerFeatureOptions = languageServerFeatureOptions;
        _projectSnapshotManagerAccessor = projectSnapshotManagerAccessor;
+        _snippetCache = snippetCache;
+        _xmlSnippetParser = xmlSnippetParser ?? new XmlSnippetParser(null);


I'm not sure why the [Export] didn't work here, but as long as we're always using the same cache it's okay. Also unsure why we don't have ILoggerFactory here but I'm guessing that's just client vs server stuff.

I have just enough understanding of our logging to make things appear in logs when I want them, and to recognize that its all weird and probably wrong, but sadly not enough to know how to fix the wrongness :)

I think we would ideally have an ILoggerFactory here, and each method in this class would construct its own logger, since they are each endpoints. I'm sure there is an issue logged for better logging somewhere

allisonchou

The "Kind" is ignored. This treats all snippets like "any". We could potentially add support for this syntactically, but right now it's not added.
No Function, Declarations, Id, Import(s), Keyword(s), Literal, Namespace, Object, Reference(s), or Command support. These are in the snippet documentation. If needed we could address each of these individually.

As someone not super well versed in snippets, what are the implications of leaving out these properties? Do we end up getting snippets offered in places where they might not be valid?

Also, you mentioned that snippets already provided by the C# language server have been excluded. Which new C# ones are being provided with this change?

...azor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/RazorCompletionListProvider.cs

allisonchou · 2023-09-29T23:17:53Z

src/Razor/src/Microsoft.VisualStudio.Editor.Razor/Snippets/XmlSnippetParser.CodeSnippet.cs

+    /// Shamelessly copied from the editor
+    /// https://devdiv.visualstudio.com/DevDiv/_git/VS-Platform?path=/src/Editor/VisualStudio/Impl/Snippet/CodeSnippet.cs
+    /// </summary>
+    internal class CodeSnippet


Long term I wonder if we could convince the Editor to ship this class so we wouldn't have to make a copy

I agree, and now that there are two repo's that have copies of it (Roslyn and Razor) it would add weight to the argument for making a service of some kind to expose this info

Yea, we might be able to ask them to ship it. I can bring up in an LSP sync

davidwengier

I have a few comments, but I wouldn't be surprised if the answer to some or all of them was "yes it's odd but this just how VS/snippets work"

src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Common/LanguageServerConstants.cs

...azor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/RazorCompletionListProvider.cs

src/Razor/src/Microsoft.VisualStudio.Editor.Razor/Snippets/SnippetInfo.cs

src/Razor/src/Microsoft.VisualStudio.Editor.Razor/Snippets/SnippetCache.cs

src/Razor/src/Microsoft.VisualStudio.RazorExtension/Snippets/SnippetService.cs

davidwengier · 2023-10-01T21:44:10Z

src/Razor/src/Microsoft.VisualStudio.RazorExtension/Snippets/SnippetService.cs

+            s_CSharpLanguageId,
+            ImmutableHashSet.Create(
+                "~", "Attribute", "checked", "class", "ctor", "cw", "do", "else", "enum", "equals", "Exception", "for", "foreach", "forr",
+                "if", "indexer", "interface", "invoke", "iterator", "iterindex", "lock", "mbox", "namespace", "#if", "#region", "prop",


Not for now, but I'm excited by the prospect that removing "if" from this list might solve #6927 and let us turn on semantic snippets

ooo, maybe.

davidwengier · 2023-10-01T21:46:42Z

src/Razor/src/Microsoft.VisualStudio.RazorExtension/Snippets/SnippetService.cs

+    {
+        {
+            // These are identified as the snippets that are provided by the C# Language Server. The list is found
+            // in https://github.com/dotnet/roslyn/blob/8eb40e64564a2a8d44be2fde9b079605f1f10e0f/src/Features/LanguageServer/Protocol/Handler/InlineCompletions/InlineCompletionsHandler.cs#L41


I'm curious why we are doing snippets in normal completion, when Roslyn does them in inline completion?

(Note I am basing this question entirely off the filename in the comment here. I have no idea what the difference between the two is, or how the Roslyn code actually works 😛)

TBH I don't know the difference between normal and inline either :/

Co-authored-by: David Wengier <david.wengier@microsoft.com>

…ppets

davidwengier · 2023-10-02T02:19:50Z

src/Razor/src/Microsoft.VisualStudio.RazorExtension/Snippets/SnippetService.cs

+    private static readonly Dictionary<Guid, ImmutableHashSet<string>> s_ignoredSnippets = new()
+    {
+        {
+            // These are identified as the snippets that are provided by the C# Language Server. The list is found


Random thought: Should we remove the restriction on the Roslyn side, and let them give us all C# snippets, and then contribute this code, or some form of it, to Web Tools for html snippets?

Maybe. I think if we can get the editor to allow access to the snippet parsing code for everyone, then adding them to the html server and csharp server likely would be the right move.

…esolve

ryzngard · 2023-10-02T21:48:30Z

The "Kind" is ignored. This treats all snippets like "any". We could potentially add support for this syntactically, but right now it's not added.
No Function, Declarations, Id, Import(s), Keyword(s), Literal, Namespace, Object, Reference(s), or Command support. These are in the snippet documentation. If needed we could address each of these individually.

As someone not super well versed in snippets, what are the implications of leaving out these properties? Do we end up getting snippets offered in places where they might not be valid?

Also, you mentioned that snippets already provided by the C# language server have been excluded. Which new C# ones are being provided with this change?

These properties give a more complex way of providing snippets, and making sure they only show up in certain contexts. So yes, in some cases we will show snippets that are invalid for the location.

No new C# built in snippets are provided, but if a user provides a custom snippet they will now be included.

davidwengier

This looks good, pending tests and/or integration tests.

Still not sure about whether this will pass RPS either, so perhaps a validation insertion?

...zor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/RazorSnippetCompletionParams.cs

src/Razor/src/Microsoft.VisualStudio.RazorExtension/Snippets/SnippetService.cs

...test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/TestDocumentMappingService.cs

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

ryzngard · 2023-10-05T00:17:42Z

Waiting on RPS Validation before I merge

davidwengier · 2023-10-05T02:16:08Z

before I merge

Tests?

Is an integration test not really easy? All we need is one Html snippet that is shipped in-box, right?

DustinCampbell

Very glad to see this happening?

src/Razor/src/Microsoft.VisualStudio.Editor.Razor/Snippets/SnippetCache.cs

src/Razor/src/Microsoft.VisualStudio.Editor.Razor/Snippets/XmlSnippetParser.CodeSnippet.cs

src/Razor/src/Microsoft.VisualStudio.RazorExtension/Snippets/SnippetService.cs

DustinCampbell · 2023-10-05T17:40:37Z

src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/DictionaryExtensions.cs

+using System;
+using System.Collections.Generic;
+
+namespace Microsoft.AspNetCore.Razor;


Change namespace to System.Collections.Generic as we've done for other extension methods in Shared.Utilties to facilitate discoverability.

DustinCampbell · 2023-10-05T17:41:59Z

src/Shared/Microsoft.AspNetCore.Razor.Utilities.Shared/DictionaryExtensions.cs

+            dictionary[key] = defaultValue;
+            return dictionary[key];


Avoid a double-lookup in the dictionary:

Suggested change

dictionary[key] = defaultValue;

return dictionary[key];

dictionary.Add(key, defaultValue);

return defaultValue;

ryzngard · 2023-10-05T19:02:10Z

before I merge

Tests?

Is an integration test not really easy? All we need is one Html snippet that is shipped in-box, right?

I appreciate this comment. I added an integration test and apparently pushed it to the internal branch but not this one 🤦

Co-authored-by: Dustin Campbell <dustin@teamcampbell.org>

This reverts commit b95f430.

Herdo · 2024-08-17T11:27:19Z

@ryzngard Just found the limitations listed in this PR here. The listed limitations only apply to custom snippets, correct?
Built-in snippets seem to work correctly in the Razor editor, e.g. prop is correctly tabbing through the individual Declarations after inserting.

For custom snippets, if they have a Declaration, they don't insert at all.

DustinCampbell · 2024-08-19T16:05:56Z

@Herdo: could you file a new issue that references this pull request? Questions on a PR resolved 10 months ago might easily be missed!

ryzngard added 3 commits September 26, 2023 17:43

Get completions populating the cache

83226d2

Add snippet endpoint

7fd6a4d

Translate snippets

b4504e9

ryzngard requested review from a team as code owners September 27, 2023 00:50

ryzngard marked this pull request as draft September 27, 2023 00:50

Remove unused code

07124ea

ryzngard commented Sep 27, 2023

View reviewed changes

Get building

a8fb40b

ryzngard marked this pull request as ready for review September 27, 2023 23:00

allisonchou reviewed Sep 30, 2023

View reviewed changes

davidwengier reviewed Oct 1, 2023

View reviewed changes

ryzngard and others added 4 commits October 1, 2023 17:49

Apply suggestions from code review

74edbba

Co-authored-by: David Wengier <david.wengier@microsoft.com>

Fix broken tests

5d2d3af

Merge branch 'snippets' of https://github.com/ryzngard/razor into sni…

9e0d1c9

…ppets

Use ImmutableInterlocked

73f0c24

davidwengier reviewed Oct 2, 2023

View reviewed changes

Simplify completion text calculation. Make it so we only compute on r…

8848175

…esolve

ryzngard added 7 commits October 2, 2023 16:35

Simplify so snippet cache holds onto the snippet parser

19983cc

Change it so snippet info lazily generates the parsed xml

8a12bdd

Cache by language id in a dictionary

9cd1b32

Get rid of pragma ignore

7624659

Change threading in SnippetService

49b9524

Spelling is hard

5fb788b

Remove usings

2646be0

davidwengier approved these changes Oct 4, 2023

View reviewed changes

ryzngard and others added 2 commits October 3, 2023 18:18

Update src/Razor/src/Microsoft.VisualStudio.RazorExtension/Snippets/S…

686a790

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

Cleanup

647cc17

ryzngard added 2 commits October 4, 2023 15:14

Fix resolve for snippets

7b38ff2

add an integration test

a49fb9c

DustinCampbell reviewed Oct 5, 2023

View reviewed changes

ryzngard and others added 6 commits October 5, 2023 12:05

Apply suggestions from code review

ca73ace

Co-authored-by: Dustin Campbell <dustin@teamcampbell.org>

Clean up allocations and apply PR feedback

13ac95c

Merge branch 'main' into snippets

7bfa1d0

Small fixes

d5656fc

Fix warnings and change namespace

4e14ca1

Get rid of mock

9938683

ryzngard enabled auto-merge (squash) October 9, 2023 20:38

ryzngard merged commit b95f430 into dotnet:main Oct 9, 2023
12 checks passed

ghost added this to the Next milestone Oct 9, 2023

allisonchou mentioned this pull request Oct 10, 2023

[Automated] PRs inserted in VS build main-34210.119 #9386

Closed

ryzngard mentioned this pull request Oct 11, 2023

Snippets don't work in Razor Editor #4331

Closed

davidwengier added a commit that referenced this pull request Oct 13, 2023

Revert "Get snippets working in Razor (#9327)"

530e276

This reverts commit b95f430.

davidwengier mentioned this pull request Oct 13, 2023

Fix some snippets #9412

Merged

jjonescz modified the milestones: Next, 17.9 P1 Oct 31, 2023

Herdo mentioned this pull request Aug 17, 2024

Include useful Blazor snippets EngstromJimmy/Blazm.Extension#19

Open

Herdo mentioned this pull request Aug 19, 2024

C# snippets do not expand in *.razor files Visual Studio 17.2.0 #6397

Open

ryzngard deleted the snippets branch August 19, 2024 18:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Get snippets working in Razor #9327

Get snippets working in Razor #9327

ryzngard commented Sep 27, 2023

ryzngard commented Sep 27, 2023

ryzngard Sep 27, 2023

davidwengier Oct 1, 2023

allisonchou left a comment

allisonchou Sep 29, 2023

davidwengier Oct 1, 2023

ryzngard Oct 2, 2023

davidwengier left a comment •

edited

Loading

davidwengier Oct 1, 2023

ryzngard Oct 2, 2023

davidwengier Oct 1, 2023

ryzngard Oct 2, 2023

davidwengier Oct 2, 2023

ryzngard Oct 2, 2023

ryzngard commented Oct 2, 2023

davidwengier left a comment

ryzngard commented Oct 5, 2023

davidwengier commented Oct 5, 2023

DustinCampbell left a comment

DustinCampbell Oct 5, 2023

DustinCampbell Oct 5, 2023

ryzngard commented Oct 5, 2023

Herdo commented Aug 17, 2024

DustinCampbell commented Aug 19, 2024

Get snippets working in Razor #9327

Get snippets working in Razor #9327

Conversation

ryzngard commented Sep 27, 2023

ryzngard commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allisonchou left a 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

davidwengier left a comment • 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

Choose a reason for hiding this comment

ryzngard commented Oct 2, 2023

davidwengier left a comment

Choose a reason for hiding this comment

ryzngard commented Oct 5, 2023

davidwengier commented Oct 5, 2023

DustinCampbell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryzngard commented Oct 5, 2023

Herdo commented Aug 17, 2024

DustinCampbell commented Aug 19, 2024

davidwengier left a comment •

edited

Loading