Address code review feedback

src/Features/Core/Portable/UnifiedSuggestions/UnifiedSuggestedActionSet.cs

@@ -17,7 +17,7 @@ namespace Microsoft.CodeAnalysis.UnifiedSuggestions
     /// Similar to SuggestedActionSet, but in a location that can be used
     /// by both local Roslyn and LSP.
     /// </summary>
-    internal class UnifiedSuggestedActionSet : IEquatable<UnifiedSuggestedActionSet>
+    internal class UnifiedSuggestedActionSet
     {
         public string? CategoryName { get; }
 
@@ -42,43 +42,5 @@ internal class UnifiedSuggestedActionSet : IEquatable<UnifiedSuggestedActionSet>
             Priority = priority;
             ApplicableToSpan = applicableToSpan;
         }
-
-        public override bool Equals(object? obj)
-            => obj is UnifiedSuggestedActionSet actionSet && Equals(actionSet);
-
-        public bool Equals(UnifiedSuggestedActionSet? otherSuggestedActionSet)
-        {
-            if (otherSuggestedActionSet == null)
-            {
-                return false;
-            }
-
-            if (this == otherSuggestedActionSet)
-            {
-                return true;
-            }
-
-            if (Title != otherSuggestedActionSet.Title || Priority != otherSuggestedActionSet.Priority ||
-                CategoryName != otherSuggestedActionSet.CategoryName ||
-                ApplicableToSpan != otherSuggestedActionSet.ApplicableToSpan)
-            {
-                return false;
-            }
-
-            if (!Actions.SequenceEqual(otherSuggestedActionSet.Actions))
-            {
-                return false;
-            }
-
-            return true;
-        }
-
-        public override int GetHashCode()
-        {
-            return Hash.Combine(CategoryName != null ? CategoryName.GetHashCode() : 0,
-                Hash.Combine(Actions.GetHashCode(),
-                Hash.Combine(Title != null ? Title.GetHashCode() : 0,
-                Hash.Combine(Priority.GetHashCode(), ApplicableToSpan.GetHashCode()))));
-        }
     }
 }

src/Features/Core/Portable/UnifiedSuggestions/UnifiedSuggestedActions/UnifiedSuggestedAction.cs

@@ -13,7 +13,7 @@ namespace Microsoft.CodeAnalysis.UnifiedSuggestions
     /// Similar to SuggestedAction, but in a location that can be used by
     /// both local Roslyn and LSP.
     /// </summary>
-    internal class UnifiedSuggestedAction : IUnifiedSuggestedAction, IEquatable<IUnifiedSuggestedAction>
+    internal class UnifiedSuggestedAction : IUnifiedSuggestedAction
     {
         public Workspace Workspace { get; }
 
@@ -27,37 +27,5 @@ public UnifiedSuggestedAction(Workspace workspace, CodeAction codeAction, CodeAc
             OriginalCodeAction = codeAction;
             CodeActionPriority = codeActionPriority;
         }
-
-        public bool Equals(IUnifiedSuggestedAction? other)
-            => other is UnifiedSuggestedAction action && Equals(action);
-
-        public override bool Equals(object? obj)
-            => obj is UnifiedSuggestedAction action && Equals(action);
-
-        internal bool Equals(UnifiedSuggestedAction otherSuggestedAction)
-        {
-            if (this == otherSuggestedAction)
-            {
-                return true;
-            }
-
-            if (OriginalCodeAction.EquivalenceKey != null && otherSuggestedAction.OriginalCodeAction.EquivalenceKey != null &&
-                OriginalCodeAction.EquivalenceKey == otherSuggestedAction.OriginalCodeAction.EquivalenceKey)
-            {
-                return true;
-            }
-
-            return OriginalCodeAction.Title == otherSuggestedAction.OriginalCodeAction.Title;
-        }
-
-        public override int GetHashCode()
-        {
-            if (OriginalCodeAction.EquivalenceKey != null)
-            {
-                return OriginalCodeAction.EquivalenceKey.GetHashCode();
-            }
-
-            return OriginalCodeAction.Title.GetHashCode();
-        }
     }
 }

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionHelpers.cs

@@ -46,7 +46,7 @@ internal static class CodeActionHelpers
                 return Array.Empty<VSCodeAction>();
             }
 
