diff --git a/src/Features/Core/Portable/UnifiedSuggestions/UnifiedSuggestedActionSet.cs b/src/Features/Core/Portable/UnifiedSuggestions/UnifiedSuggestedActionSet.cs index e6b658ada6e70ce69c6035fb4e0d8b21a4d015ba..7b42d314b1a75f418a9d6f200dfc23d2f0202ef1 100644 --- a/src/Features/Core/Portable/UnifiedSuggestions/UnifiedSuggestedActionSet.cs +++ b/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. /// - internal class UnifiedSuggestedActionSet : IEquatable + internal class UnifiedSuggestedActionSet { public string? CategoryName { get; } @@ -42,43 +42,5 @@ internal class UnifiedSuggestedActionSet : IEquatable 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())))); - } } } diff --git a/src/Features/Core/Portable/UnifiedSuggestions/UnifiedSuggestedActions/UnifiedSuggestedAction.cs b/src/Features/Core/Portable/UnifiedSuggestions/UnifiedSuggestedActions/UnifiedSuggestedAction.cs index 48b382080afdf79bb3a292114413108dac22cc29..4fe7d5765ca452e178f2a0f527e3e4787af3f594 100644 --- a/src/Features/Core/Portable/UnifiedSuggestions/UnifiedSuggestedActions/UnifiedSuggestedAction.cs +++ b/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. /// - internal class UnifiedSuggestedAction : IUnifiedSuggestedAction, IEquatable + 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(); - } } } diff --git a/src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionHelpers.cs b/src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionHelpers.cs index da4eb8adbc24d5bfa5f2350ce4a17a598594551a..54de6cacf667ce42ba7f180a12d4cc2c74b95e86 100644 --- a/src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionHelpers.cs +++ b/src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionHelpers.cs @@ -46,7 +46,7 @@ internal static class CodeActionHelpers return Array.Empty(); } - await codeActionsCache.UpdateCacheAsync(document, request.Range, actionSets.Value, cancellationToken).ConfigureAwait(false); + await codeActionsCache.UpdateActionSetsAsync(document, request.Range, actionSets.Value, cancellationToken).ConfigureAwait(false); var _ = ArrayBuilder.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.Empty; } - await codeActionsCache.UpdateCacheAsync(document, selection, actionSets.Value, cancellationToken).ConfigureAwait(false); + await codeActionsCache.UpdateActionSetsAsync(document, selection, actionSets.Value, cancellationToken).ConfigureAwait(false); } var _ = ArrayBuilder.GetInstance(out var codeActions); diff --git a/src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionsCache.cs b/src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionsCache.cs index 2cab835880036395138475a83a4b56364ea08a1d..30810bec998eac90754d10c11931f9141c891bb1 100644 --- a/src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionsCache.cs +++ b/src/Features/LanguageServer/Protocol/Handler/CodeActions/CodeActionsCache.cs @@ -33,7 +33,7 @@ internal class CodeActionsCache /// /// Maximum number of cached items. /// - private readonly int _maxCacheSize = 3; + private const int MaxCacheSize = 3; /// /// 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 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. /// - public async Task?> GetCacheAsync( + public async Task?> 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() } } - /// - /// Returns the current number of cached items. - /// - 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; /// /// 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(); + } } } diff --git a/src/Features/LanguageServer/ProtocolUnitTests/CodeActions/CodeActionsTests.cs b/src/Features/LanguageServer/ProtocolUnitTests/CodeActions/CodeActionsTests.cs index fa0391246ac79817c5de58a45cf44573c65eb851..cf07c26c3a9309b66cf0126642feb5f9be6f3401 100644 --- a/src/Features/LanguageServer/ProtocolUnitTests/CodeActions/CodeActionsTests.cs +++ b/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 RunGetCodeActionsAsync(