Use snippet InsertText in directive attributes to insert equals and quotes #12010

alexgav · 2025-07-03T19:50:42Z

Summary of the changes

Using snippet insert text will ensure that equals and quotes are added on attribute commit, and cursor is moved to the correct position between double quotes.

This will insure that equals and quotes are added on attribute commit, and cursor is moved to the correct position between double quotes.

davidwengier · 2025-07-03T22:20:25Z

...crosoft.CodeAnalysis.Razor.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.cs

+            (var endIndex, var attributePrefix) = insertText.EndsWith("...", StringComparison.Ordinal) ? (^3, true) : (^0, false);

            // Don't allocate a new string unless we need to make a change.
            if (startIndex > 0 || endIndex.Value > 0)
            {
                insertText = insertText[startIndex..endIndex];
            }

+            var isSnippet = false;
+            if (!attributePrefix // Don't even try to add snippet to something like "@bind-..."
+                && TryGetSnippetText(containingAttribute, insertText, razorCompletionOptions, out var snippetText))
+            {
+                insertText = snippetText;
+                isSnippet = true;
+            }


Not a big fan of the tuple-returning-ternary, but I think this whole bit of code is pretty hard to follow, and it took me a while to work out what attributePrefix means (I think the word "prefix" and the fact that its based on the presence of a suffix does my head in). Can it be expanded? Something like (untested):

var insertText = displayText.AsSpan(); if (insertText.StartsWith('@') { insertText = insertText[1..]; } if (insertText.EndsWith("...", StringComparison.Orginal)) { insertText = insertText[..^3]; } else { var isSnippet = false; if (TryGetSnippetText(containingAttribute, insertText, razorCompletionOptions, out var snippetText)) { insertText = snippetText; isSnippet = true; } }

Obviously either TryGetSnippetText will need to change, because inputText is now a span, but I think making it a span early means not having to jump through so many logic hoops to keep track of everything.

I'm not sure all of the span calculatoins/collection operators made things easier to read, but I gave it a try. Let me know if you see ways to simplify things. Concatenation of spans seems a bit messy, not sure if there is a way to do it better. Ambiguity and different parameter sets (singe value vs span) in SpanExtensions and MemoryExtensions make things a bit messy to IMHO. Let me know if you know of a way to clean this up more. So far I'm really on the fence about this, but if it makes it easier to read for you I guess it's fine.

src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/RazorCompletionItem.cs

davidwengier · 2025-07-03T22:30:51Z

...odeAnalysis.Razor.Workspaces.Test/Completion/DirectiveAttributeCompletionItemProviderTest.cs

@@ -45,7 +48,7 @@ private RazorCodeDocument GetCodeDocument(string content)
    public void GetCompletionItems_OnNonAttributeArea_ReturnsEmptyCollection()
    {
        // Arrange
-        var context = CreateRazorCompletionContext(absoluteIndex: 3, "<input @  />");
+        var context = CreateRazorCompletionContext("<in$$put @  />");


Love all of these updates to the test inputs, thank you! #Resolved

Thanks, yeah, inputs were pretty hard to read before, weren't they :)

davidwengier · 2025-07-03T22:32:19Z

...odeAnalysis.Razor.Workspaces.Test/Completion/DirectiveAttributeCompletionItemProviderTest.cs

@@ -278,15 +330,21 @@ private static void AssertDoesNotContain(IReadOnlyList<RazorCompletionItem> comp
               RazorCompletionItemKind.DirectiveAttribute == completion.Kind);
    }

-    private RazorCompletionContext CreateRazorCompletionContext(int absoluteIndex, string documentContent)
+    private RazorCompletionContext CreateRazorCompletionContext(string testCodeText)


You can just make the parameter TestCode testCode, and remove line 335, and everything should still comple the same. There is an implicit conversion. #Resolved

Thanks, cleaned that up.

davidwengier

Yes, I do think it's much easier to read now, though I'd love to know why they can't be proper extension method calls.

davidwengier · 2025-07-08T03:32:11Z

...crosoft.CodeAnalysis.Razor.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.cs


            // Strip off the @ from the insertion text. This change is here to align the insertion text with the
            // completion hooks into VS and VSCode. Basically, completion triggers when `@` is typed so we don't
            // want to insert `@bind` because `@` already exists.
-            var startIndex = insertText.StartsWith('@') ? 1 : 0;
+            if (SpanExtensions.StartsWith(insertTextSpan, '@'))


Why can't this just be used as an extension method?

both of them are in System - not sure if there is a better way to address this, but this is super-weird.

davidwengier · 2025-07-08T03:33:29Z

...crosoft.CodeAnalysis.Razor.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.cs

-
-            // Don't allocate a new string unless we need to make a change.
-            if (startIndex > 0 || endIndex.Value > 0)
+            if (MemoryExtensions.EndsWith(insertTextSpan, "...".AsSpan()))


As above :)

davidwengier · 2025-07-08T03:35:18Z

...crosoft.CodeAnalysis.Razor.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.cs

