Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refresh diagnostics when fading options change #77322

Merged
merged 3 commits into from
Feb 24, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,12 @@ public static bool IsGlobalOptionAffectingDiagnostics(IOption2 option)
option == SolutionCrawlerOptionsStorage.BackgroundAnalysisScopeOption ||
option == SolutionCrawlerOptionsStorage.SolutionBackgroundAnalysisScopeOption ||
option == SolutionCrawlerOptionsStorage.CompilerDiagnosticsScopeOption ||
option == s_crashOnAnalyzerException;
option == s_crashOnAnalyzerException ||
// Fading is controlled by reporting diagnostics for the faded region. So if a fading option changes we
// want to recompute and rereport up to date diagnostics.
option == FadingOptions.FadeOutUnusedImports ||
option == FadingOptions.FadeOutUnusedMembers ||
option == FadingOptions.FadeOutUnreachableCode;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this controls if the diagnostic service itself broadcasts taht a diagnostics refresh is needed.


public void RequestDiagnosticRefresh()
=> _diagnosticsRefresher.RequestWorkspaceRefresh();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,8 @@ protected virtual Task WaitForChangesAsync(string? category, RequestContext cont
var handlerName = $"{this.GetType().Name}(category: {category})";
context.TraceInformation($"{handlerName} started getting diagnostics");

var versionedCache = _categoryToVersionedCache.GetOrAdd(handlerName, static handlerName => new(handlerName));
var versionedCache = _categoryToVersionedCache.GetOrAdd(
handlerName, static (handlerName, globalOptions) => new(globalOptions, handlerName), GlobalOptions);

// Get the set of results the request said were previously reported. We can use this to determine both
// what to skip, and what files we have to tell the client have been removed.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@
using System.Collections.Immutable;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.CodeStyle;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Text;
using Roslyn.LanguageServer.Protocol;
using Roslyn.Utilities;
Expand All @@ -25,8 +28,11 @@ internal abstract partial class AbstractPullDiagnosticHandler<TDiagnosticsParams
/// 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).
/// </summary>
private sealed class DiagnosticsPullCache(string uniqueKey) : VersionedPullCache<(int globalStateVersion, VersionStamp? dependentVersion), (int globalStateVersion, Checksum dependentChecksum), DiagnosticsRequestState, ImmutableArray<DiagnosticData>>(uniqueKey)
private sealed class DiagnosticsPullCache(IGlobalOptionService globalOptions, string uniqueKey)
: VersionedPullCache<(int globalStateVersion, VersionStamp? dependentVersion), (int globalStateVersion, Checksum dependentChecksum), DiagnosticsRequestState, ImmutableArray<DiagnosticData>>(uniqueKey)
{
private readonly IGlobalOptionService _globalOptions = globalOptions;

public override async Task<(int globalStateVersion, VersionStamp? dependentVersion)> ComputeCheapVersionAsync(DiagnosticsRequestState state, CancellationToken cancellationToken)
{
return (state.GlobalStateVersion, await state.Project.GetDependentVersionAsync(cancellationToken).ConfigureAwait(false));
Expand All @@ -45,14 +51,28 @@ public override async Task<ImmutableArray<DiagnosticData>> ComputeDataAsync(Diag
return diagnostics;
}

public override Checksum ComputeChecksum(ImmutableArray<DiagnosticData> data)
public override Checksum ComputeChecksum(ImmutableArray<DiagnosticData> data, string language)
{
// Create checksums of diagnostic data and sort to ensure stable ordering for comparison.
var diagnosticDataChecksums = data
.SelectAsArray(d => Checksum.Create(d, SerializeDiagnosticData))
.Sort();
using var _ = ArrayBuilder<Checksum>.GetInstance(out var builder);
foreach (var datum in data)
builder.Add(Checksum.Create(datum, SerializeDiagnosticData));

// Ensure that if fading options change that we recompute the checksum as it will produce different data
// that we would report to the client.
var option1 = _globalOptions.GetOption(FadingOptions.FadeOutUnreachableCode, language);
var option2 = _globalOptions.GetOption(FadingOptions.FadeOutUnusedImports, language);
var option3 = _globalOptions.GetOption(FadingOptions.FadeOutUnusedMembers, language);

var value =
(option1 ? (1 << 2) : 0) |
(option2 ? (1 << 1) : 0) |
(option3 ? (1 << 0) : 0);

builder.Add(new Checksum(0, value));
builder.Sort();

return Checksum.Create(diagnosticDataChecksums);
return Checksum.Create(builder);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i needed to do tthe above because hte DiagnosticPullCache was computing the 'new daignostic checksum' and getting the same result. This si because it operates purely on DiagnosticData, and not on the 'transformed LSP diagnostics' that it would create from the data.

  1. We could either have it do the transform, and compute the checksum on that.
  2. audit it entirely for all special logic it uses to customize the result it returns.
  3. just add the known info that impacts results.

I opted for '3' for simplicity. Note that there is no actual checking if the option actually impacts the diagnostic results. We effectively just say that if any of these options changed that you should not use the cache and recompute. Given that changing this option would not be a common event, this seems totally reasonable.

}

private static void SerializeDiagnosticData(DiagnosticData diagnosticData, ObjectWriter writer)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ private sealed class CacheItem(string uniqueKey)
PreviousPullResult? previousPullResult,
bool isFullyLoaded,
TState state,
string language,
CancellationToken cancellationToken)
{
// Ensure that we only update the cache item one at a time.
Expand Down Expand Up @@ -99,7 +100,7 @@ _lastResult is not null &&

// Compute the new result for the request.
var data = await cache.ComputeDataAsync(state, cancellationToken).ConfigureAwait(false);
var dataChecksum = cache.ComputeChecksum(data);
var dataChecksum = cache.ComputeChecksum(data, language);

string newResultId;
if (_lastResult is not null && _lastResult?.resultId == previousPullResult?.PreviousResultId && _lastResult?.dataChecksum == dataChecksum)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ internal abstract partial class VersionedPullCache<TCheapVersion, TExpensiveVers
/// </summary>
public abstract Task<TComputedData> ComputeDataAsync(TState state, CancellationToken cancellationToken);

public abstract Checksum ComputeChecksum(TComputedData data);
public abstract Checksum ComputeChecksum(TComputedData data, string language);

/// <summary>
/// If results have changed since the last request this calculates and returns a new
Expand All @@ -75,14 +75,16 @@ internal abstract partial class VersionedPullCache<TCheapVersion, TExpensiveVers
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);
var cacheEntry = _idToLastReportedResult.GetOrAdd(
(project.Solution.Workspace, projectOrDocumentId),
static (_, uniqueKey) => new CacheItem(uniqueKey),
uniqueKey);
return await cacheEntry.UpdateCacheItemAsync(
this, previousResult, isFullyLoaded, state, project.Language, cancellationToken).ConfigureAwait(false);
}

private long GetNextResultId()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ internal sealed class SourceGeneratedDocumentCache(string uniqueKey) : Versioned
return SpecializedTasks.Null<object>();
}

public override Checksum ComputeChecksum(SourceText? data)
public override Checksum ComputeChecksum(SourceText? data, string language)
{
return data is null ? Checksum.Null : Checksum.From(data.GetChecksum());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ internal sealed class SpellCheckPullCache(string uniqueKey) : VersionedPullCache
return (parseOptionsChecksum, textChecksum);
}

public override Checksum ComputeChecksum(ImmutableArray<SpellCheckSpan> data)
public override Checksum ComputeChecksum(ImmutableArray<SpellCheckSpan> data, string language)
{
var checksums = data.SelectAsArray(s => Checksum.Create(s, SerializeSpellCheckSpan)).Sort();
return Checksum.Create(checksums);
Expand Down
Loading