-            await codeActionsCache.UpdateCacheAsync(document, request.Range, actionSets.Value, cancellationToken).ConfigureAwait(false);
+            await codeActionsCache.UpdateActionSetsAsync(document, request.Range, actionSets.Value, cancellationToken).ConfigureAwait(false);
 
             var _ = ArrayBuilder<VSCodeAction>.GetInstance(out var codeActions);
             foreach (var set in actionSets)
@@ -115,7 +115,7 @@ internal static class CodeActionHelpers
             IThreadingContext threadingContext,
             CancellationToken cancellationToken)
         {
-            var actionSets = await codeActionsCache.GetCacheAsync(document, selection, cancellationToken).ConfigureAwait(false);
+            var actionSets = await codeActionsCache.GetActionSetsAsync(document, selection, cancellationToken).ConfigureAwait(false);
             if (actionSets == null)
             {
                 actionSets = await GetActionSetsAsync(
@@ -126,7 +126,7 @@ internal static class CodeActionHelpers
                     return ImmutableArray<CodeAction>.Empty;
                 }
 
-                await codeActionsCache.UpdateCacheAsync(document, selection, actionSets.Value, cancellationToken).ConfigureAwait(false);
+                await codeActionsCache.UpdateActionSetsAsync(document, selection, actionSets.Value, cancellationToken).ConfigureAwait(false);
             }
 
             var _ = ArrayBuilder<CodeAction>.GetInstance(out var codeActions);

src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionsCache.cs

@@ -33,7 +33,7 @@ internal class CodeActionsCache
         /// <summary>
         /// Maximum number of cached items.
         /// </summary>
-        private readonly int _maxCacheSize = 3;
+        private const int MaxCacheSize = 3;
 
         /// <summary>
         /// Current list of cached items.
@@ -46,7 +46,7 @@ public CodeActionsCache()
         {
         }
 
-        public async Task UpdateCacheAsync(
+        public async Task UpdateActionSetsAsync(
             Document document,
             LSP.Range range,
             ImmutableArray<UnifiedSuggestedActionSet> cachedSuggestedActionSets,
@@ -55,21 +55,14 @@ public CodeActionsCache()
             using (await _semaphore.DisposableWaitAsync(cancellationToken).ConfigureAwait(false))
             {
                 // If there's a value in the cache with the same document and range we're searching for,
-                // remove and replace it with our updated value if the updated value is different.
-                var previousCachedItem = _cachedItems.Where(
-                    c => c.Document == document && c.Range.Start == range.Start && c.Range.End == range.End);
-                if (previousCachedItem.Any())
+                // remove and replace it with our updated value.
+                var previousCachedItem = _cachedItems.Where(c => IsMatch(document, range, c));
+                if (!previousCachedItem.IsEmpty())
                 {
-                    var item = previousCachedItem.First();
-                    if (item.CachedSuggestedActionSets.SequenceEqual(cachedSuggestedActionSets))
-                    {
-                        return;
-                    }
-
-                    _cachedItems.Remove(item);
+                    _cachedItems.Remove(previousCachedItem.First());
                 }
                 // If the cache is full, remove the oldest cached value.
-                else if (_cachedItems.Count >= _maxCacheSize)
+                else if (_cachedItems.Count >= MaxCacheSize)
                 {
                     _cachedItems.RemoveAt(0);
                 }
@@ -82,7 +75,7 @@ public CodeActionsCache()
         /// Attempts to retrieve the cached action sets that match the given document and range.
         /// Returns null if no match is found.
         /// </summary>
-        public async Task<ImmutableArray<UnifiedSuggestedActionSet>?> GetCacheAsync(
+        public async Task<ImmutableArray<UnifiedSuggestedActionSet>?> GetActionSetsAsync(
             Document document,
             LSP.Range range,
             CancellationToken cancellationToken)
@@ -91,8 +84,7 @@ public CodeActionsCache()
             {
                 foreach (var cachedItem in _cachedItems)
                 {
-                    if (document == cachedItem.Document && document.Project.Solution == cachedItem.Document.Project.Solution &&
-                        range.Start == cachedItem.Range?.Start && range.End == cachedItem.Range?.End)
+                    if (IsMatch(document, range, cachedItem))
                     {
                         return cachedItem.CachedSuggestedActionSets;
                     }
@@ -102,10 +94,11 @@ public CodeActionsCache()
             }
         }
 
-        /// <summary>
-        /// Returns the current number of cached items.
-        /// </summary>
-        public int GetNumCacheItems() => _cachedItems.Count;
+        private static bool IsMatch(Document document, LSP.Range range, CodeActionsCacheItem cachedItem)
+            => document == cachedItem.Document &&
+            document.Project.Solution == cachedItem.Document.Project.Solution &&
+            range.Start == cachedItem.Range.Start &&
+            range.End == cachedItem.Range.End;
 
         /// <summary>
         /// Contains the necessary information for each cached item.
@@ -126,5 +119,21 @@ public CodeActionsCache()
                 CachedSuggestedActionSets = cachedSuggestedActionSets;
             }
         }
+
+        internal TestAccessor GetTestAccessor()
+            => new TestAccessor(this);
+
+        internal readonly struct TestAccessor
+        {
+            private readonly CodeActionsCache _codeActionsCache;
+
+            public static int MaximumCacheSize => MaxCacheSize;
+
+            public TestAccessor(CodeActionsCache codeActionsCache)
+                => _codeActionsCache = codeActionsCache;
+
+            public List<(Document Document, LSP.Range Range)> GetDocumentsAndRangesInCache()
+                => _codeActionsCache._cachedItems.Select(c => (c.Document, c.Range)).ToList();
+        }
     }
 }

src/Features/LanguageServer/ProtocolUnitTests/CodeActions/CodeActionsTests.cs

@@ -3,6 +3,7 @@
 // See the LICENSE file in the project root for more information.
 
 using System;
+using System.Collections.Immutable;
 using System.Linq;
 using System.Threading;
 using System.Threading.Tasks;
@@ -10,6 +11,7 @@
 using Microsoft.CodeAnalysis.Editor.UnitTests.Workspaces;
 using Microsoft.CodeAnalysis.LanguageServer.Handler.CodeActions;
 using Microsoft.CodeAnalysis.Text;
+using Microsoft.CodeAnalysis.UnifiedSuggestions;
 using Microsoft.VisualStudio.LanguageServer.Protocol;
 using Newtonsoft.Json.Linq;
 using Roslyn.Test.Utilities;
@@ -91,50 +93,100 @@ void M()
 }";
             using var workspace = CreateTestWorkspace(markup, out var locations);
             var cache = GetCodeActionsCache(workspace);
