From c0d24b0450f40eb35a8e33f50456566292d79141 Mon Sep 17 00:00:00 2001 From: David Barbet Date: Fri, 18 Oct 2024 15:14:40 -0700 Subject: [PATCH 01/10] Reuse resultid when diagnostics have not actually changed --- .../AbstractPullDiagnosticHandler.cs | 40 +++-- .../Diagnostics/DiagnosticsPullCache.cs | 44 ++++++ .../VersionedPullCache.CacheItem.cs | 139 ++++++++++++++++++ .../PullHandlers/VersionedPullCache.cs | 100 +++++++++++++ .../PullHandlers/VersionedPullCache`1.cs | 41 ------ .../PullHandlers/VersionedPullCache`2.cs | 134 ----------------- .../AbstractSpellCheckingHandler.cs | 30 ++-- .../Handler/SpellCheck/SpellCheckPullCache.cs | 42 ++++++ .../Diagnostics/PullDiagnosticTests.cs | 6 +- 9 files changed, 363 insertions(+), 213 deletions(-) create mode 100644 src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs create mode 100644 src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs create mode 100644 src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs delete mode 100644 src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache`1.cs delete mode 100644 src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache`2.cs create mode 100644 src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs diff --git a/src/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs b/src/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs index 6336c64247162..e455c29e65eb3 100644 --- a/src/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs +++ b/src/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs @@ -2,12 +2,16 @@ // 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.Collections.Concurrent; using System.Collections.Generic; using System.Collections.Immutable; +using System.Diagnostics; using System.Diagnostics.CodeAnalysis; +using System.Linq; using System.Threading; using System.Threading.Tasks; +using Microsoft.CodeAnalysis.CodeCleanup; using Microsoft.CodeAnalysis.Diagnostics; using Microsoft.CodeAnalysis.Options; using Microsoft.CodeAnalysis.PooledObjects; @@ -45,13 +49,11 @@ internal abstract partial class AbstractPullDiagnosticHandler - /// Cache where we store the data produced by prior requests so that they can be returned if nothing of significance - /// changed. The is produced by while the - /// is produced by . The former is faster - /// and works well for us in the normal case. The latter still allows us to reuse diagnostics when changes happen that - /// update the version stamp but not the content (for example, forking LSP text). + /// Map of diagnostic category to the diagnostics cache for that category. + /// Each category has a separate cache as they have disjoint resultIds and diagnostics. For example, we may have + /// one cache for DocumentSyntax, another for DocumentSemantic, another for WorkspaceSemantic, etc etc. /// - private readonly ConcurrentDictionary> _categoryToVersionedCache = []; + private readonly ConcurrentDictionary _categoryToVersionedCache = []; protected virtual bool PotentialDuplicate => false; @@ -156,18 +158,18 @@ protected virtual Task WaitForChangesAsync(string? category, RequestContext cont var globalStateVersion = _diagnosticRefresher.GlobalStateVersion; var project = diagnosticSource.GetProject(); + var cacheState = new DiagnosticsRequestState(project, globalStateVersion, context, diagnosticSource); - var newResultId = await versionedCache.GetNewResultIdAsync( + var newResult = await versionedCache.GetNewResultIdAsync( documentIdToPreviousDiagnosticParams, diagnosticSource.GetId(), project, - computeCheapVersionAsync: async () => (globalStateVersion, await project.GetDependentVersionAsync(cancellationToken).ConfigureAwait(false)), - computeExpensiveVersionAsync: async () => (globalStateVersion, await project.GetDependentChecksumAsync(cancellationToken).ConfigureAwait(false)), + cacheState, cancellationToken).ConfigureAwait(false); - if (newResultId != null) + if (newResult != null) { - await ComputeAndReportCurrentDiagnosticsAsync( - context, diagnosticSource, progress, newResultId, clientCapabilities, cancellationToken).ConfigureAwait(false); + ReportCurrentDiagnostics( + diagnosticSource, newResult.Value.Data, progress, newResult.Value.ResultId, clientCapabilities); } else { @@ -269,16 +271,14 @@ static async Task ProcessPreviousResultsAsync( } } - private async Task ComputeAndReportCurrentDiagnosticsAsync( - RequestContext context, + private void ReportCurrentDiagnostics( IDiagnosticSource diagnosticSource, + ImmutableArray diagnostics, BufferedProgress progress, - string resultId, - ClientCapabilities clientCapabilities, - CancellationToken cancellationToken) + string newResultId, + ClientCapabilities clientCapabilities) { using var _ = ArrayBuilder.GetInstance(out var result); - var diagnostics = await diagnosticSource.GetDiagnosticsAsync(context, cancellationToken).ConfigureAwait(false); // If we can't get a text document identifier we can't report diagnostics for this source. // This can happen for 'fake' projects (e.g. used for TS script blocks). @@ -290,12 +290,10 @@ private async Task ComputeAndReportCurrentDiagnosticsAsync( return; } - context.TraceInformation($"Found {diagnostics.Length} diagnostics for {diagnosticSource.ToDisplayString()}"); - foreach (var diagnostic in diagnostics) result.AddRange(ConvertDiagnostic(diagnosticSource, diagnostic, clientCapabilities)); - var report = CreateReport(documentIdentifier, result.ToArray(), resultId); + var report = CreateReport(documentIdentifier, result.ToArray(), newResultId); progress.Report(report); } diff --git a/src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs b/src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs new file mode 100644 index 0000000000000..d8848c9b5f1c1 --- /dev/null +++ b/src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs @@ -0,0 +1,44 @@ +// 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. + +using System.Collections.Immutable; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Diagnostics; +using Roslyn.LanguageServer.Protocol; +using Roslyn.Utilities; + +namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; + +internal abstract partial class AbstractPullDiagnosticHandler where TDiagnosticsParams : IPartialResultParams +{ + internal record struct DiagnosticsRequestState(Project Project, int GlobalStateVersion, RequestContext Context, IDiagnosticSource DiagnosticSource); + + /// + /// Cache where we store the data produced by prior requests so that they can be returned if nothing of significance + /// changed. The is produced by while the + /// is produced by . The former is faster + /// and works well for us in the normal case. The latter still allows us to reuse diagnostics when changes happen that + /// update the version stamp but not the content (for example, forking LSP text). + /// + internal class DiagnosticsPullCache(string uniqueKey) : VersionedPullCache<(int globalStateVersion, VersionStamp? dependentVersion), (int globalStateVersion, Checksum dependentChecksum), DiagnosticsRequestState, DiagnosticData>(uniqueKey) + { + public override async Task<(int globalStateVersion, VersionStamp? dependentVersion)> ComputeCheapVersionAsync(DiagnosticsRequestState state, CancellationToken cancellationToken) + { + return (state.GlobalStateVersion, await state.Project.GetDependentVersionAsync(cancellationToken).ConfigureAwait(false)); + } + + public override async Task<(int globalStateVersion, Checksum dependentChecksum)> ComputeExpensiveVersionAsync(DiagnosticsRequestState state, CancellationToken cancellationToken) + { + return (state.GlobalStateVersion, await state.Project.GetDependentChecksumAsync(cancellationToken).ConfigureAwait(false)); + } + + public override async Task> ComputeDataAsync(DiagnosticsRequestState state, CancellationToken cancellationToken) + { + var diagnostics = await state.DiagnosticSource.GetDiagnosticsAsync(state.Context, cancellationToken).ConfigureAwait(false); + state.Context.TraceInformation($"Found {diagnostics.Length} diagnostics for {state.DiagnosticSource.ToDisplayString()}"); + return diagnostics; + } + } +} diff --git a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs new file mode 100644 index 0000000000000..1fea6ebcfd1f8 --- /dev/null +++ b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs @@ -0,0 +1,139 @@ +// 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. + +using System.Collections.Immutable; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; +using Roslyn.Utilities; + +namespace Microsoft.CodeAnalysis.LanguageServer.Handler; + +internal abstract partial class VersionedPullCache +{ + /// + /// Internal cache item that updates state for a particular and in + /// This type ensures that the state for a particular key is never updated concurrently for the same key (but different key states can be concurrent). + /// + private class CacheItem(string uniqueKey) + { + /// + /// Guards access to . + /// This ensures that a cache entry is fully updated in a single transaction. + /// + private readonly SemaphoreSlim _semaphore = new(1); + + /// + /// Stores the current state associated with this cache item. + /// + /// + /// The resultId reported to the client. + /// The TCheapVersion of the data that was used to calculate results. + /// + /// Note that this version can change even when nothing has actually changed (for example, forking the + /// LSP text, reloading the same project). So we additionally store: + /// A TExpensiveVersion (normally a checksum) checksum that will still allow us to reuse data even when + /// unimportant changes happen that trigger the cheap version change detection. + /// The hash of the data that was computed when the resultId was generated. + /// + /// When the versions above change, we must recalculate the data. However sometimes that data ends up being exactly the same as the prior request. + /// When that happens, this allows us to send back an unchanged result instead of reserializing data the client already has. + /// + /// + /// + /// + /// + private (string resultId, TCheapVersion cheapVersion, TExpensiveVersion expensiveVersion, int hashedData)? _lastResult; + + /// + /// Updates the values for this cache entry. Guarded by + /// + /// Returns null if the previousPullResult can be re-used, otherwise returns a new resultId and the new data associated with it. + /// + public async Task<(string, ImmutableArray)?> UpdateCacheItemAsync( + VersionedPullCache cache, + PreviousPullResult? previousPullResult, + bool isFullyLoaded, + TState state, + CancellationToken cancellationToken) + { + // Ensure that we only update the cache item one at a time. + // This means that the computation of new data for this item only occurs sequentially. + using (await _semaphore.DisposableWaitAsync(cancellationToken).ConfigureAwait(false)) + { + TCheapVersion cheapVersion; + TExpensiveVersion expensiveVersion; + + // Check if the version we have in the cache matches the request version. If so we can re-use the resultId. + if (isFullyLoaded && + _lastResult is not null && + _lastResult.Value.resultId == previousPullResult?.PreviousResultId) + { + cheapVersion = await cache.ComputeCheapVersionAsync(state, cancellationToken).ConfigureAwait(false); + if (cheapVersion != null && cheapVersion.Equals(_lastResult.Value.cheapVersion)) + { + // The client's resultId matches our cached resultId and the cheap version is an + // exact match for our current cheap version. We return early here to avoid calculating + // expensive versions as we know nothing is changed. + return null; + } + + // The current cheap version does not match the last reported. This may be because we've forked + // or reloaded a project, so fall back to calculating the full expensive version to determine if + // anything is actually changed. + expensiveVersion = await cache.ComputeExpensiveVersionAsync(state, cancellationToken).ConfigureAwait(false); + if (expensiveVersion != null && expensiveVersion.Equals(_lastResult.Value.expensiveVersion)) + { + return null; + } + } + else + { + // The versions we have in our cache (if any) do not match the ones provided by the client (if any). + // We need to calculate new results. + cheapVersion = await cache.ComputeCheapVersionAsync(state, cancellationToken).ConfigureAwait(false); + expensiveVersion = await cache.ComputeExpensiveVersionAsync(state, cancellationToken).ConfigureAwait(false); + } + + // Compute the new result for the request. + var data = await cache.ComputeDataAsync(state, cancellationToken).ConfigureAwait(false); + var dataHash = GetComputedDataHash(data); + + string newResultId; + if (_lastResult is not null && _lastResult?.resultId == previousPullResult?.PreviousResultId && _lastResult?.hashedData == dataHash) + { + // The new data we've computed is exactly the same as the data we computed last time even though the versions have changed. + // Instead of reserializing everything, we can return the same result id back to the client. + + // Ensure we store the updated versions we calculated with the old resultId. + _lastResult = (_lastResult.Value.resultId, cheapVersion, expensiveVersion, dataHash); + return null; + } + else + { + // Keep track of the results we reported here so that we can short-circuit producing results for + // the same state of the world in the future. Use a custom result-id per type (doc requests or workspace + // requests) so that clients of one don't errantly call into the other. + // + // For example, a client getting document diagnostics should not ask for workspace diagnostics with the result-ids it got for + // doc-diagnostics. The two systems are different and cannot share results, or do things like report + // what changed between each other. + // + // Note that we can safely update the map before computation as any cancellation or exception + // during computation means that the client will never recieve this resultId and so cannot ask us for it. + newResultId = $"{uniqueKey}:{cache.IncrementResultId()}"; + _lastResult = (newResultId, cheapVersion, expensiveVersion, dataHash); + return (newResultId, data); + } + } + } + + private static int GetComputedDataHash(ImmutableArray data) + { + var hashes = data.SelectAsArray(d => d.GetHashCode()).Sort(); + return Hash.CombineValues(hashes); + } + } +} + diff --git a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs new file mode 100644 index 0000000000000..2a8dad4907ebe --- /dev/null +++ b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs @@ -0,0 +1,100 @@ +// 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. + +using System.Collections.Concurrent; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Host; +using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; +using Roslyn.Utilities; + +namespace Microsoft.CodeAnalysis.LanguageServer.Handler +{ + /// + /// Specialized cache used by the 'pull' LSP handlers. Supports storing data to know when to tell a client + /// that existing results can be reused, or if new results need to be computed. Multiple keys can be used, + /// with different computation costs to determine if the previous cached data is still valid. + /// + internal abstract partial class VersionedPullCache(string uniqueKey) where TComputedData : notnull + { + + /// + /// Map of workspace and diagnostic source to the data used to make the last pull report. + /// This is a concurrent dictionary as parallel access is allowed for different workspace+project/doc combinations. + /// + /// The itself however will internally guarantee that the state for a specific workspace+project/doc will only + /// be updated sequentially. + /// + private readonly ConcurrentDictionary<(Workspace workspace, ProjectOrDocumentId id), CacheItem> _idToLastReportedResult = []; + + /// + /// The next available id to label results with. Note that results are tagged on a per-document bases. That + /// way we can update results with the client with per-doc granularity. + /// + /// Called by with Interlocked access to ensure that all cache items generate unique resultIds. + /// + private long _nextDocumentResultId; + + /// + /// Computes a cheap version of the current state. This is compared to the cached version we calculated for the client's previous resultId. + /// + /// Note - this will run under the semaphore in . + /// + public abstract Task ComputeCheapVersionAsync(TState state, CancellationToken cancellationToken); + + /// + /// Computes a more expensive version of the current state. If the cheap versions are mismatched, we then compare the expensive version of the current state against the + /// expensive version we have cached for the client's previous resultId. + /// + /// Note - this will run under the semaphore in . + /// + public abstract Task ComputeExpensiveVersionAsync(TState state, CancellationToken cancellationToken); + + /// + /// Computes new data for this request. This data must be hashable as it we store the hash with the requestId to determine if + /// the data has changed between requests. + /// + /// Note - this will run under the semaphore in . + /// + public abstract Task> ComputeDataAsync(TState state, CancellationToken cancellationToken); + + /// + /// If results have changed since the last request this calculates and returns a new + /// non-null resultId to use for subsequent computation and caches it. + /// + /// a map of roslyn document or project id to the previous result the client sent us for that doc. + /// the id of the project or document that we are checking to see if it has changed. + /// Null when results are unchanged, otherwise returns a non-null new resultId. + public async Task<(string ResultId, ImmutableArray Data)?> GetNewResultIdAsync( + Dictionary idToClientLastResult, + ProjectOrDocumentId projectOrDocumentId, + Project project, + TState state, + CancellationToken cancellationToken) + { + var workspace = project.Solution.Workspace; + + // We have to make sure we've been fully loaded before using cached results as the previous results may not be complete. + var isFullyLoaded = await IsFullyLoadedAsync(project.Solution, cancellationToken).ConfigureAwait(false); + var previousResult = IDictionaryExtensions.GetValueOrDefault(idToClientLastResult, projectOrDocumentId); + + var cacheEntry = _idToLastReportedResult.GetOrAdd((project.Solution.Workspace, projectOrDocumentId), (_) => new CacheItem(uniqueKey)); + return await cacheEntry.UpdateCacheItemAsync(this, previousResult, isFullyLoaded, state, cancellationToken).ConfigureAwait(false); + } + + public long IncrementResultId() + { + return Interlocked.Increment(ref _nextDocumentResultId); + } + + private static async Task IsFullyLoadedAsync(Solution solution, CancellationToken cancellationToken) + { + var workspaceStatusService = solution.Services.GetRequiredService(); + var isFullyLoaded = await workspaceStatusService.IsFullyLoadedAsync(cancellationToken).ConfigureAwait(false); + return isFullyLoaded; + } + } +} diff --git a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache`1.cs b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache`1.cs deleted file mode 100644 index c9ca5e63f60ef..0000000000000 --- a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache`1.cs +++ /dev/null @@ -1,41 +0,0 @@ -// 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. - -using System; -using System.Collections.Generic; -using System.Linq; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; -using Roslyn.Utilities; - -namespace Microsoft.CodeAnalysis.LanguageServer.Handler -{ - /// - /// Simplified version of that only uses a - /// single cheap key to check results against. - /// - internal class VersionedPullCache : VersionedPullCache - { - public VersionedPullCache(string uniqueKey) - : base(uniqueKey) - { - } - - public Task GetNewResultIdAsync( - Dictionary documentToPreviousDiagnosticParams, - Document document, - Func> computeVersionAsync, - CancellationToken cancellationToken) - { - return GetNewResultIdAsync( - documentToPreviousDiagnosticParams.ToDictionary(kvp => new ProjectOrDocumentId(kvp.Key.Id), kvp => kvp.Value), - new ProjectOrDocumentId(document.Id), - document.Project, - computeVersionAsync, - computeExpensiveVersionAsync: SpecializedTasks.Null, - cancellationToken); - } - } -} diff --git a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache`2.cs b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache`2.cs deleted file mode 100644 index 4d07953ad1b3f..0000000000000 --- a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache`2.cs +++ /dev/null @@ -1,134 +0,0 @@ -// 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. - -using System; -using System.Collections.Generic; -using System.Threading; -using System.Threading.Tasks; -using Microsoft.CodeAnalysis.Host; -using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; -using Roslyn.Utilities; - -namespace Microsoft.CodeAnalysis.LanguageServer.Handler -{ - /// - /// Specialized cache used by the 'pull' LSP handlers. Supports storing data to know when to tell a client - /// that existing results can be reused, or if new results need to be computed. Multiple keys can be used, - /// with different computation costs to determine if the previous cached data is still valid. - /// - internal class VersionedPullCache - { - private readonly string _uniqueKey; - - /// - /// Lock to protect and . - /// This enables this type to be used by request handlers that process requests concurrently. - /// - private readonly SemaphoreSlim _semaphore = new(1); - - /// - /// Mapping of a diagnostic source to the data used to make the last pull report which contains: - /// - /// The resultId reported to the client. - /// The TCheapVersion of the data that was used to calculate results. - /// - /// Note that this version can change even when nothing has actually changed (for example, forking the - /// LSP text, reloading the same project). So we additionally store: - /// A TExpensiveVersion (normally a checksum) checksum that will still allow us to reuse data even when - /// unimportant changes happen that trigger the cheap version change detection. - /// - /// This is used to determine if we need to re-calculate results. - /// - private readonly Dictionary<(Workspace workspace, ProjectOrDocumentId id), (string resultId, TCheapVersion cheapVersion, TExpensiveVersion expensiveVersion)> _idToLastReportedResult = []; - - /// - /// The next available id to label results with. Note that results are tagged on a per-document bases. That - /// way we can update results with the client with per-doc granularity. - /// - private long _nextDocumentResultId; - - public VersionedPullCache(string uniqueKey) - { - _uniqueKey = uniqueKey; - } - - /// - /// If results have changed since the last request this calculates and returns a new - /// non-null resultId to use for subsequent computation and caches it. - /// - /// a map of roslyn document or project id to the previous result the client sent us for that doc. - /// the id of the project or document that we are checking to see if it has changed. - /// Null when results are unchanged, otherwise returns a non-null new resultId. - public async Task GetNewResultIdAsync( - Dictionary idToClientLastResult, - ProjectOrDocumentId projectOrDocumentId, - Project project, - Func> computeCheapVersionAsync, - Func> computeExpensiveVersionAsync, - CancellationToken cancellationToken) - { - TCheapVersion cheapVersion; - TExpensiveVersion expensiveVersion; - - var workspace = project.Solution.Workspace; - - // We have to make sure we've been fully loaded before using cached results as the previous results may not be complete. - var isFullyLoaded = await IsFullyLoadedAsync(project.Solution, cancellationToken).ConfigureAwait(false); - using (await _semaphore.DisposableWaitAsync(cancellationToken).ConfigureAwait(false)) - { - if (isFullyLoaded && idToClientLastResult.TryGetValue(projectOrDocumentId, out var previousResult) && - previousResult.PreviousResultId != null && - _idToLastReportedResult.TryGetValue((workspace, projectOrDocumentId), out var lastResult) && - lastResult.resultId == previousResult.PreviousResultId) - { - cheapVersion = await computeCheapVersionAsync().ConfigureAwait(false); - if (cheapVersion != null && cheapVersion.Equals(lastResult.cheapVersion)) - { - // The client's resultId matches our cached resultId and the cheap version is an - // exact match for our current cheap version. We return early here to avoid calculating - // expensive versions as we know nothing is changed. - return null; - } - - // The current cheap version does not match the last reported. This may be because we've forked - // or reloaded a project, so fall back to calculating the full expensive version to determine if - // anything is actually changed. - expensiveVersion = await computeExpensiveVersionAsync().ConfigureAwait(false); - if (expensiveVersion != null && expensiveVersion.Equals(lastResult.expensiveVersion)) - { - return null; - } - } - else - { - // Client didn't give us a resultId or we have nothing cached - // We need to calculate new results and store what we calculated the results for. - cheapVersion = await computeCheapVersionAsync().ConfigureAwait(false); - expensiveVersion = await computeExpensiveVersionAsync().ConfigureAwait(false); - } - - // Keep track of the results we reported here so that we can short-circuit producing results for - // the same state of the world in the future. Use a custom result-id per type (doc requests or workspace - // requests) so that clients of one don't errantly call into the other. - // - // For example, a client getting document diagnostics should not ask for workspace diagnostics with the result-ids it got for - // doc-diagnostics. The two systems are different and cannot share results, or do things like report - // what changed between each other. - // - // Note that we can safely update the map before computation as any cancellation or exception - // during computation means that the client will never recieve this resultId and so cannot ask us for it. - var newResultId = $"{_uniqueKey}:{_nextDocumentResultId++}"; - _idToLastReportedResult[(project.Solution.Workspace, projectOrDocumentId)] = (newResultId, cheapVersion, expensiveVersion); - return newResultId; - } - } - - private static async Task IsFullyLoadedAsync(Solution solution, CancellationToken cancellationToken) - { - var workspaceStatusService = solution.Services.GetRequiredService(); - var isFullyLoaded = await workspaceStatusService.IsFullyLoadedAsync(cancellationToken).ConfigureAwait(false); - return isFullyLoaded; - } - } -} diff --git a/src/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs b/src/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs index 927be98a4b8c7..22a77b83f6c9a 100644 --- a/src/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs +++ b/src/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs @@ -5,9 +5,11 @@ using System; using System.Collections.Generic; using System.Collections.Immutable; +using System.Linq; using System.Runtime.CompilerServices; using System.Threading; using System.Threading.Tasks; +using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; using Microsoft.CodeAnalysis.Serialization; using Microsoft.CodeAnalysis.Shared.Extensions; using Microsoft.CodeAnalysis.SpellCheck; @@ -30,7 +32,7 @@ internal abstract class AbstractSpellCheckHandler /// significance changed. The version key is produced by combining the checksums for project options and /// - private readonly VersionedPullCache<(Checksum parseOptionsChecksum, Checksum textChecksum)?> _versionedCache; + private readonly SpellCheckPullCache _versionedCache; public bool MutatesSolutionState => false; public bool RequiresLSPSolution => true; @@ -97,16 +99,19 @@ protected AbstractSpellCheckHandler() continue; } - var newResultId = await _versionedCache.GetNewResultIdAsync( - documentToPreviousParams, - document, - computeVersionAsync: async () => await ComputeChecksumsAsync(document, cancellationToken).ConfigureAwait(false), + var documentToPreviousDiagnosticParams = documentToPreviousParams.ToDictionary(kvp => new ProjectOrDocumentId(kvp.Key.Id), kvp => kvp.Value); + var newResult = await _versionedCache.GetNewResultIdAsync( + documentToPreviousDiagnosticParams, + new ProjectOrDocumentId(document.Id), + document.Project, + new SpellCheckState(languageService, document), cancellationToken).ConfigureAwait(false); - if (newResultId != null) + if (newResult != null) { + var (newResultId, spans) = newResult.Value; context.TraceInformation($"Spans were changed for document: {document.FilePath}"); - await foreach (var report in ComputeAndReportCurrentSpansAsync( - document, languageService, newResultId, cancellationToken).ConfigureAwait(false)) + foreach (var report in ReportCurrentSpans( + document, spans, newResultId)) { progress.Report(report); } @@ -148,16 +153,13 @@ private static async Task> GetDocumentT return result; } - private async IAsyncEnumerable ComputeAndReportCurrentSpansAsync( + private IEnumerable ReportCurrentSpans( Document document, - ISpellCheckSpanService service, - string resultId, - [EnumeratorCancellation] CancellationToken cancellationToken) + ImmutableArray spans, + string resultId) { var textDocumentIdentifier = ProtocolConversions.DocumentToTextDocumentIdentifier(document); - var spans = await service.GetSpansAsync(document, cancellationToken).ConfigureAwait(false); - // protocol requires the results be in sorted order spans = spans.Sort(static (s1, s2) => s1.TextSpan.CompareTo(s2.TextSpan)); diff --git a/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs b/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs new file mode 100644 index 0000000000000..8529a16381b97 --- /dev/null +++ b/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs @@ -0,0 +1,42 @@ +// 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. + +using System.Collections.Immutable; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.SpellCheck; +using Roslyn.Utilities; + +namespace Microsoft.CodeAnalysis.LanguageServer.Handler.SpellCheck; + +internal record struct SpellCheckState(ISpellCheckSpanService Service, Document Document); + +/// +/// Simplified version of that only uses a +/// single cheap key to check results against. +/// +internal class SpellCheckPullCache(string uniqueKey) : VersionedPullCache<(Checksum parseOptionsChecksum, Checksum textChecksum)?, object?, SpellCheckState, SpellCheckSpan>(uniqueKey) +{ + public override async Task<(Checksum parseOptionsChecksum, Checksum textChecksum)?> ComputeCheapVersionAsync(SpellCheckState state, CancellationToken cancellationToken) + { + var project = state.Document.Project; + var parseOptionsChecksum = project.State.GetParseOptionsChecksum(); + + var documentChecksumState = await state.Document.State.GetStateChecksumsAsync(cancellationToken).ConfigureAwait(false); + var textChecksum = documentChecksumState.Text; + + return (parseOptionsChecksum, textChecksum); + } + + public override async Task> ComputeDataAsync(SpellCheckState state, CancellationToken cancellationToken) + { + var spans = await state.Service.GetSpansAsync(state.Document, cancellationToken).ConfigureAwait(false); + return spans; + } + + public override Task ComputeExpensiveVersionAsync(SpellCheckState state, CancellationToken cancellationToken) + { + return SpecializedTasks.Null(); + } +} diff --git a/src/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs b/src/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs index 6ab5e649f52ed..d691ffc12ebf9 100644 --- a/src/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs @@ -329,9 +329,9 @@ public async Task TestDocumentDiagnosticsWhenGlobalStateChanges(bool useVSDiagno results = await RunGetDocumentPullDiagnosticsAsync( testLspServer, document.GetURI(), useVSDiagnostics, previousResultId: resultId); - // Result should be different, but diagnostics should be the same - Assert.NotEqual(resultId, results.Single().ResultId); - Assert.Equal("CS1513", results.Single().Diagnostics.Single().Code); + // Diagnostics should be re-calculated, but re-use the same resultId since they are the same). + // TODO - need to validate that diagnostics were re-calculated. Maybe ENC tests are enough + Assert.Equal(resultId, results.Single().ResultId); } [Theory, CombinatorialData] From db6d1946a728f337634660ac798af305ee780eff Mon Sep 17 00:00:00 2001 From: David Barbet Date: Tue, 22 Oct 2024 12:57:53 -0700 Subject: [PATCH 02/10] Update tests to assert we return unchanged when diagnostic data is unchanged --- .../Diagnostics/DiagnosticsPullCacheTests.cs | 155 ++++++++++++++++++ .../Diagnostics/PullDiagnosticTests.cs | 136 +++++++-------- 2 files changed, 217 insertions(+), 74 deletions(-) create mode 100644 src/LanguageServer/ProtocolUnitTests/Diagnostics/DiagnosticsPullCacheTests.cs diff --git a/src/LanguageServer/ProtocolUnitTests/Diagnostics/DiagnosticsPullCacheTests.cs b/src/LanguageServer/ProtocolUnitTests/Diagnostics/DiagnosticsPullCacheTests.cs new file mode 100644 index 0000000000000..08d7a15399478 --- /dev/null +++ b/src/LanguageServer/ProtocolUnitTests/Diagnostics/DiagnosticsPullCacheTests.cs @@ -0,0 +1,155 @@ +// 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. + +using System; +using System.Collections.Immutable; +using System.Composition; +using System.Linq; +using System.Threading; +using System.Threading.Tasks; +using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Host.Mef; +using Microsoft.CodeAnalysis.LanguageServer.Handler; +using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; +using Microsoft.CodeAnalysis.SolutionCrawler; +using Microsoft.CodeAnalysis.Test.Utilities; +using Roslyn.Test.Utilities; +using Xunit; +using Xunit.Abstractions; +using LSP = Roslyn.LanguageServer.Protocol; + +namespace Microsoft.CodeAnalysis.LanguageServer.UnitTests.Diagnostics; +public class DiagnosticsPullCacheTests(ITestOutputHelper testOutputHelper) : AbstractPullDiagnosticTestsBase(testOutputHelper) +{ + [Theory, CombinatorialData] + public async Task TestDocumentDiagnosticsCallsDiagnosticSourceWhenVersionChanges(bool useVSDiagnostics, bool mutatingLspWorkspace) + { + var markup = +@"class A { }"; + await using var testLspServer = await CreateTestWorkspaceWithDiagnosticsAsync(markup, mutatingLspWorkspace, BackgroundAnalysisScope.OpenFiles, useVSDiagnostics); + + var testProvider = (TestDiagnosticSourceProvider)testLspServer.TestWorkspace.ExportProvider.GetExportedValues().Single(d => d is TestDiagnosticSourceProvider); + + var document = testLspServer.GetCurrentSolution().Projects.Single().Documents.Single(); + + await OpenDocumentAsync(testLspServer, document); + var results = await RunGetDocumentPullDiagnosticsAsync(testLspServer, document.GetURI(), useVSDiagnostics); + Assert.Equal(TestDiagnosticSource.Id, results[0].Diagnostics.Single().Code); + Assert.Equal(1, testProvider.DiagnosticsRequestedCount); + + // Make a change that modifies the versions we use to cache. + await InsertTextAsync(testLspServer, document, position: 0, text: " "); + + results = await RunGetDocumentPullDiagnosticsAsync( + testLspServer, document.GetURI(), + useVSDiagnostics, + previousResultId: results[0].ResultId); + + // Assert diagnostics were calculated again even though we got an unchanged result. + Assert.Null(results.Single().Diagnostics); + Assert.Equal(results[0].ResultId, results.Single().ResultId); + Assert.Equal(2, testProvider.DiagnosticsRequestedCount); + } + + [Theory, CombinatorialData] + public async Task TestDocumentDiagnosticsCallsDiagnosticSourceWhenGlobalVersionChanges(bool useVSDiagnostics, bool mutatingLspWorkspace) + { + var markup = +@"class A { }"; + await using var testLspServer = await CreateTestWorkspaceWithDiagnosticsAsync(markup, mutatingLspWorkspace, BackgroundAnalysisScope.OpenFiles, useVSDiagnostics); + + var testProvider = (TestDiagnosticSourceProvider)testLspServer.TestWorkspace.ExportProvider.GetExportedValues().Single(d => d is TestDiagnosticSourceProvider); + + var document = testLspServer.GetCurrentSolution().Projects.Single().Documents.Single(); + + await OpenDocumentAsync(testLspServer, document); + var results = await RunGetDocumentPullDiagnosticsAsync(testLspServer, document.GetURI(), useVSDiagnostics); + Assert.Equal(TestDiagnosticSource.Id, results[0].Diagnostics.Single().Code); + Assert.Equal(1, testProvider.DiagnosticsRequestedCount); + + // Make a global version change + var refresher = testLspServer.TestWorkspace.ExportProvider.GetExportedValue(); + refresher.RequestWorkspaceRefresh(); + + results = await RunGetDocumentPullDiagnosticsAsync( + testLspServer, document.GetURI(), + useVSDiagnostics, + previousResultId: results[0].ResultId); + + // Assert diagnostics were calculated again even though we got an unchanged result. + Assert.Null(results.Single().Diagnostics); + Assert.Equal(results[0].ResultId, results.Single().ResultId); + Assert.Equal(2, testProvider.DiagnosticsRequestedCount); + } + + [Theory, CombinatorialData] + public async Task TestDocumentDiagnosticsDoesNotCallDiagnosticSourceWhenVersionSame(bool useVSDiagnostics, bool mutatingLspWorkspace) + { + var markup = +@"class A { }"; + await using var testLspServer = await CreateTestWorkspaceWithDiagnosticsAsync(markup, mutatingLspWorkspace, BackgroundAnalysisScope.OpenFiles, useVSDiagnostics); + + var testProvider = (TestDiagnosticSourceProvider)testLspServer.TestWorkspace.ExportProvider.GetExportedValues().Single(d => d is TestDiagnosticSourceProvider); + + var document = testLspServer.GetCurrentSolution().Projects.Single().Documents.Single(); + + await OpenDocumentAsync(testLspServer, document); + var results = await RunGetDocumentPullDiagnosticsAsync(testLspServer, document.GetURI(), useVSDiagnostics); + Assert.Equal(TestDiagnosticSource.Id, results[0].Diagnostics.Single().Code); + Assert.Equal(1, testProvider.DiagnosticsRequestedCount); + + // Make another request without modifying anything and assert we did not re-calculate anything. + results = await RunGetDocumentPullDiagnosticsAsync( + testLspServer, document.GetURI(), + useVSDiagnostics, + previousResultId: results[0].ResultId); + + // Assert diagnostics were not recalculated. + Assert.Null(results.Single().Diagnostics); + Assert.Equal(results[0].ResultId, results.Single().ResultId); + Assert.Equal(1, testProvider.DiagnosticsRequestedCount); + } + + protected override TestComposition Composition => base.Composition.AddParts(typeof(TestDiagnosticSourceProvider)); + + private class TestDiagnosticSource(Document document, TestDiagnosticSourceProvider provider) : AbstractDocumentDiagnosticSource(document) + { + public const string Id = "Id"; + + public override Task> GetDiagnosticsAsync(RequestContext context, CancellationToken cancellationToken) + { + Interlocked.Increment(ref provider.DiagnosticsRequestedCount); + return Task.FromResult>([new DiagnosticData(Id, category: "category", context.Document!.Name, DiagnosticSeverity.Error, DiagnosticSeverity.Error, + isEnabledByDefault: true, warningLevel: 0, [], ImmutableDictionary.Empty,context.Document!.Project.Id, + new DiagnosticDataLocation(new FileLinePositionSpan(context.Document!.FilePath!, new Text.LinePosition(0, 0), new Text.LinePosition(0, 0))))]); + } + + public override bool IsLiveSource() + { + return true; + } + } + + [Export(typeof(IDiagnosticSourceProvider)), Shared, PartNotDiscoverable] + [method: ImportingConstructor] + [method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)] + private class TestDiagnosticSourceProvider() : IDiagnosticSourceProvider + { + public bool IsDocument => true; + + public string Name => nameof(TestDiagnosticSource); + + public int DiagnosticsRequestedCount = 0; + + public ValueTask> CreateDiagnosticSourcesAsync(RequestContext context, CancellationToken cancellationToken) + { + return new ValueTask>([new TestDiagnosticSource(context.Document!, this)]); + } + + public bool IsEnabled(LSP.ClientCapabilities clientCapabilities) + { + return true; + } + } +} diff --git a/src/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs b/src/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs index d691ffc12ebf9..cf313a0a5ebd1 100644 --- a/src/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs @@ -330,7 +330,6 @@ public async Task TestDocumentDiagnosticsWhenGlobalStateChanges(bool useVSDiagno testLspServer, document.GetURI(), useVSDiagnostics, previousResultId: resultId); // Diagnostics should be re-calculated, but re-use the same resultId since they are the same). - // TODO - need to validate that diagnostics were re-calculated. Maybe ENC tests are enough Assert.Equal(resultId, results.Single().ResultId); } @@ -958,23 +957,17 @@ public async Task TestWorkspaceDiagnosticsForClosedFilesWithRunCodeAnalysisAndFS var results2 = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: CreateDiagnosticParamsFromPreviousReports(results)); - Assert.Equal(results.Length, results2.Length); - - Assert.Equal(results[0].Diagnostics, results2[0].Diagnostics); - // this should be considered a build-error, since it was produced by the last code-analysis run. - Assert.Contains(VSDiagnosticTags.BuildError, results2[0].Diagnostics.Single().Tags!); - Assert.Equal(results[1].Diagnostics, results2[1].Diagnostics); - Assert.Equal(results[2].Diagnostics, results2[2].Diagnostics); + // We did not run code analysis - we should not get any new diagnostics. + AssertEx.Empty(results2); // Re-run code analysis and verify up-to-date diagnostics are returned now, i.e. there are no compiler errors. await testLspServer.RunCodeAnalysisAsync(projectId); - var results3 = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: CreateDiagnosticParamsFromPreviousReports(results2)); + var results3 = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: CreateDiagnosticParamsFromPreviousReports(results)); - Assert.Equal(results.Length, results3.Length); + // We should get 1 changed diagnostic now that we have re-run code analysis, the rest are unchanged. + Assert.Equal(1, results3.Length); AssertEx.Empty(results3[0].Diagnostics); - AssertEx.Empty(results3[1].Diagnostics); - AssertEx.Empty(results3[2].Diagnostics); } [Theory, CombinatorialData, WorkItem("https://github.com/dotnet/roslyn/issues/65967")] @@ -1006,23 +999,19 @@ public async Task TestWorkspaceDiagnosticsForClosedFilesWithRunCodeAnalysisFSAOn var results2 = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: CreateDiagnosticParamsFromPreviousReports(results)); - Assert.Equal(results.Length, results2.Length); - - Assert.Equal(results.Length, results2.Length); + // We should get 1 report clearing out the diagnostics for the changed file. The other files have unchanged diagnostics and are not reported. + Assert.Equal(1, results2.Length); AssertEx.Empty(results2[0].Diagnostics); - AssertEx.Empty(results2[1].Diagnostics); - AssertEx.Empty(results2[2].Diagnostics); // Now rerun code analysis and verify we still get up-to-date workspace diagnostics. await testLspServer.RunCodeAnalysisAsync(projectId); - var results3 = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: CreateDiagnosticParamsFromPreviousReports(results2)); - - Assert.Equal(results2.Length, results3.Length); + // Concat the single changed result from result2 with the unchanged reports from results to mimick the client re-using unchanged reports. + var previousParams = results2.Concat(results[1]).Concat(results[2]); + var results3 = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: CreateDiagnosticParamsFromPreviousReports(previousParams)); - Assert.Equal(results2[0].Diagnostics, results3[0].Diagnostics); - Assert.Equal(results2[1].Diagnostics, results3[1].Diagnostics); - Assert.Equal(results2[2].Diagnostics, results3[2].Diagnostics); + // The diagnostics did not change, so we should get nothing back. + AssertEx.Empty(results3); } [Theory, CombinatorialData] @@ -1433,16 +1422,10 @@ public async Task TestWorkspaceDiagnosticsForRemovedDocument(bool useVSDiagnosti var results2 = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: CreateDiagnosticParamsFromPreviousReports(results)); // First doc should show up as removed. - Assert.Equal(3, results2.Length); + Assert.Equal(1, results2.Length); // VS represents removal with null diagnostics, VS code represents with an empty diagnostics array. Assert.Equal(useVSDiagnostics ? null : [], results2[0].Diagnostics); Assert.Null(results2[0].ResultId); - - // Second and third doc should be changed as the project has changed. - AssertEx.Empty(results2[1].Diagnostics); - Assert.NotEqual(results[1].ResultId, results2[1].ResultId); - AssertEx.Empty(results2[2].Diagnostics); - Assert.NotEqual(results[2].ResultId, results2[2].ResultId); } [Theory, CombinatorialData] @@ -1488,16 +1471,10 @@ public async Task TestWorkspaceDiagnosticsRemovedAfterErrorIsFixed(bool useVSDia var results2 = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: CreateDiagnosticParamsFromPreviousReports(results)); - Assert.Equal(3, results2.Length); + // Only 1 file has changed diagnostics, we should get 1 report with an empty set. + // Reports for the other files are not sent (they are unchanged and we skip reporting unchanged docs in workspace diags). + Assert.Equal(1, results2.Length); AssertEx.Empty(results2[0].Diagnostics); - // Project has changed, so we re-computed diagnostics as changes in the first file - // may have changed results in the second. - AssertEx.Empty(results2[1].Diagnostics); - AssertEx.Empty(results2[2].Diagnostics); - - Assert.NotEqual(results[0].ResultId, results2[0].ResultId); - Assert.NotEqual(results[1].ResultId, results2[1].ResultId); - Assert.NotEqual(results[2].ResultId, results2[2].ResultId); } [Theory, CombinatorialData] @@ -1625,15 +1602,17 @@ public class {|caret:|} { } var previousResultIds = CreateDiagnosticParamsFromPreviousReports(results); results = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: previousResultIds); AssertEx.NotNull(results); - Assert.Equal(4, results.Length); + + // We should get updated diagnostics for both A and B now that B exists. + Assert.Equal(2, results.Length); // Verify diagnostics for A.cs are updated as the type B now exists. AssertEx.Empty(results[0].Diagnostics); Assert.NotEqual(previousResultIds[0].resultId, results[0].ResultId); // Verify diagnostics for B.cs are updated as the class definition is now correct. - AssertEx.Empty(results[2].Diagnostics); - Assert.NotEqual(previousResultIds[2].resultId, results[2].ResultId); + AssertEx.Empty(results[1].Diagnostics); + Assert.NotEqual(previousResultIds[2].resultId, results[1].ResultId); } [Theory, CombinatorialData] @@ -1642,14 +1621,14 @@ public async Task TestWorkspaceDiagnosticsWithChangeInRecursiveReferencedProject var markup1 = @"namespace M { - public class A + public class A : B { } }"; var markup2 = @"namespace M { - public class B + public class B : C { } }"; @@ -1679,14 +1658,17 @@ public class {|caret:|} await using var testLspServer = await CreateTestWorkspaceFromXmlAsync(workspaceXml, mutatingLspWorkspace, BackgroundAnalysisScope.FullSolution, useVSDiagnostics).ConfigureAwait(false); var csproj3Document = testLspServer.GetCurrentSolution().Projects.Where(p => p.Name == "CSProj3").Single().Documents.First(); - // Verify we have a diagnostic in C.cs initially. + // Verify we have diagnostics in A.cs, B.cs, and C.cs initially. var results = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics); AssertEx.NotNull(results); Assert.Equal(6, results.Length); - AssertEx.Empty(results[0].Diagnostics); + // Type C does not exist. + Assert.Equal("CS0246", results[0].Diagnostics.Single().Code); AssertEx.Empty(results[1].Diagnostics); - AssertEx.Empty(results[2].Diagnostics); + // Type C does not exist. + Assert.Equal("CS0246", results[2].Diagnostics.Single().Code); AssertEx.Empty(results[3].Diagnostics); + // Syntax error missing identifier. Assert.Equal("CS1001", results[4].Diagnostics.Single().Code); AssertEx.Empty(results[5].Diagnostics); @@ -1700,24 +1682,21 @@ public class {|caret:|} var previousResultIds = CreateDiagnosticParamsFromPreviousReports(results); results = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: previousResultIds).ConfigureAwait(false); AssertEx.NotNull(results); - Assert.Equal(6, results.Length); - // Verify that new diagnostics are returned for all files (even though the diagnostics for the first two files are the same) - // since we re-calculate when transitive project dependencies change. - AssertEx.Empty(results[0].Diagnostics); + // Verify that we get 3 new reports as the diagnostics in A.cs, B.cs, and C.cs have all changed due to the transitive change in C.cs. + Assert.Equal(3, results.Length); + + // A.cs should report CS0012 indicating that C is not directly referenced. + Assert.Equal("CS0012", results[0].Diagnostics.Single().Code); Assert.NotEqual(previousResultIds[0].resultId, results[0].ResultId); + + // B.cs should no longer have a diagnostic since C exists. AssertEx.Empty(results[1].Diagnostics); - Assert.NotEqual(previousResultIds[1].resultId, results[1].ResultId); + Assert.NotEqual(previousResultIds[2].resultId, results[1].ResultId); + // C.cs should no longer have a diagnostic since C was fully defined. AssertEx.Empty(results[2].Diagnostics); - Assert.NotEqual(previousResultIds[2].resultId, results[2].ResultId); - AssertEx.Empty(results[3].Diagnostics); - Assert.NotEqual(previousResultIds[3].resultId, results[3].ResultId); - - AssertEx.Empty(results[4].Diagnostics); - Assert.NotEqual(previousResultIds[4].resultId, results[4].ResultId); - AssertEx.Empty(results[5].Diagnostics); - Assert.NotEqual(previousResultIds[5].resultId, results[5].ResultId); + Assert.NotEqual(previousResultIds[4].resultId, results[2].ResultId); } [Theory, CombinatorialData] @@ -1767,15 +1746,16 @@ public class {|caret:|} { } var previousResultIds = CreateDiagnosticParamsFromPreviousReports(results); results = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResultIds); AssertEx.NotNull(results); - Assert.Equal(2, results.Length); - // Note: tehre will be no results for A.cs as it is unchanged and does not reference CSProj2. - // Verify that the diagnostics result for B.cs reflects the change we made to it. + // We should get 1 report from B.cs reflecting the change we made to it. + // A.cs is unchanged and we will not get a report for it. + Assert.Equal(1, results.Length); AssertEx.Empty(results[0].Diagnostics); Assert.NotEqual(previousResultIds[2].resultId, results[0].ResultId); - AssertEx.Empty(results[1].Diagnostics); - Assert.NotEqual(previousResultIds[3].resultId, results[1].ResultId); } +#if true +#else +#endif [Theory, CombinatorialData] public async Task TestWorkspaceDiagnosticsWithDependentProjectReloadedAndChanged(bool useVSDiagnostics, bool mutatingLspWorkspace) @@ -1788,7 +1768,13 @@ class A : B { } var markup2 = @"namespace M { - public class {|caret:|} { } + public class B + { + public static void Do() + { + int unusedVariable; + } + } }"; var workspaceXml = @@ -1810,12 +1796,13 @@ public class {|caret:|} { } var results = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics); AssertEx.NotNull(results); Assert.Equal(4, results.Length); - Assert.Equal("CS0246", results[0].Diagnostics.Single().Code); - Assert.Equal("CS1001", results[2].Diagnostics.Single().Code); + AssertEx.Empty(results[0].Diagnostics); + Assert.Equal("CS0168", results[2].Diagnostics.Single().Code); + Assert.Equal(LSP.DiagnosticSeverity.Warning, results[2].Diagnostics.Single().Severity); // Change and reload the project via the workspace. var projectInfo = testLspServer.TestWorkspace.Projects.Where(p => p.AssemblyName == "CSProj2").Single().ToProjectInfo(); - projectInfo = projectInfo.WithCompilationOptions(projectInfo.CompilationOptions!.WithPlatform(Platform.X64)); + projectInfo = projectInfo.WithCompilationOptions(projectInfo.CompilationOptions!.WithGeneralDiagnosticOption(ReportDiagnostic.Error)); testLspServer.TestWorkspace.OnProjectReloaded(projectInfo); var operations = testLspServer.TestWorkspace.ExportProvider.GetExportedValue(); await operations.GetWaiter(FeatureAttribute.Workspace).ExpeditedWaitAsync(); @@ -1825,11 +1812,12 @@ public class {|caret:|} { } results = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: previousResultIds); AssertEx.NotNull(results); - Assert.Equal(4, results.Length); - // The diagnostics should have been recalculated for both projects as a referenced project changed. - Assert.Equal("CS0246", results[0].Diagnostics.Single().Code); - Assert.Equal("CS1001", results[2].Diagnostics.Single().Code); + // We should get a single report back for B.cs now that the diagnostic has been promoted to an error. + // The diagnostics in A.cs did not change and so are not reported again. + Assert.Equal(1, results.Length); + Assert.Equal("CS0168", results[0].Diagnostics.Single().Code); + Assert.Equal(LSP.DiagnosticSeverity.Error, results[0].Diagnostics.Single().Severity); } [Theory, CombinatorialData] @@ -2139,5 +2127,5 @@ public async Task TestWorkspaceDiagnosticsForClosedFilesSwitchFSAFromOffToOn(boo AssertEx.Empty(results[1].Diagnostics); } - #endregion +#endregion } From d1b4128e19f45afc63e466420e029830988d0eee Mon Sep 17 00:00:00 2001 From: David Barbet Date: Tue, 22 Oct 2024 13:47:46 -0700 Subject: [PATCH 03/10] Update spellcheck tests to assert new behavior --- .../SpellCheck/SpellCheckTests.cs | 35 +++++++++---------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/src/LanguageServer/ProtocolUnitTests/SpellCheck/SpellCheckTests.cs b/src/LanguageServer/ProtocolUnitTests/SpellCheck/SpellCheckTests.cs index 4f882cdbe5239..46a5b5668f8df 100644 --- a/src/LanguageServer/ProtocolUnitTests/SpellCheck/SpellCheckTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/SpellCheck/SpellCheckTests.cs @@ -62,7 +62,7 @@ public async Task TestDocumentResultsForOpenFiles(bool mutatingLspWorkspace) Assert.Single(results); AssertJsonEquals(results.Single(), new VSInternalSpellCheckableRangeReport { - ResultId = "DocumentSpellCheckHandler:0", + ResultId = "DocumentSpellCheckHandler:1", Ranges = GetRanges(testDocument.AnnotatedSpans), }); } @@ -98,7 +98,7 @@ class {|Identifier:A{{v}}|} { AssertJsonEquals(results[i], new VSInternalSpellCheckableRangeReport { - ResultId = "DocumentSpellCheckHandler:0", + ResultId = "DocumentSpellCheckHandler:1", Ranges = allRanges.Skip(3 * i * 1000).Take(3 * 1000).ToArray(), }); } @@ -130,7 +130,7 @@ public async Task TestDocumentResultsForRemovedDocument(bool mutatingLspWorkspac Assert.Single(results); AssertJsonEquals(results.Single(), new VSInternalSpellCheckableRangeReport { - ResultId = "DocumentSpellCheckHandler:0", + ResultId = "DocumentSpellCheckHandler:1", Ranges = GetRanges(workspace.Documents.Single().AnnotatedSpans), }); @@ -166,7 +166,7 @@ public async Task TestNoChangeIfDocumentResultsCalledTwice(bool mutatingLspWorks Assert.Single(results); AssertJsonEquals(results.Single(), new VSInternalSpellCheckableRangeReport { - ResultId = "DocumentSpellCheckHandler:0", + ResultId = "DocumentSpellCheckHandler:1", Ranges = GetRanges(testLspServer.TestWorkspace.Documents.Single().AnnotatedSpans), }); @@ -202,7 +202,7 @@ public async Task TestDocumentResultChangedAfterEntityAdded(bool mutatingLspWork Assert.Single(results); AssertJsonEquals(results.Single(), new VSInternalSpellCheckableRangeReport { - ResultId = "DocumentSpellCheckHandler:0", + ResultId = "DocumentSpellCheckHandler:1", Ranges = GetRanges(testLspServer.TestWorkspace.Documents.Single().AnnotatedSpans), }); @@ -223,13 +223,13 @@ public async Task TestDocumentResultChangedAfterEntityAdded(bool mutatingLspWork Assert.Single(results); AssertJsonEquals(results.Single(), new VSInternalSpellCheckableRangeReport { - ResultId = "DocumentSpellCheckHandler:1", + ResultId = "DocumentSpellCheckHandler:2", Ranges = GetRanges(annotatedSpans), }); } [Theory, CombinatorialData] - public async Task TestDocumentResultIdChangesAfterEdit(bool mutatingLspWorkspace) + public async Task TestDocumentResultIdSameAfterIrrelevantEdit(bool mutatingLspWorkspace) { var markup = @"class {|Identifier:A|} @@ -249,7 +249,7 @@ public async Task TestDocumentResultIdChangesAfterEdit(bool mutatingLspWorkspace Assert.Single(results); AssertJsonEquals(results.Single(), new VSInternalSpellCheckableRangeReport { - ResultId = "DocumentSpellCheckHandler:0", + ResultId = "DocumentSpellCheckHandler:1", Ranges = GetRanges(testLspServer.TestWorkspace.Documents.Single().AnnotatedSpans), }); @@ -264,7 +264,6 @@ public async Task TestDocumentResultIdChangesAfterEdit(bool mutatingLspWorkspace AssertJsonEquals(results.Single(), new VSInternalSpellCheckableRangeReport { ResultId = "DocumentSpellCheckHandler:1", - Ranges = GetRanges(testLspServer.TestWorkspace.Documents.Single().AnnotatedSpans), }); } @@ -291,7 +290,7 @@ class {|Identifier:A|} Assert.Single(results); AssertJsonEquals(results.Single(), new VSInternalSpellCheckableRangeReport { - ResultId = "DocumentSpellCheckHandler:0", + ResultId = "DocumentSpellCheckHandler:1", Ranges = GetRanges(testLspServer.TestWorkspace.Documents.Single().AnnotatedSpans), }); } @@ -318,14 +317,14 @@ public async Task TestStreamingDocumentDiagnostics(bool mutatingLspWorkspace) Assert.Single(results); AssertJsonEquals(results.Single(), new VSInternalSpellCheckableRangeReport { - ResultId = "DocumentSpellCheckHandler:0", + ResultId = "DocumentSpellCheckHandler:1", Ranges = GetRanges(testLspServer.TestWorkspace.Documents.Single().AnnotatedSpans), }); } #endregion - #region Workspace Diagnostics + #region Workspace [Theory, CombinatorialData] public async Task TestWorkspaceResultsForClosedFiles(bool mutatingLspWorkspace) @@ -346,7 +345,7 @@ public async Task TestWorkspaceResultsForClosedFiles(bool mutatingLspWorkspace) AssertJsonEquals(results[0], new VSInternalWorkspaceSpellCheckableReport { TextDocument = CreateTextDocumentIdentifier(document.GetURI()), - ResultId = "WorkspaceSpellCheckHandler:0", + ResultId = "WorkspaceSpellCheckHandler:1", Ranges = GetRanges(testLspServer.TestWorkspace.Documents.First().AnnotatedSpans), }); AssertEx.Empty(results[1].Ranges); @@ -419,7 +418,7 @@ public async Task TestWorkspaceResultsForRemovedDocument(bool mutatingLspWorkspa AssertJsonEquals(results[0], new VSInternalWorkspaceSpellCheckableReport { TextDocument = CreateTextDocumentIdentifier(document.GetURI()), - ResultId = "WorkspaceSpellCheckHandler:0", + ResultId = "WorkspaceSpellCheckHandler:1", Ranges = GetRanges(testLspServer.TestWorkspace.Documents.First().AnnotatedSpans), }); AssertEx.Empty(results[1].Ranges); @@ -457,7 +456,7 @@ public async Task TestNoChangeIfWorkspaceResultsCalledTwice(bool mutatingLspWork AssertJsonEquals(results[0], new VSInternalWorkspaceSpellCheckableReport { TextDocument = CreateTextDocumentIdentifier(document.GetURI()), - ResultId = "WorkspaceSpellCheckHandler:0", + ResultId = "WorkspaceSpellCheckHandler:1", Ranges = GetRanges(testLspServer.TestWorkspace.Documents.First().AnnotatedSpans), }); AssertEx.Empty(results[1].Ranges); @@ -493,7 +492,7 @@ public async Task TestWorkspaceResultUpdatedAfterEdit(bool mutatingLspWorkspace) AssertJsonEquals(results[0], new VSInternalWorkspaceSpellCheckableReport { TextDocument = CreateTextDocumentIdentifier(document.GetURI()), - ResultId = "WorkspaceSpellCheckHandler:0", + ResultId = "WorkspaceSpellCheckHandler:1", Ranges = GetRanges(testLspServer.TestWorkspace.Documents.First().AnnotatedSpans), }); AssertEx.Empty(results[1].Ranges); @@ -518,7 +517,7 @@ public async Task TestWorkspaceResultUpdatedAfterEdit(bool mutatingLspWorkspace) AssertJsonEquals(results2[0], new VSInternalWorkspaceSpellCheckableReport { TextDocument = CreateTextDocumentIdentifier(document.GetURI()), - ResultId = "WorkspaceSpellCheckHandler:2", + ResultId = "WorkspaceSpellCheckHandler:3", Ranges = GetRanges(annotatedSpans), }); Assert.Null(results2[1].Ranges); @@ -546,7 +545,7 @@ public async Task TestStreamingWorkspaceResults(bool mutatingLspWorkspace) AssertJsonEquals(results[0], new VSInternalWorkspaceSpellCheckableReport { TextDocument = CreateTextDocumentIdentifier(document.GetURI()), - ResultId = "WorkspaceSpellCheckHandler:0", + ResultId = "WorkspaceSpellCheckHandler:1", Ranges = GetRanges(testLspServer.TestWorkspace.Documents.First().AnnotatedSpans), }); AssertEx.Empty(results[1].Ranges); From 87e5a3d49fcfa04d98fbaa54447d49c853946adf Mon Sep 17 00:00:00 2001 From: David Barbet Date: Tue, 22 Oct 2024 13:52:48 -0700 Subject: [PATCH 04/10] Fix one more diagnostics test --- .../Diagnostics/AdditionalFileDiagnosticsTests.cs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/LanguageServer/ProtocolUnitTests/Diagnostics/AdditionalFileDiagnosticsTests.cs b/src/LanguageServer/ProtocolUnitTests/Diagnostics/AdditionalFileDiagnosticsTests.cs index 79c732d12ec6e..0bd88792bf0d4 100644 --- a/src/LanguageServer/ProtocolUnitTests/Diagnostics/AdditionalFileDiagnosticsTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/Diagnostics/AdditionalFileDiagnosticsTests.cs @@ -73,17 +73,11 @@ public async Task TestWorkspaceDiagnosticsWithRemovedAdditionalFile(bool useVSDi await testLspServer.TestWorkspace.ChangeSolutionAsync(newSolution); var results2 = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: CreateDiagnosticParamsFromPreviousReports(results)); - Assert.Equal(3, results2.Length); - // The first report is the report for the removed additional file. + // We should get a single report for the removed additional file, the rest are unchanged and do not report. + Assert.Equal(1, results2.Length); Assert.Equal(useVSDiagnostics ? null : [], results2[0].Diagnostics); Assert.Null(results2[0].ResultId); - - // The other files should have new results since the solution changed. - AssertEx.Empty(results2[1].Diagnostics); - Assert.NotNull(results2[1].ResultId); - AssertEx.Empty(results2[2].Diagnostics); - Assert.NotNull(results2[2].ResultId); } [Theory, CombinatorialData] From 042876513d1735febfd39894fc58e2b12c0f0c2c Mon Sep 17 00:00:00 2001 From: David Barbet Date: Tue, 22 Oct 2024 13:57:37 -0700 Subject: [PATCH 05/10] rename method for clarity --- .../Handler/Diagnostics/AbstractPullDiagnosticHandler.cs | 2 +- .../Protocol/Handler/PullHandlers/VersionedPullCache.cs | 2 +- .../Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs b/src/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs index e455c29e65eb3..768adc82f6111 100644 --- a/src/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs +++ b/src/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs @@ -160,7 +160,7 @@ protected virtual Task WaitForChangesAsync(string? category, RequestContext cont var project = diagnosticSource.GetProject(); var cacheState = new DiagnosticsRequestState(project, globalStateVersion, context, diagnosticSource); - var newResult = await versionedCache.GetNewResultIdAsync( + var newResult = await versionedCache.GetOrComputeNewDataAsync( documentIdToPreviousDiagnosticParams, diagnosticSource.GetId(), project, diff --git a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs index 2a8dad4907ebe..bfd8c8caefc6f 100644 --- a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs +++ b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs @@ -68,7 +68,7 @@ internal abstract partial class VersionedPullCachea map of roslyn document or project id to the previous result the client sent us for that doc. /// the id of the project or document that we are checking to see if it has changed. /// Null when results are unchanged, otherwise returns a non-null new resultId. - public async Task<(string ResultId, ImmutableArray Data)?> GetNewResultIdAsync( + public async Task<(string ResultId, ImmutableArray Data)?> GetOrComputeNewDataAsync( Dictionary idToClientLastResult, ProjectOrDocumentId projectOrDocumentId, Project project, diff --git a/src/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs b/src/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs index 22a77b83f6c9a..3dffb71521054 100644 --- a/src/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs +++ b/src/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs @@ -100,7 +100,7 @@ protected AbstractSpellCheckHandler() } var documentToPreviousDiagnosticParams = documentToPreviousParams.ToDictionary(kvp => new ProjectOrDocumentId(kvp.Key.Id), kvp => kvp.Value); - var newResult = await _versionedCache.GetNewResultIdAsync( + var newResult = await _versionedCache.GetOrComputeNewDataAsync( documentToPreviousDiagnosticParams, new ProjectOrDocumentId(document.Id), document.Project, From 74b8298ddd83d4f21a0537cfcd8bb5e911a5cc4c Mon Sep 17 00:00:00 2001 From: David Barbet Date: Tue, 22 Oct 2024 14:18:18 -0700 Subject: [PATCH 06/10] removed unused method --- .../SpellCheck/AbstractSpellCheckingHandler.cs | 11 ----------- 1 file changed, 11 deletions(-) diff --git a/src/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs b/src/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs index 3dffb71521054..88197335d9adb 100644 --- a/src/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs +++ b/src/LanguageServer/Protocol/Handler/SpellCheck/AbstractSpellCheckingHandler.cs @@ -225,16 +225,5 @@ private async Task HandleRemovedDocumentsAsync( } } } - - private static async Task<(Checksum parseOptionsChecksum, Checksum textChecksum)> ComputeChecksumsAsync(Document document, CancellationToken cancellationToken) - { - var project = document.Project; - var parseOptionsChecksum = project.State.GetParseOptionsChecksum(); - - var documentChecksumState = await document.State.GetStateChecksumsAsync(cancellationToken).ConfigureAwait(false); - var textChecksum = documentChecksumState.Text; - - return (parseOptionsChecksum, textChecksum); - } } } From 82bdba1c2defdaa6f594bee8003e50b7a01b992b Mon Sep 17 00:00:00 2001 From: David Barbet Date: Tue, 22 Oct 2024 15:15:00 -0700 Subject: [PATCH 07/10] fix formatting --- .../ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs b/src/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs index cf313a0a5ebd1..778167de168cf 100644 --- a/src/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs @@ -2127,5 +2127,5 @@ public async Task TestWorkspaceDiagnosticsForClosedFilesSwitchFSAFromOffToOn(boo AssertEx.Empty(results[1].Diagnostics); } -#endregion + #endregion } From df070f34aa153c82f1f8dc147d489621aebc2bbf Mon Sep 17 00:00:00 2001 From: David Barbet Date: Wed, 23 Oct 2024 12:02:51 -0700 Subject: [PATCH 08/10] Switch to checksums for comparing computed data to last data --- .../Diagnostics/DiagnosticsPullCache.cs | 79 +++++++++++++++++++ .../VersionedPullCache.CacheItem.cs | 18 ++--- .../PullHandlers/VersionedPullCache.cs | 2 + .../Handler/SpellCheck/SpellCheckPullCache.cs | 13 +++ .../Portable/Workspace/Solution/Checksum.cs | 8 +- 5 files changed, 107 insertions(+), 13 deletions(-) diff --git a/src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs b/src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs index d8848c9b5f1c1..7231b19729d14 100644 --- a/src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs +++ b/src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs @@ -6,6 +6,7 @@ using System.Threading; using System.Threading.Tasks; using Microsoft.CodeAnalysis.Diagnostics; +using Microsoft.CodeAnalysis.Text; using Roslyn.LanguageServer.Protocol; using Roslyn.Utilities; @@ -40,5 +41,83 @@ public override async Task> ComputeDataAsync(Diag state.Context.TraceInformation($"Found {diagnostics.Length} diagnostics for {state.DiagnosticSource.ToDisplayString()}"); return diagnostics; } + + public override Checksum ComputeChecksum(ImmutableArray data) + { + // Create checksums of diagnostic data and sort to ensure stable ordering for comparison. + var diagnosticDataChecksums = data + .SelectAsArray(d => Checksum.Create(d, SerializeDiagnosticData)) + .Sort(); + + return Checksum.Create(diagnosticDataChecksums); + } + + private static void SerializeDiagnosticData(DiagnosticData diagnosticData, ObjectWriter writer) + { + writer.WriteString(diagnosticData.Id); + writer.WriteString(diagnosticData.Category); + writer.WriteString(diagnosticData.Message); + writer.WriteInt32((int)diagnosticData.Severity); + writer.WriteInt32((int)diagnosticData.DefaultSeverity); + writer.WriteBoolean(diagnosticData.IsEnabledByDefault); + writer.WriteInt32(diagnosticData.WarningLevel); + + // Ensure the tag order is stable before we write it. + foreach (var tag in diagnosticData.CustomTags.Sort()) + { + writer.WriteString(tag); + } + + foreach (var key in diagnosticData.Properties.Keys.ToImmutableArray().Sort()) + { + writer.WriteString(key); + writer.WriteString(diagnosticData.Properties[key]); + } + + if (diagnosticData.ProjectId != null) + writer.WriteGuid(diagnosticData.ProjectId.Id); + + WriteDiagnosticDataLocation(diagnosticData.DataLocation, writer); + + foreach (var additionalLocation in diagnosticData.AdditionalLocations) + { + WriteDiagnosticDataLocation(additionalLocation, writer); + } + + writer.WriteString(diagnosticData.Language); + writer.WriteString(diagnosticData.Title); + writer.WriteString(diagnosticData.Description); + writer.WriteString(diagnosticData.HelpLink); + writer.WriteBoolean(diagnosticData.IsSuppressed); + + static void WriteDiagnosticDataLocation(DiagnosticDataLocation location, ObjectWriter writer) + { + WriteFileLinePositionSpan(location.UnmappedFileSpan, writer); + if (location.DocumentId != null) + writer.WriteGuid(location.DocumentId.Id); + + WriteFileLinePositionSpan(location.MappedFileSpan, writer); + } + + static void WriteFileLinePositionSpan(FileLinePositionSpan fileSpan, ObjectWriter writer) + { + writer.WriteString(fileSpan.Path); + WriteLinePositionSpan(fileSpan.Span, writer); + writer.WriteBoolean(fileSpan.HasMappedPath); + } + + static void WriteLinePositionSpan(LinePositionSpan span, ObjectWriter writer) + { + WriteLinePosition(span.Start, writer); + WriteLinePosition(span.End, writer); + } + + static void WriteLinePosition(LinePosition position, ObjectWriter writer) + { + writer.WriteInt32(position.Line); + writer.WriteInt32(position.Character); + } + + } } } diff --git a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs index 1fea6ebcfd1f8..f163b0681a965 100644 --- a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs +++ b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs @@ -35,7 +35,7 @@ private class CacheItem(string uniqueKey) /// LSP text, reloading the same project). So we additionally store: /// A TExpensiveVersion (normally a checksum) checksum that will still allow us to reuse data even when /// unimportant changes happen that trigger the cheap version change detection. - /// The hash of the data that was computed when the resultId was generated. + /// The checksum of the data that was computed when the resultId was generated. /// /// When the versions above change, we must recalculate the data. However sometimes that data ends up being exactly the same as the prior request. /// When that happens, this allows us to send back an unchanged result instead of reserializing data the client already has. @@ -44,7 +44,7 @@ private class CacheItem(string uniqueKey) /// /// /// - private (string resultId, TCheapVersion cheapVersion, TExpensiveVersion expensiveVersion, int hashedData)? _lastResult; + private (string resultId, TCheapVersion cheapVersion, TExpensiveVersion expensiveVersion, Checksum dataChecksum)? _lastResult; /// /// Updates the values for this cache entry. Guarded by @@ -98,16 +98,16 @@ _lastResult is not null && // Compute the new result for the request. var data = await cache.ComputeDataAsync(state, cancellationToken).ConfigureAwait(false); - var dataHash = GetComputedDataHash(data); + var dataChecksum = cache.ComputeChecksum(data); string newResultId; - if (_lastResult is not null && _lastResult?.resultId == previousPullResult?.PreviousResultId && _lastResult?.hashedData == dataHash) + if (_lastResult is not null && _lastResult?.resultId == previousPullResult?.PreviousResultId && _lastResult?.dataChecksum == dataChecksum) { // The new data we've computed is exactly the same as the data we computed last time even though the versions have changed. // Instead of reserializing everything, we can return the same result id back to the client. // Ensure we store the updated versions we calculated with the old resultId. - _lastResult = (_lastResult.Value.resultId, cheapVersion, expensiveVersion, dataHash); + _lastResult = (_lastResult.Value.resultId, cheapVersion, expensiveVersion, dataChecksum); return null; } else @@ -123,17 +123,11 @@ _lastResult is not null && // Note that we can safely update the map before computation as any cancellation or exception // during computation means that the client will never recieve this resultId and so cannot ask us for it. newResultId = $"{uniqueKey}:{cache.IncrementResultId()}"; - _lastResult = (newResultId, cheapVersion, expensiveVersion, dataHash); + _lastResult = (newResultId, cheapVersion, expensiveVersion, dataChecksum); return (newResultId, data); } } } - - private static int GetComputedDataHash(ImmutableArray data) - { - var hashes = data.SelectAsArray(d => d.GetHashCode()).Sort(); - return Hash.CombineValues(hashes); - } } } diff --git a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs index bfd8c8caefc6f..2ca20abc4f04a 100644 --- a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs +++ b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs @@ -61,6 +61,8 @@ internal abstract partial class VersionedPullCache public abstract Task> ComputeDataAsync(TState state, CancellationToken cancellationToken); + public abstract Checksum ComputeChecksum(ImmutableArray data); + /// /// If results have changed since the last request this calculates and returns a new /// non-null resultId to use for subsequent computation and caches it. diff --git a/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs b/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs index 8529a16381b97..00a69de273ce8 100644 --- a/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs +++ b/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs @@ -29,6 +29,12 @@ internal class SpellCheckPullCache(string uniqueKey) : VersionedPullCache<(Check return (parseOptionsChecksum, textChecksum); } + public override Checksum ComputeChecksum(ImmutableArray data) + { + var checksums = data.SelectAsArray(s => Checksum.Create(s, SerializeSpellCheckSpan)).Sort(); + return Checksum.Create(checksums); + } + public override async Task> ComputeDataAsync(SpellCheckState state, CancellationToken cancellationToken) { var spans = await state.Service.GetSpansAsync(state.Document, cancellationToken).ConfigureAwait(false); @@ -39,4 +45,11 @@ public override async Task> ComputeDataAsync(Spel { return SpecializedTasks.Null(); } + + private void SerializeSpellCheckSpan(SpellCheckSpan span, ObjectWriter writer) + { + writer.WriteInt32(span.TextSpan.Start); + writer.WriteInt32(span.TextSpan.Length); + writer.WriteInt32((int)span.Kind); + } } diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs b/src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs index 44ea0b096f45d..a41817bd55212 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs @@ -21,7 +21,7 @@ namespace Microsoft.CodeAnalysis; [DataContract, StructLayout(LayoutKind.Explicit, Size = HashSize)] internal readonly partial record struct Checksum( [field: FieldOffset(0)][property: DataMember(Order = 0)] long Data1, - [field: FieldOffset(8)][property: DataMember(Order = 1)] long Data2) + [field: FieldOffset(8)][property: DataMember(Order = 1)] long Data2) : IComparable { /// /// The intended size of the structure. @@ -116,6 +116,12 @@ public override int GetHashCode() // The checksum is already a hash. Just read a 4-byte value to get a well-distributed hash code. return (int)Data1; } + + public int CompareTo(Checksum other) + { + var result = Data1.CompareTo(other.Data1); + return (result != 0) ? result : Data2.CompareTo(other.Data2); + } } internal static class ChecksumExtensions From 8624a3f452be5ad7152b6a69a78c2e0092291750 Mon Sep 17 00:00:00 2001 From: David Barbet Date: Wed, 23 Oct 2024 12:22:40 -0700 Subject: [PATCH 09/10] misc feedback --- .../Diagnostics/DiagnosticsPullCache.cs | 6 +- .../VersionedPullCache.CacheItem.cs | 18 ++- .../PullHandlers/VersionedPullCache.cs | 148 +++++++++--------- .../Handler/SpellCheck/SpellCheckPullCache.cs | 1 + .../Diagnostics/DiagnosticsPullCacheTests.cs | 4 +- 5 files changed, 92 insertions(+), 85 deletions(-) diff --git a/src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs b/src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs index 7231b19729d14..41e00efaeae52 100644 --- a/src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs +++ b/src/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticsPullCache.cs @@ -12,7 +12,8 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; -internal abstract partial class AbstractPullDiagnosticHandler where TDiagnosticsParams : IPartialResultParams +internal abstract partial class AbstractPullDiagnosticHandler + where TDiagnosticsParams : IPartialResultParams { internal record struct DiagnosticsRequestState(Project Project, int GlobalStateVersion, RequestContext Context, IDiagnosticSource DiagnosticSource); @@ -23,7 +24,7 @@ internal record struct DiagnosticsRequestState(Project Project, int GlobalStateV /// and works well for us in the normal case. The latter still allows us to reuse diagnostics when changes happen that /// update the version stamp but not the content (for example, forking LSP text). /// - internal class DiagnosticsPullCache(string uniqueKey) : VersionedPullCache<(int globalStateVersion, VersionStamp? dependentVersion), (int globalStateVersion, Checksum dependentChecksum), DiagnosticsRequestState, DiagnosticData>(uniqueKey) + private sealed class DiagnosticsPullCache(string uniqueKey) : VersionedPullCache<(int globalStateVersion, VersionStamp? dependentVersion), (int globalStateVersion, Checksum dependentChecksum), DiagnosticsRequestState, DiagnosticData>(uniqueKey) { public override async Task<(int globalStateVersion, VersionStamp? dependentVersion)> ComputeCheapVersionAsync(DiagnosticsRequestState state, CancellationToken cancellationToken) { @@ -35,6 +36,7 @@ internal class DiagnosticsPullCache(string uniqueKey) : VersionedPullCache<(int return (state.GlobalStateVersion, await state.Project.GetDependentChecksumAsync(cancellationToken).ConfigureAwait(false)); } + /// public override async Task> ComputeDataAsync(DiagnosticsRequestState state, CancellationToken cancellationToken) { var diagnostics = await state.DiagnosticSource.GetDiagnosticsAsync(state.Context, cancellationToken).ConfigureAwait(false); diff --git a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs index f163b0681a965..4c7abb5d4ba86 100644 --- a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs +++ b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.CacheItem.cs @@ -16,16 +16,17 @@ internal abstract partial class VersionedPullCache and in /// This type ensures that the state for a particular key is never updated concurrently for the same key (but different key states can be concurrent). /// - private class CacheItem(string uniqueKey) + private sealed class CacheItem(string uniqueKey) { /// /// Guards access to . /// This ensures that a cache entry is fully updated in a single transaction. /// - private readonly SemaphoreSlim _semaphore = new(1); + private readonly SemaphoreSlim _gate = new(initialCount: 1); /// /// Stores the current state associated with this cache item. + /// Guarded by /// /// /// The resultId reported to the client. @@ -47,9 +48,9 @@ private class CacheItem(string uniqueKey) private (string resultId, TCheapVersion cheapVersion, TExpensiveVersion expensiveVersion, Checksum dataChecksum)? _lastResult; /// - /// Updates the values for this cache entry. Guarded by + /// Updates the values for this cache entry. Guarded by /// - /// Returns null if the previousPullResult can be re-used, otherwise returns a new resultId and the new data associated with it. + /// Returns if the previousPullResult can be re-used, otherwise returns a new resultId and the new data associated with it. /// public async Task<(string, ImmutableArray)?> UpdateCacheItemAsync( VersionedPullCache cache, @@ -60,7 +61,7 @@ private class CacheItem(string uniqueKey) { // Ensure that we only update the cache item one at a time. // This means that the computation of new data for this item only occurs sequentially. - using (await _semaphore.DisposableWaitAsync(cancellationToken).ConfigureAwait(false)) + using (await _gate.DisposableWaitAsync(cancellationToken).ConfigureAwait(false)) { TCheapVersion cheapVersion; TExpensiveVersion expensiveVersion; @@ -106,7 +107,10 @@ _lastResult is not null && // The new data we've computed is exactly the same as the data we computed last time even though the versions have changed. // Instead of reserializing everything, we can return the same result id back to the client. - // Ensure we store the updated versions we calculated with the old resultId. + // Ensure we store the updated versions we calculated against old resultId. If we do not do this, + // subsequent requests will always fail the version comparison check (the resultId is still associated with the older version even + // though we reused it here for a newer version) and will trigger re-computation. + // By storing the updated version with the resultId we can short circuit earlier in the version checks. _lastResult = (_lastResult.Value.resultId, cheapVersion, expensiveVersion, dataChecksum); return null; } @@ -122,7 +126,7 @@ _lastResult is not null && // // Note that we can safely update the map before computation as any cancellation or exception // during computation means that the client will never recieve this resultId and so cannot ask us for it. - newResultId = $"{uniqueKey}:{cache.IncrementResultId()}"; + newResultId = $"{uniqueKey}:{cache.GetNextResultId()}"; _lastResult = (newResultId, cheapVersion, expensiveVersion, dataChecksum); return (newResultId, data); } diff --git a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs index 2ca20abc4f04a..0d7722f9f21d7 100644 --- a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs +++ b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs @@ -11,92 +11,92 @@ using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; using Roslyn.Utilities; -namespace Microsoft.CodeAnalysis.LanguageServer.Handler +namespace Microsoft.CodeAnalysis.LanguageServer.Handler; + +/// +/// Specialized cache used by the 'pull' LSP handlers. Supports storing data to know when to tell a client +/// that existing results can be reused, or if new results need to be computed. Multiple keys can be used, +/// with different computation costs to determine if the previous cached data is still valid. +/// +internal abstract partial class VersionedPullCache(string uniqueKey) + where TComputedData : notnull { + /// - /// Specialized cache used by the 'pull' LSP handlers. Supports storing data to know when to tell a client - /// that existing results can be reused, or if new results need to be computed. Multiple keys can be used, - /// with different computation costs to determine if the previous cached data is still valid. + /// Map of workspace and diagnostic source to the data used to make the last pull report. + /// This is a concurrent dictionary as parallel access is allowed for different workspace+project/doc combinations. + /// + /// The itself however will internally guarantee that the state for a specific workspace+project/doc will only + /// be updated sequentially. /// - internal abstract partial class VersionedPullCache(string uniqueKey) where TComputedData : notnull - { + private readonly ConcurrentDictionary<(Workspace workspace, ProjectOrDocumentId id), CacheItem> _idToLastReportedResult = []; - /// - /// Map of workspace and diagnostic source to the data used to make the last pull report. - /// This is a concurrent dictionary as parallel access is allowed for different workspace+project/doc combinations. - /// - /// The itself however will internally guarantee that the state for a specific workspace+project/doc will only - /// be updated sequentially. - /// - private readonly ConcurrentDictionary<(Workspace workspace, ProjectOrDocumentId id), CacheItem> _idToLastReportedResult = []; - - /// - /// The next available id to label results with. Note that results are tagged on a per-document bases. That - /// way we can update results with the client with per-doc granularity. - /// - /// Called by with Interlocked access to ensure that all cache items generate unique resultIds. - /// - private long _nextDocumentResultId; + /// + /// The next available id to label results with. Note that results are tagged on a per-document bases. That + /// way we can update results with the client with per-doc granularity. + /// + /// Called by with Interlocked access to ensure that all cache items generate unique resultIds. + /// + private long _nextDocumentResultId; - /// - /// Computes a cheap version of the current state. This is compared to the cached version we calculated for the client's previous resultId. - /// - /// Note - this will run under the semaphore in . - /// - public abstract Task ComputeCheapVersionAsync(TState state, CancellationToken cancellationToken); + /// + /// Computes a cheap version of the current state. This is compared to the cached version we calculated for the client's previous resultId. + /// + /// Note - this will run under the semaphore in . + /// + public abstract Task ComputeCheapVersionAsync(TState state, CancellationToken cancellationToken); - /// - /// Computes a more expensive version of the current state. If the cheap versions are mismatched, we then compare the expensive version of the current state against the - /// expensive version we have cached for the client's previous resultId. - /// - /// Note - this will run under the semaphore in . - /// - public abstract Task ComputeExpensiveVersionAsync(TState state, CancellationToken cancellationToken); + /// + /// Computes a more expensive version of the current state. If the cheap versions are mismatched, we then compare the expensive version of the current state against the + /// expensive version we have cached for the client's previous resultId. + /// + /// Note - this will run under the semaphore in . + /// + public abstract Task ComputeExpensiveVersionAsync(TState state, CancellationToken cancellationToken); - /// - /// Computes new data for this request. This data must be hashable as it we store the hash with the requestId to determine if - /// the data has changed between requests. - /// - /// Note - this will run under the semaphore in . - /// - public abstract Task> ComputeDataAsync(TState state, CancellationToken cancellationToken); + /// + /// Computes new data for this request. This data must be hashable as it we store the hash with the requestId to determine if + /// the data has changed between requests. + /// + /// Note - this will run under the semaphore in . + /// + public abstract Task> ComputeDataAsync(TState state, CancellationToken cancellationToken); - public abstract Checksum ComputeChecksum(ImmutableArray data); + public abstract Checksum ComputeChecksum(ImmutableArray data); - /// - /// If results have changed since the last request this calculates and returns a new - /// non-null resultId to use for subsequent computation and caches it. - /// - /// a map of roslyn document or project id to the previous result the client sent us for that doc. - /// the id of the project or document that we are checking to see if it has changed. - /// Null when results are unchanged, otherwise returns a non-null new resultId. - public async Task<(string ResultId, ImmutableArray Data)?> GetOrComputeNewDataAsync( - Dictionary idToClientLastResult, - ProjectOrDocumentId projectOrDocumentId, - Project project, - TState state, - CancellationToken cancellationToken) - { - var workspace = project.Solution.Workspace; + /// + /// If results have changed since the last request this calculates and returns a new + /// non-null resultId to use for subsequent computation and caches it. + /// + /// a map of roslyn document or project id to the previous result the client sent us for that doc. + /// the id of the project or document that we are checking to see if it has changed. + /// Null when results are unchanged, otherwise returns a non-null new resultId. + public async Task<(string ResultId, ImmutableArray Data)?> GetOrComputeNewDataAsync( + Dictionary idToClientLastResult, + ProjectOrDocumentId projectOrDocumentId, + Project project, + TState state, + CancellationToken cancellationToken) + { + var workspace = project.Solution.Workspace; - // We have to make sure we've been fully loaded before using cached results as the previous results may not be complete. - var isFullyLoaded = await IsFullyLoadedAsync(project.Solution, cancellationToken).ConfigureAwait(false); - var previousResult = IDictionaryExtensions.GetValueOrDefault(idToClientLastResult, projectOrDocumentId); + // We have to make sure we've been fully loaded before using cached results as the previous results may not be complete. + var isFullyLoaded = await IsFullyLoadedAsync(project.Solution, cancellationToken).ConfigureAwait(false); + var previousResult = IDictionaryExtensions.GetValueOrDefault(idToClientLastResult, projectOrDocumentId); - var cacheEntry = _idToLastReportedResult.GetOrAdd((project.Solution.Workspace, projectOrDocumentId), (_) => new CacheItem(uniqueKey)); - return await cacheEntry.UpdateCacheItemAsync(this, previousResult, isFullyLoaded, state, cancellationToken).ConfigureAwait(false); - } + var cacheEntry = _idToLastReportedResult.GetOrAdd((project.Solution.Workspace, projectOrDocumentId), (_) => new CacheItem(uniqueKey)); + return await cacheEntry.UpdateCacheItemAsync(this, previousResult, isFullyLoaded, state, cancellationToken).ConfigureAwait(false); + } - public long IncrementResultId() - { - return Interlocked.Increment(ref _nextDocumentResultId); - } + private long GetNextResultId() + { + return Interlocked.Increment(ref _nextDocumentResultId); + } - private static async Task IsFullyLoadedAsync(Solution solution, CancellationToken cancellationToken) - { - var workspaceStatusService = solution.Services.GetRequiredService(); - var isFullyLoaded = await workspaceStatusService.IsFullyLoadedAsync(cancellationToken).ConfigureAwait(false); - return isFullyLoaded; - } + private static async Task IsFullyLoadedAsync(Solution solution, CancellationToken cancellationToken) + { + var workspaceStatusService = solution.Services.GetRequiredService(); + var isFullyLoaded = await workspaceStatusService.IsFullyLoadedAsync(cancellationToken).ConfigureAwait(false); + return isFullyLoaded; } } diff --git a/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs b/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs index 00a69de273ce8..1cebb11c4cb1c 100644 --- a/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs +++ b/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs @@ -43,6 +43,7 @@ public override async Task> ComputeDataAsync(Spel public override Task ComputeExpensiveVersionAsync(SpellCheckState state, CancellationToken cancellationToken) { + // Spell check does not need an expensive version check - we return null to effectively skip this check. return SpecializedTasks.Null(); } diff --git a/src/LanguageServer/ProtocolUnitTests/Diagnostics/DiagnosticsPullCacheTests.cs b/src/LanguageServer/ProtocolUnitTests/Diagnostics/DiagnosticsPullCacheTests.cs index 08d7a15399478..0fc045014346d 100644 --- a/src/LanguageServer/ProtocolUnitTests/Diagnostics/DiagnosticsPullCacheTests.cs +++ b/src/LanguageServer/ProtocolUnitTests/Diagnostics/DiagnosticsPullCacheTests.cs @@ -14,13 +14,13 @@ using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics; using Microsoft.CodeAnalysis.SolutionCrawler; using Microsoft.CodeAnalysis.Test.Utilities; -using Roslyn.Test.Utilities; using Xunit; using Xunit.Abstractions; using LSP = Roslyn.LanguageServer.Protocol; namespace Microsoft.CodeAnalysis.LanguageServer.UnitTests.Diagnostics; -public class DiagnosticsPullCacheTests(ITestOutputHelper testOutputHelper) : AbstractPullDiagnosticTestsBase(testOutputHelper) +public class DiagnosticsPullCacheTests(ITestOutputHelper testOutputHelper) + : AbstractPullDiagnosticTestsBase(testOutputHelper) { [Theory, CombinatorialData] public async Task TestDocumentDiagnosticsCallsDiagnosticSourceWhenVersionChanges(bool useVSDiagnostics, bool mutatingLspWorkspace) From fe6a2c42a12d680ab39b7caf063e58726e34d8d5 Mon Sep 17 00:00:00 2001 From: David Barbet Date: Wed, 23 Oct 2024 13:12:02 -0700 Subject: [PATCH 10/10] minor nits --- .../Protocol/Handler/PullHandlers/VersionedPullCache.cs | 1 - .../Protocol/Handler/SpellCheck/SpellCheckPullCache.cs | 2 +- src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs index 0d7722f9f21d7..740ee258b2a3c 100644 --- a/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs +++ b/src/LanguageServer/Protocol/Handler/PullHandlers/VersionedPullCache.cs @@ -21,7 +21,6 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler; internal abstract partial class VersionedPullCache(string uniqueKey) where TComputedData : notnull { - /// /// Map of workspace and diagnostic source to the data used to make the last pull report. /// This is a concurrent dictionary as parallel access is allowed for different workspace+project/doc combinations. diff --git a/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs b/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs index 1cebb11c4cb1c..33df968335b8e 100644 --- a/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs +++ b/src/LanguageServer/Protocol/Handler/SpellCheck/SpellCheckPullCache.cs @@ -16,7 +16,7 @@ internal record struct SpellCheckState(ISpellCheckSpanService Service, Document /// Simplified version of that only uses a /// single cheap key to check results against. /// -internal class SpellCheckPullCache(string uniqueKey) : VersionedPullCache<(Checksum parseOptionsChecksum, Checksum textChecksum)?, object?, SpellCheckState, SpellCheckSpan>(uniqueKey) +internal sealed class SpellCheckPullCache(string uniqueKey) : VersionedPullCache<(Checksum parseOptionsChecksum, Checksum textChecksum)?, object?, SpellCheckState, SpellCheckSpan>(uniqueKey) { public override async Task<(Checksum parseOptionsChecksum, Checksum textChecksum)?> ComputeCheapVersionAsync(SpellCheckState state, CancellationToken cancellationToken) { diff --git a/src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs b/src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs index a41817bd55212..0d697cb0e5369 100644 --- a/src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs +++ b/src/Workspaces/Core/Portable/Workspace/Solution/Checksum.cs @@ -120,7 +120,7 @@ public override int GetHashCode() public int CompareTo(Checksum other) { var result = Data1.CompareTo(other.Data1); - return (result != 0) ? result : Data2.CompareTo(other.Data2); + return result != 0 ? result : Data2.CompareTo(other.Data2); } }