+                && containingAttribute is not (MarkupTagHelperDirectiveAttributeSyntax or MarkupAttributeBlockSyntax)
+                && containingAttribute.Parent is not (MarkupTagHelperDirectiveAttributeSyntax or MarkupAttributeBlockSyntax))
+            {
+                var suffixTextSpan = razorCompletionOptions.AutoInsertAttributeQuotes ? "=\"$0\"".AsSpan() : "=$0".AsSpan();


I think it would be better to store these two spans as static ReadOnlyMemory<char> so they're only created once.

Learning opportunity for me. What extra allocations happen here? I would have thought the strings would be compiled into consts, and then the AsSpan calls would just create a struct around that.

I believe this is the implementation so it seems allocation does happen and caching makes sense

https://github.com/dotnet/runtime/blob/b02b5a026ce4929f62d22a69980db18e8d19f388/src/libraries/System.Private.CoreLib/src/System/MemoryExtensions.cs#L97C42-L97C48

I believe they're all structs, so not really an "allocation" as we would usually talk about it (ie, not something to be garbage collected), I just meant to store then as a static readonly, like you'd do with any other thing that is only ever one value, but can't be a const for reasons.

But really I'm just copying @DustinCampbell since I assume he actually knows what he's talking about: https://github.com/dotnet/razor/blob/main/src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/RazorHtmlWriter.cs#L18

... or maybe not, looking at it more closely. Either way, seems like caching can't hurt.

ToddGrun · 2025-07-08T12:51:48Z

...crosoft.CodeAnalysis.Razor.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.cs

@@ -140,15 +154,42 @@ internal static ImmutableArray<RazorCompletionItem> GetAttributeCompletions(

            var razorCompletionItem = RazorCompletionItem.CreateDirectiveAttribute(
                displayText,
-                insertText,
+                insertTextSpan.ToString(),


insertTextSpan.ToString(),

In the common case (none of the conditions above hit), aren't we allocating something for insertTextSpan whereas we didn't before?

I don't think we actually allocate here because ReadOnlySpan special-cases 'char' case

https://github.com/dotnet/runtime/blob/6d869a82ccbad6c279da52f9bd507057e6e30495/src/libraries/System.Private.CoreLib/src/System/ReadOnlySpan.cs#L358

Also, the common case is actually use modifying the string rather than not modifying it - snippet ="{0}" is almost always added. It's not added only for indexers (ending with "...").

However, it was easy enough to check by adding another local (let me know if I'm overlooking a clever way of doing it), so I modified the code to do so. Thanks!

I don't think we actually allocate here

Isn't that specialization you linked to doing a string allocation? Either way, the check you added makes that no longer a concern.

public override string ToString() { if (typeof(T) == typeof(char)) { return new string(new ReadOnlySpan<char>(ref Unsafe.As<T, char>(ref _reference), _length)); } return $"System.ReadOnlySpan<{typeof(T).Name}>[{_length}]"; }

It's a similar one, but I did say later ".. or maybe not, looking at it more closely" :)

ToddGrun · 2025-07-08T19:48:06Z

...crosoft.CodeAnalysis.Razor.Workspaces/Completion/DirectiveAttributeCompletionItemProvider.cs


            completionItems.Add(razorCompletionItem);
        }

        return completionItems.ToImmutableAndClear();

+        bool TryGetSnippetText(


bool TryGetSnippetText(

nit: can this be static instead to be more explicit about what is being captured?

There was actually a subtle bug there where I was capturing containingAttribute unintentionally. Nice suggestion, thanks. Fixed.

alexgav · 2025-07-10T06:25:15Z

I'll enable auto-merge per earlier in-person conversation. Happy to address any additional feedback (if any) in a follow-up PR.

Use sinppet InsertText in directive attributes

6853aec

This will insure that equals and quotes are added on attribute commit, and cursor is moved to the correct position between double quotes.

alexgav requested a review from a team as a code owner July 3, 2025 19:50

davidwengier approved these changes Jul 3, 2025

View reviewed changes

CR feedback

ef6335c

davidwengier reviewed Jul 8, 2025

View reviewed changes

davidwengier requested a review from DustinCampbell July 8, 2025 03:36

More CR suggestions

f638e97

ToddGrun reviewed Jul 8, 2025

View reviewed changes

alexgav added 2 commits July 8, 2025 09:41

CR feeback - don't create another string unnecessarily

e567f00

User range operator instead of Slice per analyzer suggestion

958729a

ToddGrun reviewed Jul 8, 2025

View reviewed changes

ToddGrun approved these changes Jul 8, 2025

View reviewed changes

CR feedback - make local function static

7dff5f9

alexgav enabled auto-merge (squash) July 10, 2025 06:26

alexgav merged commit 27d4d4c into main Jul 10, 2025
11 checks passed

alexgav deleted the dev/alexgav/UseSnippetInsertTextInDirectiveAttributes branch July 10, 2025 06:59

dotnet-policy-service bot added this to the Next milestone Jul 10, 2025

davidwengier mentioned this pull request Jul 14, 2025

Bump Razor to 10.0.0-preview.25360.2 dotnet/vscode-csharp#8413

Merged

RikkiGibson modified the milestones: Next, 18.0 P1 Aug 20, 2025

Use snippet InsertText in directive attributes to insert equals and quotes #12010

Use snippet InsertText in directive attributes to insert equals and quotes #12010

Uh oh!

Conversation

alexgav commented Jul 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of the changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

davidwengier Jul 3, 2025 • edited by alexgav Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwengier Jul 3, 2025 • edited by alexgav Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidwengier 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!

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!

ToddGrun Jul 8, 2025 • 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!

alexgav Jul 8, 2025 • 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!

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!

alexgav commented Jul 10, 2025

Uh oh!

Uh oh!

Uh oh!

alexgav commented Jul 3, 2025 •

edited

Loading

davidwengier Jul 3, 2025 •

edited by alexgav

Loading

davidwengier Jul 3, 2025 •

edited by alexgav

Loading

ToddGrun Jul 8, 2025 •

edited

Loading

alexgav Jul 8, 2025 •

edited

Loading