+            var testAccessor = cache.GetTestAccessor();
 
-            var caretLocation = locations["caret"].Single();
-            await RunGetCodeActionsAsync(workspace.CurrentSolution, caretLocation);
+            // This test assumes that the maximum cache size is 3, and will have to modified if this number changes.
+            Assert.True(CodeActionsCache.TestAccessor.MaximumCacheSize == 3);
 
+            var caretLocation = locations["caret"].Single();
             var document = GetDocument(workspace, CreateTextDocumentIdentifier(caretLocation.Uri));
-            var cacheResults = await cache.GetCacheAsync(document, caretLocation.Range, CancellationToken.None);
-            Assert.NotNull(cacheResults);
-            Assert.True(cache.GetNumCacheItems() == 1);
 
-            // Invoking code actions on the same unmodified document and range should use the existing cached item
+            // 1. Invoking code actions on document with empty cache.
+            await RunCodeActionsAndAssertActionsInCacheAsync(workspace, cache, caretLocation, document);
+
+            // Ensuring contents of cache are as expected.
+            var docAndRange = testAccessor.GetDocumentsAndRangesInCache().Single();
+            AssertRangeAndDocEqual(caretLocation.Range, document, docAndRange);
+
+            // 2. Invoking code actions on the same unmodified document and range should use the existing cached item
             // instead of generating a new cached item.
-            await RunGetCodeActionsAsync(workspace.CurrentSolution, caretLocation);
-            cacheResults = await cache.GetCacheAsync(document, caretLocation.Range, CancellationToken.None);
-            Assert.True(cache.GetNumCacheItems() == 1);
+            await RunCodeActionsAndAssertActionsInCacheAsync(workspace, cache, caretLocation, document);
+
+            // Ensuring contents of cache are as expected.
+            docAndRange = testAccessor.GetDocumentsAndRangesInCache().Single();
+            AssertRangeAndDocEqual(caretLocation.Range, document, docAndRange);
+
+            var originalRange = caretLocation.Range;
 
