Merge pull request #52123 from dotnet/nimullen/razor.30232.resolve

Changed LSP completion resolve to not cache data on items.

src/EditorFeatures/TestUtilities/LanguageServer/AbstractLanguageServerProtocolTests.cs

@@ -246,8 +246,7 @@ protected static LSP.CompletionParams CreateCompletionParams(
             string? insertText = null,
             string? sortText = null,
             string? filterText = null,
-            string? displayText = null,
-            int resultId = 0)
+            long resultId = 0)
         {
             var position = await document.GetPositionFromLinePositionAsync(
                 ProtocolConversions.PositionToLinePosition(request.Position), CancellationToken.None).ConfigureAwait(false);
@@ -265,10 +264,6 @@ protected static LSP.CompletionParams CreateCompletionParams(
                 Kind = kind,
                 Data = JObject.FromObject(new CompletionResolveData()
                 {
-                    DisplayText = displayText ?? label,
-                    TextDocument = request.TextDocument,
-                    Position = request.Position,
-                    CompletionTrigger = completionTrigger,
                     ResultId = resultId,
                 }),
                 Preselect = preselect

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

@@ -81,7 +81,7 @@ public CompletionHandler(
             var completionTrigger = await ProtocolConversions.LSPToRoslynCompletionTriggerAsync(request.Context, document, position, cancellationToken).ConfigureAwait(false);
 
             var list = await completionService.GetCompletionsAsync(document, position, completionTrigger, options: completionOptions, cancellationToken: cancellationToken).ConfigureAwait(false);
-            if (list == null || list.Items.IsEmpty)
+            if (list == null || list.Items.IsEmpty || cancellationToken.IsCancellationRequested)
             {
                 return null;
             }
@@ -91,7 +91,7 @@ public CompletionHandler(
             var commitCharactersRuleCache = new Dictionary<ImmutableArray<CharacterSetModificationRule>, ImmutableArray<string>>();
 
             // Cache the completion list so we can avoid recomputation in the resolve handler
-            var resultId = await _completionListCache.UpdateCacheAsync(list, cancellationToken).ConfigureAwait(false);
+            var resultId = _completionListCache.UpdateCache(request.TextDocument, list);
 
             // Feature flag to enable the return of TextEdits instead of InsertTexts (will increase payload size).
             // Flag is defined in VisualStudio\Core\Def\PackageRegistration.pkgdef.
@@ -225,10 +225,6 @@ static async Task<TCompletionItem> CreateCompletionItemAsync<TCompletionItem>(
                     Kind = GetCompletionKind(item.Tags),
                     Data = new CompletionResolveData
                     {
-                        TextDocument = request.TextDocument,
-                        Position = request.Position,
-                        DisplayText = item.DisplayText,
-                        CompletionTrigger = completionTrigger,
                         ResultId = resultId,
                     },
                     Preselect = item.Rules.SelectionBehavior == CompletionItemSelectionBehavior.HardSelection,

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

@@ -3,10 +3,8 @@
 // See the LICENSE file in the project root for more information.
 
 using System.Collections.Generic;
-using System.Threading;
-using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.Completion;
-using Roslyn.Utilities;
+using LSP = Microsoft.VisualStudio.LanguageServer.Protocol;
 
 namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Completion
 {
@@ -23,12 +21,12 @@ internal class CompletionListCache
 
         /// <summary>
         /// Multiple cache requests or updates may be received concurrently.
-        /// We need this sempahore to ensure that we aren't making concurrent
+        /// We need this lock to ensure that we aren't making concurrent
         /// modifications to _nextResultId or _resultIdToCompletionList.
         /// </summary>
-        private readonly SemaphoreSlim _semaphore = new(1);
+        private readonly object _accessLock = new object();
 
-        #region protected by _semaphore
+        #region protected by _accessLock
         /// <summary>
         /// The next resultId available to use.
         /// </summary>
@@ -38,23 +36,19 @@ internal class CompletionListCache
         /// Keeps track of the resultIds in the cache and their associated
         /// completion list.
         /// </summary>
-        private readonly List<(long, CompletionList)> _resultIdToCompletionList = new();
+        private readonly List<CacheEntry> _resultIdToCompletionList = new();
         #endregion
 
-        public CompletionListCache()
-        {
-        }
-
         /// <summary>
         /// Adds a completion list to the cache. If the cache reaches its maximum size, the oldest completion
         /// list in the cache is removed.
         /// </summary>
         /// <returns>
         /// The generated resultId associated with the passed in completion list.
         /// </returns>
-        public async Task<long> UpdateCacheAsync(CompletionList completionList, CancellationToken cancellationToken)
+        public long UpdateCache(LSP.TextDocumentIdentifier textDocument, CompletionList completionList)
         {
-            using (await _semaphore.DisposableWaitAsync(cancellationToken).ConfigureAwait(false))
+            lock (_accessLock)
             {
                 // If cache exceeds maximum size, remove the oldest list in the cache
                 if (_resultIdToCompletionList.Count >= MaxCacheSize)
@@ -66,7 +60,8 @@ public async Task<long> UpdateCacheAsync(CompletionList completionList, Cancella
                 var resultId = _nextResultId++;
 
                 // Add passed in completion list to cache
-                _resultIdToCompletionList.Add((resultId, completionList));
+                var cacheEntry = new CacheEntry(resultId, textDocument, completionList);
+                _resultIdToCompletionList.Add(cacheEntry);
 
                 // Return generated resultId so completion list can later be retrieved from cache
                 return resultId;
@@ -77,16 +72,16 @@ public async Task<long> UpdateCacheAsync(CompletionList completionList, Cancella
         /// Attempts to return the completion list in the cache associated with the given resultId.
         /// Returns null if no match is found.
         /// </summary>
-        public async Task<CompletionList?> GetCachedCompletionListAsync(long resultId, CancellationToken cancellationToken)
+        public CacheEntry? GetCachedCompletionList(long resultId)
         {
-            using (await _semaphore.DisposableWaitAsync(cancellationToken).ConfigureAwait(false))
+            lock (_accessLock)
             {
-                foreach (var (cachedResultId, cachedCompletionList) in _resultIdToCompletionList)
+                foreach (var cacheEntry in _resultIdToCompletionList)
                 {
-                    if (cachedResultId == resultId)
+                    if (cacheEntry.ResultId == resultId)
                     {
                         // We found a match - return completion list
-                        return cachedCompletionList;
+                        return cacheEntry;
                     }
                 }
 
@@ -106,8 +101,10 @@ internal readonly struct TestAccessor
             public TestAccessor(CompletionListCache completionListCache)
                 => _completionListCache = completionListCache;
 
-            public List<(long, CompletionList)> GetCacheContents()
+            public List<CacheEntry> GetCacheContents()
                 => _completionListCache._resultIdToCompletionList;
         }
+
+        public record CacheEntry(long ResultId, LSP.TextDocumentIdentifier TextDocument, CompletionList CompletionList);
     }
 }

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

@@ -2,11 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
-#nullable disable
-
-using Microsoft.CodeAnalysis.Completion;
 using Microsoft.CodeAnalysis.LanguageServer.Handler.Completion;
-using Microsoft.VisualStudio.LanguageServer.Protocol;
 using LSP = Microsoft.VisualStudio.LanguageServer.Protocol;
 
 namespace Microsoft.CodeAnalysis.LanguageServer.Handler
@@ -17,14 +13,6 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler
     /// </summary>
     internal class CompletionResolveData
     {
-        public TextDocumentIdentifier TextDocument { get; set; }
-
-        public Position Position { get; set; }
-
-        public string DisplayText { get; set; }
-
-        public CompletionTrigger CompletionTrigger { get; set; }
-
         /// <summary>
         /// ID associated with the item's completion list.
         /// </summary>

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

@@ -2,10 +2,12 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 // See the LICENSE file in the project root for more information.
 
+using System;
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
 using Microsoft.CodeAnalysis.Completion;
+using Microsoft.CodeAnalysis.Internal.Log;
 using Microsoft.CodeAnalysis.LanguageServer.Handler.Completion;
 using Microsoft.VisualStudio.Text.Adornments;
 using Newtonsoft.Json.Linq;
@@ -31,54 +33,31 @@ public CompletionResolveHandler(CompletionListCache completionListCache)
             _completionListCache = completionListCache;
         }
 
-        private static CompletionResolveData GetCompletionResolveData(LSP.CompletionItem request)
-        {
-            Contract.ThrowIfNull(request.Data);
-
-            return ((JToken)request.Data).ToObject<CompletionResolveData>();
-        }
-
         public LSP.TextDocumentIdentifier? GetTextDocumentIdentifier(LSP.CompletionItem request)
-            => GetCompletionResolveData(request).TextDocument;
+            => GetCompletionListCacheEntry(request)?.TextDocument;
 
         public async Task<LSP.CompletionItem> HandleRequestAsync(LSP.CompletionItem completionItem, RequestContext context, CancellationToken cancellationToken)
         {
             var document = context.Document;
             if (document == null)
             {
+                context.TraceInformation("No associated document found for the provided completion item.");
                 return completionItem;
             }
 
             var completionService = document.Project.LanguageServices.GetRequiredService<CompletionService>();
-            var data = GetCompletionResolveData(completionItem);
-
-            CompletionList? list = null;
-
-            // See if we have a cache of the completion list we need
-            if (data.ResultId.HasValue)
+            var cacheEntry = GetCompletionListCacheEntry(completionItem);
+            if (cacheEntry == null)
             {
-                list = await _completionListCache.GetCachedCompletionListAsync(data.ResultId.Value, cancellationToken).ConfigureAwait(false);
+                // Don't have a cache associated with this completion item, cannot resolve.
+                context.TraceInformation("No cache entry found for the provided completion item at resolve time.");
+                return completionItem;
             }
 
-            if (list == null)
-            {
-                // We don't have a cache, so we need to recompute the list
-                var position = await document.GetPositionFromLinePositionAsync(ProtocolConversions.PositionToLinePosition(data.Position), cancellationToken).ConfigureAwait(false);
-                var completionOptions = await CompletionHandler.GetCompletionOptionsAsync(document, cancellationToken).ConfigureAwait(false);
-
-                list = await completionService.GetCompletionsAsync(document, position, data.CompletionTrigger, options: completionOptions, cancellationToken: cancellationToken).ConfigureAwait(false);
-                if (list == null)
-                {
-                    return completionItem;
-                }
-            }
+            var list = cacheEntry.CompletionList;
 
             // Find the matching completion item in the completion list
-            var selectedItem = list.Items.FirstOrDefault(
-                i => data.DisplayText == i.DisplayText &&
-                    completionItem.Label.StartsWith(i.DisplayTextPrefix) &&
-                    completionItem.Label.EndsWith(i.DisplayTextSuffix));
-
+            var selectedItem = list.Items.FirstOrDefault(cachedCompletionItem => MatchesLSPCompletionItem(completionItem, cachedCompletionItem));
             if (selectedItem == null)
             {
                 return completionItem;
@@ -112,6 +91,27 @@ private static CompletionResolveData GetCompletionResolveData(LSP.CompletionItem
             return completionItem;
         }
 
+        private static bool MatchesLSPCompletionItem(LSP.CompletionItem lspCompletionItem, CompletionItem completionItem)
+        {
+            if (!lspCompletionItem.Label.StartsWith(completionItem.DisplayTextPrefix, StringComparison.Ordinal))
+            {
+                return false;
+            }
+
+            if (!lspCompletionItem.Label.EndsWith(completionItem.DisplayTextSuffix, StringComparison.Ordinal))
+            {
+                return false;
+            }
+
+            if (string.Compare(lspCompletionItem.Label, completionItem.DisplayTextPrefix.Length, completionItem.DisplayText, 0, completionItem.DisplayText.Length, StringComparison.Ordinal) != 0)
+            {
+                return false;
+            }
+
+            // All parts of the LSP completion item match the provided completion item.
+            return true;
+        }
+
         // Internal for testing
         internal static async Task<LSP.TextEdit> GenerateTextEditAsync(
             Document document,
@@ -157,5 +157,25 @@ private static CompletionResolveData GetCompletionResolveData(LSP.CompletionItem
 
             return textEdit;
         }
+
+        private CompletionListCache.CacheEntry? GetCompletionListCacheEntry(LSP.CompletionItem request)
+        {
+            Contract.ThrowIfNull(request.Data);
+            var resolveData = ((JToken)request.Data).ToObject<CompletionResolveData>();
+            if (resolveData?.ResultId == null)
+            {
+                Contract.Fail("Result id should always be provided when resolving a completion item we returned.");
+                return null;
+            }
+
+            var cacheEntry = _completionListCache.GetCachedCompletionList(resolveData.ResultId.Value);
+            if (cacheEntry == null)
+            {
+                // No cache for associated completion item. Log some telemetry so we can understand how frequently this actually happens.
+                Logger.Log(FunctionId.LSP_CompletionListCacheMiss, KeyValueLogMessage.NoProperty);
+            }
+
+            return cacheEntry;
+        }
     }
 }

src/Features/LanguageServer/Protocol/IsExternalInit.cs

@@ -0,0 +1,11 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+namespace System.Runtime.CompilerServices
+{
+    // Used to compile against C# 9 in a netstandard2.0
+    internal class IsExternalInit
+    {
+    }
+}