Minor clean up of some completion code (#11609)

davidwengier · web-flow · commit 284b32d1cdc6 · 2025-03-14T14:32:21.000+11:00
Starting to look at cohosting completion resolve and noticed some things
that annoyed me :)
diff --git a/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/Delegation/DelegatedCompletionItemResolver.cs b/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/Delegation/DelegatedCompletionItemResolver.cs
@@ -31,7 +31,7 @@ internal class DelegatedCompletionItemResolver(
     public override async Task<VSInternalCompletionItem?> ResolveAsync(
         VSInternalCompletionItem item,
         VSInternalCompletionList containingCompletionList,
-        object? originalRequestContext,
+        ICompletionResolveContext originalRequestContext,
         VSInternalClientCapabilities? clientCapabilities,
         IComponentAvailabilityService componentAvailabilityService,
         CancellationToken cancellationToken)
diff --git a/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/RazorCompletionResolveEndpoint.cs b/src/Razor/src/Microsoft.AspNetCore.Razor.LanguageServer/Completion/RazorCompletionResolveEndpoint.cs
@@ -1,6 +1,7 @@
 ﻿// Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the MIT license. See License.txt in the project root for license information.
 
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
@@ -33,37 +34,8 @@ public void ApplyCapabilities(VSInternalServerCapabilities serverCapabilities, V
 
     public async Task<VSInternalCompletionItem> HandleRequestAsync(VSInternalCompletionItem completionItem, RazorRequestContext requestContext, CancellationToken cancellationToken)
     {
-        if (!completionItem.TryGetCompletionListResultIds(out var resultIds))
-        {
-            // Unable to lookup completion item result info
-            return completionItem;
-        }
-
-        object? originalRequestContext = null;
-        VSInternalCompletionList? containingCompletionList = null;
-        foreach (var resultId in resultIds)
-        {
-            if (!_completionListCache.TryGet(resultId, out var cacheEntry))
-            {
-                continue;
-            }
-
-            // See if this is the right completion list for this corresponding completion item. We cross-check this based on label only given that
-            // is what users interact with.
-            if (cacheEntry.CompletionList.Items.Any(completion =>
-                completionItem.Label == completion.Label
-                // Check the Kind as well, e.g. we may have a Razor snippet and a C# keyword with the same label, etc.
-                && completionItem.Kind == completion.Kind))
-            {
-                originalRequestContext = cacheEntry.Context;
-                containingCompletionList = cacheEntry.CompletionList;
-                break;
-            }
-        }
-
-        if (containingCompletionList is null)
+        if (!TryGetOriginalRequestData(completionItem, out var containingCompletionList, out var originalRequestContext))
         {
-            // Couldn't find an associated completion list
             return completionItem;
         }
 
@@ -81,4 +53,33 @@ public async Task<VSInternalCompletionItem> HandleRequestAsync(VSInternalComplet
 
         return resolvedCompletionItem;
     }
+
+    private bool TryGetOriginalRequestData(VSInternalCompletionItem completionItem, [NotNullWhen(true)] out VSInternalCompletionList? completionList, [NotNullWhen(true)] out ICompletionResolveContext? context)
+    {
+        context = null;
+        completionList = null;
+
+        if (!completionItem.TryGetCompletionListResultIds(out var resultIds))
+        {
+            // Unable to lookup completion item result info
+            return false;
+        }
+
+        foreach (var resultId in resultIds)
+        {
+            // See if this is the right completion list for this corresponding completion item. We cross-check this based on label only given that
+            // is what users interact with.
+            if (_completionListCache.TryGet(resultId, out completionList, out context) &&
+                completionList.Items.Any(
+                    completion =>
+                        completionItem.Label == completion.Label &&
+                        // Check the Kind as well, e.g. we may have a Razor snippet and a C# keyword with the same label, etc.
+                        completionItem.Kind == completion.Kind))
+            {
+                return true;
+            }
+        }
+
+        return false;
+    }
 }
diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/AggregateCompletionItemResolver.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/AggregateCompletionItemResolver.cs
@@ -28,7 +28,7 @@ public AggregateCompletionItemResolver(IEnumerable<CompletionItemResolver> compl
     public async Task<VSInternalCompletionItem?> ResolveAsync(
         VSInternalCompletionItem item,
         VSInternalCompletionList containingCompletionList,
-        object? originalRequestContext,
+        ICompletionResolveContext originalRequestContext,
         VSInternalClientCapabilities? clientCapabilities,
         IComponentAvailabilityService componentAvailabilityService,
         CancellationToken cancellationToken)
diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/CompletionItemResolver.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/CompletionItemResolver.cs
@@ -13,7 +13,7 @@ internal abstract class CompletionItemResolver
     public abstract Task<VSInternalCompletionItem?> ResolveAsync(
         VSInternalCompletionItem item,
         VSInternalCompletionList containingCompletionList,
-        object? originalRequestContext,
+        ICompletionResolveContext originalRequestContext,
         VSInternalClientCapabilities? clientCapabilities,
         IComponentAvailabilityService componentAvailabilityService,
         CancellationToken cancellationToken);
diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/CompletionListCache.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/CompletionListCache.cs
@@ -1,18 +1,14 @@
 ﻿// Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the MIT license. See License.txt in the project root for license information.
 
-using System;
+using System.Diagnostics.CodeAnalysis;
 using Microsoft.VisualStudio.LanguageServer.Protocol;
 
 namespace Microsoft.CodeAnalysis.Razor.Completion;
 
 internal class CompletionListCache
 {
-    private record struct Slot(
-        int Id,
-        VSInternalCompletionList CompletionList,
-        object? Context,
-        bool Used = true);
+    private record struct Slot(int Id, VSInternalCompletionList CompletionList, ICompletionResolveContext Context);
 
     // Internal for testing
     internal const int MaxCacheSize = 10;
@@ -25,13 +21,8 @@ private record struct Slot(
     private int _nextIndex;
     private int _nextId;
 
-    public int Add(VSInternalCompletionList completionList, object? context)
+    public int Add(VSInternalCompletionList completionList, ICompletionResolveContext context)
     {
-        if (completionList is null)
-        {
-            throw new ArgumentNullException(nameof(completionList));
-        }
-
         lock (_accessLock)
         {
             var index = _nextIndex++;
@@ -52,7 +43,7 @@ public int Add(VSInternalCompletionList completionList, object? context)
         }
     }
 
-    public bool TryGet(int id, out (VSInternalCompletionList CompletionList, object? Context) result)
+    public bool TryGet(int id, [NotNullWhen(true)] out VSInternalCompletionList? completionList, [NotNullWhen(true)] out ICompletionResolveContext? context)
     {
         lock (_accessLock)
         {
@@ -74,22 +65,27 @@ public bool TryGet(int id, out (VSInternalCompletionList CompletionList, object?
 
                 var slot = _items[index];
 
-                if (!slot.Used)
+                // CompletionList is annotated as non-nullable, but we are allocating an array of 10 items for our cache, so initially
+                // those array entries will be default. By checking for null here, we detect if we're hitting an unused part of the array
+                // so stop looping.
+                if (slot.CompletionList is null)
                 {
                     break;
                 }
 
                 if (slot.Id == id)
                 {
-                    result = (slot.CompletionList, slot.Context);
+                    completionList = slot.CompletionList;
+                    context = slot.Context;
                     return true;
                 }
 
                 count--;
             }
 
             // A cache entry associated with the given id was not found.
-            result = default;
+            completionList = null;
+            context = null;
             return false;
         }
     }
diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/CompletionListMerger.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/CompletionListMerger.cs
@@ -4,6 +4,7 @@
 using System.Collections.Generic;
 using System.Collections.Immutable;
 using System.Diagnostics;
+using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using System.Text.Json;
 using Microsoft.AspNetCore.Razor.PooledObjects;
@@ -17,6 +18,8 @@ internal static class CompletionListMerger
     private static readonly string s_data2Key = nameof(MergedCompletionListData.Data2);
     private static readonly object s_emptyData = new();
 
+    [return: NotNullIfNotNull(nameof(razorCompletionList))]
+    [return: NotNullIfNotNull(nameof(delegatedCompletionList))]
     public static VSInternalCompletionList? Merge(VSInternalCompletionList? razorCompletionList, VSInternalCompletionList? delegatedCompletionList)
     {
         if (razorCompletionList is null)
diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/ICompletionResolveContext.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/ICompletionResolveContext.cs
@@ -0,0 +1,11 @@
+﻿// Copyright (c) .NET Foundation. All rights reserved.
+// Licensed under the MIT license. See License.txt in the project root for license information.
+
+namespace Microsoft.CodeAnalysis.Razor.Completion;
+
+/// <summary>
+/// Marker interface to make the CompletionListCache API easier to use
+/// </summary>
+internal interface ICompletionResolveContext
+{
+}
diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/RazorCompletionItemResolver.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/RazorCompletionItemResolver.cs
@@ -17,7 +17,7 @@ internal class RazorCompletionItemResolver : CompletionItemResolver
     public override async Task<VSInternalCompletionItem?> ResolveAsync(
         VSInternalCompletionItem completionItem,
         VSInternalCompletionList containingCompletionList,
-        object? originalRequestContext,
+        ICompletionResolveContext originalRequestContext,
         VSInternalClientCapabilities? clientCapabilities,
         IComponentAvailabilityService componentAvailabilityService,
         CancellationToken cancellationToken)
diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/RazorCompletionResolveContext.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Completion/RazorCompletionResolveContext.cs
@@ -5,4 +5,4 @@
 
 namespace Microsoft.CodeAnalysis.Razor.Completion;
 
-internal record RazorCompletionResolveContext(string FilePath, ImmutableArray<RazorCompletionItem> CompletionItems);
+internal record RazorCompletionResolveContext(string FilePath, ImmutableArray<RazorCompletionItem> CompletionItems) : ICompletionResolveContext;
diff --git a/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Protocol/DelegatedTypes.cs b/src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/Protocol/DelegatedTypes.cs
@@ -10,6 +10,7 @@
 
 namespace Microsoft.CodeAnalysis.Razor.Protocol;
 
+using Microsoft.CodeAnalysis.Razor.Completion;
 using Microsoft.VisualStudio.LanguageServer.Protocol;
 
 internal record DelegatedSpellCheckParams(
@@ -70,7 +71,7 @@ internal record DelegatedMapCodeParams(
 
 internal record DelegatedCompletionResolutionContext(
     [property: JsonPropertyName("originalRequestParams")] DelegatedCompletionParams OriginalRequestParams,
-    [property: JsonPropertyName("originalCompletionListData")] object? OriginalCompletionListData);
+    [property: JsonPropertyName("originalCompletionListData")] object? OriginalCompletionListData) : ICompletionResolveContext;
 
 internal record DelegatedCompletionItemResolveParams(
     [property: JsonPropertyName("identifier")] TextDocumentIdentifierAndVersion Identifier,
diff --git a/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/Delegation/DelegatedCompletionItemResolverTest.NetFx.cs b/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/Delegation/DelegatedCompletionItemResolverTest.NetFx.cs
@@ -1,8 +1,6 @@
 ﻿// Copyright (c) .NET Foundation. All rights reserved.
 // Licensed under the MIT license. See License.txt in the project root for license information.
 
-#nullable disable
-
 using System;
 using System.Collections.Generic;
 using System.Linq;
@@ -12,6 +10,7 @@
 using Microsoft.AspNetCore.Razor.LanguageServer.Formatting;
 using Microsoft.AspNetCore.Razor.LanguageServer.Hover;
 using Microsoft.AspNetCore.Razor.LanguageServer.Test;
+using Microsoft.AspNetCore.Razor.Test.Common;
 using Microsoft.AspNetCore.Razor.Test.Common.LanguageServer;
 using Microsoft.CodeAnalysis.Razor.Completion;
 using Microsoft.CodeAnalysis.Razor.Formatting;
@@ -94,7 +93,7 @@ public async Task ResolveAsync_CanNotFindCompletionItem_Noops()
         var resolver = new DelegatedCompletionItemResolver(_documentContextFactory, _formattingService.GetValue(), optionsMonitor, server);
         var item = new VSInternalCompletionItem();
         var notContainingCompletionList = new VSInternalCompletionList();
-        var originalRequestContext = new object();
+        var originalRequestContext = StrictMock.Of<ICompletionResolveContext>();
 
         // Act
         var resolvedItem = await resolver.ResolveAsync(
@@ -112,8 +111,8 @@ public async Task ResolveAsync_UnknownRequestContext_Noops()
         var optionsMonitor = TestRazorLSPOptionsMonitor.Create();
         var resolver = new DelegatedCompletionItemResolver(_documentContextFactory, _formattingService.GetValue(), optionsMonitor, server);
         var item = new VSInternalCompletionItem();
-        var containingCompletionList = new VSInternalCompletionList() { Items = new[] { item, } };
-        var originalRequestContext = new object();
+        var containingCompletionList = new VSInternalCompletionList() { Items = [item] };
+        var originalRequestContext = StrictMock.Of<ICompletionResolveContext>();
 
         // Act
         var resolvedItem = await resolver.ResolveAsync(
@@ -135,7 +134,7 @@ public async Task ResolveAsync_UsesItemsData()
         {
             Data = expectedData,
         };
-        var containingCompletionList = new VSInternalCompletionList() { Items = new[] { item, }, Data = new object() };
+        var containingCompletionList = new VSInternalCompletionList() { Items = [item], Data = new object() };
         var originalRequestContext = new DelegatedCompletionResolutionContext(_csharpCompletionParams, new object());
 
         // Act
@@ -154,7 +153,7 @@ public async Task ResolveAsync_InheritsOriginalCompletionListData()
         var optionsMonitor = TestRazorLSPOptionsMonitor.Create();
         var resolver = new DelegatedCompletionItemResolver(_documentContextFactory, _formattingService.GetValue(), optionsMonitor, server);
         var item = new VSInternalCompletionItem();
-        var containingCompletionList = new VSInternalCompletionList() { Items = new[] { item, }, Data = new object() };
+        var containingCompletionList = new VSInternalCompletionList() { Items = [item], Data = new object() };
         var expectedData = new object();
         var originalRequestContext = new DelegatedCompletionResolutionContext(_csharpCompletionParams, expectedData);
 
@@ -218,7 +217,7 @@ public async Task ResolveAsync_Html_Resolves()
         var optionsMonitor = TestRazorLSPOptionsMonitor.Create();
         var resolver = new DelegatedCompletionItemResolver(_documentContextFactory, _formattingService.GetValue(), optionsMonitor, server);
         var item = new VSInternalCompletionItem();
-        var containingCompletionList = new VSInternalCompletionList() { Items = new[] { item, } };
+        var containingCompletionList = new VSInternalCompletionList() { Items = [item] };
         var originalRequestContext = new DelegatedCompletionResolutionContext(_htmlCompletionParams, new object());
 
         // Act
@@ -267,7 +266,7 @@ private async Task<CSharpTestLspServer> CreateCSharpServerAsync(RazorCodeDocumen
             CompletionProvider = new CompletionOptions
             {
                 ResolveProvider = true,
-                TriggerCharacters = new[] { " ", "(", "=", "#", ".", "<", "[", "{", "\"", "/", ":", "~" }
+                TriggerCharacters = [" ", "(", "=", "#", ".", "<", "[", "{", "\"", "/", ":", "~"]
             }
         };
 
@@ -333,16 +332,11 @@ public static TestDelegatedCompletionItemResolverServer Create(VSInternalComplet
             return provider;
         }
 
-        private class StaticCompletionResolveRequestHandler : CompletionResolveRequestResponseFactory
+        private class StaticCompletionResolveRequestHandler(VSInternalCompletionItem resolveResponse) : CompletionResolveRequestResponseFactory
         {
-            private readonly VSInternalCompletionItem _resolveResponse;
+            private readonly VSInternalCompletionItem _resolveResponse = resolveResponse;
             private DelegatedCompletionItemResolveParams _delegatedParams;
 
-            public StaticCompletionResolveRequestHandler(VSInternalCompletionItem resolveResponse)
-            {
-                _resolveResponse = resolveResponse;
-            }
-
             public override DelegatedCompletionItemResolveParams DelegatedParams => _delegatedParams;
 
             public override Task<object> OnCompletionResolveDelegationAsync(object parameters)
@@ -354,20 +348,14 @@ public override Task<object> OnCompletionResolveDelegationAsync(object parameter
             }
         }
 
-        private class DelegatedCSharpCompletionRequestHandler : CompletionResolveRequestResponseFactory
+        private class DelegatedCSharpCompletionRequestHandler(
+            CSharpTestLspServer csharpServer,
+            CancellationToken cancellationToken) : CompletionResolveRequestResponseFactory
         {
-            private readonly CSharpTestLspServer _csharpServer;
-            private readonly CancellationToken _cancellationToken;
+            private readonly CSharpTestLspServer _csharpServer = csharpServer;
+            private readonly CancellationToken _cancellationToken = cancellationToken;
             private DelegatedCompletionItemResolveParams _delegatedParams;
 
-            public DelegatedCSharpCompletionRequestHandler(
-                CSharpTestLspServer csharpServer,
-                CancellationToken cancellationToken)
-            {
-                _csharpServer = csharpServer;
-                _cancellationToken = cancellationToken;
-            }
-
             public override DelegatedCompletionItemResolveParams DelegatedParams => _delegatedParams;
 
             public override async Task<object> OnCompletionResolveDelegationAsync(object parameters)
diff --git a/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/RazorCompletionItemResolverTest.cs b/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/RazorCompletionItemResolverTest.cs
diff --git a/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/RazorCompletionResolveEndpointTest.cs b/src/Razor/test/Microsoft.AspNetCore.Razor.LanguageServer.Test/Completion/RazorCompletionResolveEndpointTest.cs
diff --git a/src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/Completion/CompletionListCacheTest.cs b/src/Razor/test/Microsoft.CodeAnalysis.Razor.Workspaces.Test/Completion/CompletionListCacheTest.cs

Original file line number	Diff line number	Diff line change
`@@ -1,18 +1,14 @@`
`1`	`1`	`// Copyright (c) .NET Foundation. All rights reserved.`
`2`	`2`	`// Licensed under the MIT license. See License.txt in the project root for license information.`
`3`	`3`
`4`		`-using System;`
	`4`	`+using System.Diagnostics.CodeAnalysis;`
`5`	`5`	`using Microsoft.VisualStudio.LanguageServer.Protocol;`
`6`	`6`
`7`	`7`	`namespace Microsoft.CodeAnalysis.Razor.Completion;`
`8`	`8`
`9`	`9`	`internal class CompletionListCache`
`10`	`10`	`{`
`11`		`- private record struct Slot(`
`12`		`- int Id,`
`13`		`- VSInternalCompletionList CompletionList,`
`14`		`- object? Context,`
`15`		`- bool Used = true);`
	`11`	`+ private record struct Slot(int Id, VSInternalCompletionList CompletionList, ICompletionResolveContext Context);`
`16`	`12`
`17`	`13`	`// Internal for testing`
`18`	`14`	`internal const int MaxCacheSize = 10;`
`@@ -25,13 +21,8 @@ private record struct Slot(`
`25`	`21`	`private int _nextIndex;`
`26`	`22`	`private int _nextId;`
`27`	`23`
`28`		`- public int Add(VSInternalCompletionList completionList, object? context)`
	`24`	`+ public int Add(VSInternalCompletionList completionList, ICompletionResolveContext context)`
`29`	`25`	`{`
`30`		`- if (completionList is null)`
`31`		`- {`
`32`		`- throw new ArgumentNullException(nameof(completionList));`
`33`		`- }`
`34`		`-`
`35`	`26`	`lock (_accessLock)`
`36`	`27`	`{`
`37`	`28`	`var index = _nextIndex++;`
`@@ -52,7 +43,7 @@ public int Add(VSInternalCompletionList completionList, object? context)`
`52`	`43`	`}`
`53`	`44`	`}`
`54`	`45`
`55`		`- public bool TryGet(int id, out (VSInternalCompletionList CompletionList, object? Context) result)`
	`46`	`+ public bool TryGet(int id, [NotNullWhen(true)] out VSInternalCompletionList? completionList, [NotNullWhen(true)] out ICompletionResolveContext? context)`
`56`	`47`	`{`
`57`	`48`	`lock (_accessLock)`
`58`	`49`	`{`
`@@ -74,22 +65,27 @@ public bool TryGet(int id, out (VSInternalCompletionList CompletionList, object?`
`74`	`65`
`75`	`66`	`var slot = _items[index];`
`76`	`67`
`77`		`- if (!slot.Used)`
	`68`	`+ // CompletionList is annotated as non-nullable, but we are allocating an array of 10 items for our cache, so initially`
	`69`	`+ // those array entries will be default. By checking for null here, we detect if we're hitting an unused part of the array`
	`70`	`+ // so stop looping.`
	`71`	`+ if (slot.CompletionList is null)`
`78`	`72`	`{`
`79`	`73`	`break;`
`80`	`74`	`}`
`81`	`75`
`82`	`76`	`if (slot.Id == id)`
`83`	`77`	`{`
`84`		`- result = (slot.CompletionList, slot.Context);`
	`78`	`+ completionList = slot.CompletionList;`
	`79`	`+ context = slot.Context;`
`85`	`80`	`return true;`
`86`	`81`	`}`
`87`	`82`
`88`	`83`	`count--;`
`89`	`84`	`}`
`90`	`85`
`91`	`86`	`// A cache entry associated with the given id was not found.`
`92`		`- result = default;`
	`87`	`+ completionList = null;`
	`88`	`+ context = null;`
`93`	`89`	`return false;`
`94`	`90`	`}`
`95`	`91`	`}`
Original file line number	Diff line number	Diff line change
`@@ -5,4 +5,4 @@`
`5`	`5`
`6`	`6`	`namespace Microsoft.CodeAnalysis.Razor.Completion;`
`7`	`7`
`8`		`-internal record RazorCompletionResolveContext(string FilePath, ImmutableArray<RazorCompletionItem> CompletionItems);`
	`8`	`+internal record RazorCompletionResolveContext(string FilePath, ImmutableArray<RazorCompletionItem> CompletionItems) : ICompletionResolveContext;`