-            // Invoking code actions on a different range should generate a new cached item.
+            // 3. Invoking code actions on a different range should generate a new cached item.
             caretLocation.Range = new LSP.Range
             {
                 Start = new LSP.Position() { Line = 0, Character = 0 },
                 End = new LSP.Position() { Line = 0, Character = 0 }
             };
 
-            await RunGetCodeActionsAsync(workspace.CurrentSolution, caretLocation);
-            Assert.True(cache.GetNumCacheItems() == 2);
+            await RunCodeActionsAndAssertActionsInCacheAsync(workspace, cache, caretLocation, document);
+
+            // Ensuring contents of cache are as expected.
+            var docsAndRanges = testAccessor.GetDocumentsAndRangesInCache();
+            Assert.True(docsAndRanges.Count == 2);
+            AssertRangeAndDocEqual(originalRange, document, docsAndRanges[0]);
+            AssertRangeAndDocEqual(caretLocation.Range, document, docsAndRanges[1]);
 
-            // Changing the document should generate a new cached item.
+            // 4. Changing the document should generate a new cached item.
             var currentDocText = await document.GetTextAsync();
             var changedSourceText = currentDocText.WithChanges(new TextChange(new TextSpan(0, 0), "class D { } \n"));
             var docId = ((TestWorkspace)workspace).Documents.First().Id;
             ((TestWorkspace)workspace).ChangeDocument(docId, changedSourceText);
             UpdateSolutionProvider((TestWorkspace)workspace, workspace.CurrentSolution);
+            var updatedDocument = GetDocument(workspace, CreateTextDocumentIdentifier(caretLocation.Uri));
 
-            await RunGetCodeActionsAsync(workspace.CurrentSolution, caretLocation);
-            Assert.True(cache.GetNumCacheItems() == 3);
+            await RunCodeActionsAndAssertActionsInCacheAsync(workspace, cache, caretLocation, updatedDocument);
+
+            // Ensuring contents of cache are as expected.
+            docsAndRanges = testAccessor.GetDocumentsAndRangesInCache();
+            AssertRangeAndDocEqual(originalRange, document, docsAndRanges[0]);
+            AssertRangeAndDocEqual(caretLocation.Range, document, docsAndRanges[1]);
+            AssertRangeAndDocEqual(caretLocation.Range, updatedDocument, docsAndRanges[2]);
+
+            var updatedRange = caretLocation.Range;
 
-            // The current cache size is 3. Adding a 4th item to the cache should still keep the cache size the same.
+            // 5. The current cache size is 3. Adding a 4th item to the cache should still keep the cache size the same,
+            // and boot out the oldest item in the cache.
             caretLocation.Range = new LSP.Range
             {
                 Start = new LSP.Position() { Line = 0, Character = 0 },
                 End = new LSP.Position() { Line = 0, Character = 1 }
             };
 
+            await RunCodeActionsAndAssertActionsInCacheAsync(workspace, cache, caretLocation, updatedDocument);
+
+            // Ensuring contents of cache are as expected.
+            docsAndRanges = testAccessor.GetDocumentsAndRangesInCache();
+            AssertRangeAndDocEqual(updatedRange, document, docsAndRanges[0]);
+            AssertRangeAndDocEqual(updatedRange, updatedDocument, docsAndRanges[1]);
+            AssertRangeAndDocEqual(caretLocation.Range, updatedDocument, docsAndRanges[2]);
+        }
+
+        private static async Task RunCodeActionsAndAssertActionsInCacheAsync(
+            Workspace workspace,
+            CodeActionsCache cache,
+            LSP.Location caretLocation,
+            Document document)
+        {
             await RunGetCodeActionsAsync(workspace.CurrentSolution, caretLocation);
-            Assert.True(cache.GetNumCacheItems() == 3);
+            var cacheResults = await cache.GetActionSetsAsync(document, caretLocation.Range, CancellationToken.None);
+            Assert.NotNull(cacheResults);
+        }
+
+        private static void AssertRangeAndDocEqual(
+            LSP.Range range,
+            Document document,
+            (Document Document, LSP.Range Range) actualDocAndRange)
+        {
+            Assert.Equal(document, actualDocAndRange.Document);
+            Assert.Equal(range.Start, actualDocAndRange.Range.Start);
+            Assert.Equal(range.End, actualDocAndRange.Range.End);
         }
 
         private static async Task<LSP.VSCodeAction[]> RunGetCodeActionsAsync(