Skip to content

Properly update workspace options when switching from FSA on to off #72971

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Apr 10, 2024
Merged

Properly update workspace options when switching from FSA on to off #72971

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
2b9c877
Properly update workspace options when switching from FSA on to off
CyrusNajmabadi Apr 10, 2024
b0b10f7
update tests
CyrusNajmabadi Apr 10, 2024
3079d4f
Merge remote-tracking branch 'upstream/main' into fsaSwitch
CyrusNajmabadi Apr 10, 2024
4b284aa
Update tests
CyrusNajmabadi Apr 10, 2024
2fe4181
docs
CyrusNajmabadi Apr 10, 2024
7b5614c
use collection exprs
CyrusNajmabadi Apr 10, 2024
9ac239b
Update f# as well
CyrusNajmabadi Apr 10, 2024
f48dac8
Cleanup
CyrusNajmabadi Apr 10, 2024
98c5d69
cleanup
CyrusNajmabadi Apr 10, 2024
6ba5c07
Docs
CyrusNajmabadi Apr 10, 2024
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
2 changes: 2 additions & 0 deletions src/EditorFeatures/Core/ExternalAccess/VSTypeScript/Api/VSTypeScriptGlobalOptions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ public void SetBackgroundAnalysisScope(bool openFilesOnly)
{
_globalOptions.SetGlobalOption(SolutionCrawlerOptionsStorage.BackgroundAnalysisScopeOption, InternalLanguageNames.TypeScript,
openFilesOnly ? BackgroundAnalysisScope.OpenFiles : BackgroundAnalysisScope.FullSolution);
_globalOptions.SetGlobalOption(SolutionCrawlerOptionsStorage.CompilerDiagnosticsScopeOption, InternalLanguageNames.TypeScript,
openFilesOnly ? CompilerDiagnosticsScope.OpenFiles : CompilerDiagnosticsScope.FullSolution);
Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Apr 10, 2024

Choose a reason for hiding this comment

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

both these options are checked, so both need to change in order to get proper behavior corresponding to the single switch TS has.


_globalOptions.SetGlobalOption(SolutionCrawlerOptionsStorage.RemoveDocumentDiagnosticsOnDocumentClose, InternalLanguageNames.TypeScript,
openFilesOnly);
Expand Down
77 changes: 50 additions & 27 deletions src/Features/LanguageServer/Protocol/Handler/Diagnostics/AbstractPullDiagnosticHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,8 @@ protected virtual Task WaitForChangesAsync(RequestContext context, CancellationT
}
else
{
Contract.ThrowIfNull(context.Solution);

var clientCapabilities = context.GetRequiredClientCapabilities();
var category = GetDiagnosticCategory(diagnosticsParams) ?? "";
var sourceIdentifier = GetDiagnosticSourceIdentifier(diagnosticsParams) ?? "";
Expand All @@ -160,11 +162,13 @@ protected virtual Task WaitForChangesAsync(RequestContext context, CancellationT
// Create a mapping from documents to the previous results the client says it has for them. That way as we
// process documents we know if we should tell the client it should stay the same, or we can tell it what
// the updated diagnostics are.
var documentToPreviousDiagnosticParams = GetIdToPreviousDiagnosticParams(context, previousResults, out var removedResults);
using var _1 = PooledDictionary<ProjectOrDocumentId, PreviousPullResult>.GetInstance(out var documentIdToPreviousDiagnosticParams);
using var _2 = PooledHashSet<PreviousPullResult>.GetInstance(out var removedDocuments);
ProcessPreviousResults(context.Solution, previousResults, documentIdToPreviousDiagnosticParams, removedDocuments);

// First, let the client know if any workspace documents have gone away. That way it can remove those for
// the user from squiggles or error-list.
HandleRemovedDocuments(context, removedResults, progress);
HandleRemovedDocuments(context, removedDocuments, progress);

// Next process each file in priority order. Determine if diagnostics are changed or unchanged since the
// last time we notified the client. Report back either to the client so they can update accordingly.
Expand All @@ -173,14 +177,19 @@ protected virtual Task WaitForChangesAsync(RequestContext context, CancellationT

context.TraceInformation($"Processing {orderedSources.Length} documents");

// Keep track of what diagnostic sources we see this time around. For any we do not see this time
// around, we'll notify the client that the diagnostics for it have been removed.
using var _3 = PooledHashSet<ProjectOrDocumentId>.GetInstance(out var seenDiagnosticSourceIds);

foreach (var diagnosticSource in orderedSources)
{
seenDiagnosticSourceIds.Add(diagnosticSource.GetId());
var globalStateVersion = _diagnosticRefresher.GlobalStateVersion;

var project = diagnosticSource.GetProject();

var newResultId = await versionedCache.GetNewResultIdAsync(
documentToPreviousDiagnosticParams,
documentIdToPreviousDiagnosticParams,
diagnosticSource.GetId(),
project,
computeCheapVersionAsync: async () => (globalStateVersion, await project.GetDependentVersionAsync(cancellationToken).ConfigureAwait(false)),
Expand All @@ -201,12 +210,30 @@ await ComputeAndReportCurrentDiagnosticsAsync(
//
// Note: if this is a workspace request, we can do nothing, as that will be interpreted by the
// client as nothing having been changed for that document.
var previousParams = documentToPreviousDiagnosticParams[diagnosticSource.GetId()];
var previousParams = documentIdToPreviousDiagnosticParams[diagnosticSource.GetId()];
if (TryCreateUnchangedReport(previousParams.TextDocument, previousParams.PreviousResultId, out var report))
progress.Report(report);
}
}

// Now, for any diagnostics reported from a prior source that we do not see this time around, report its
// diagnostics as being removed. This allows for different sets of diagnostic-sources to be computed
// each time around, while still producing accurate diagnostic reports.
//
// Only do this if we haven't already created a removal report for that prior result above.
//
// Note: we are intentionally notifying the client that this is not a remove (vs an empty set of
// results). As far as we and the client are concerned, this document no longer exists at this point
// for the purposes of diagnostics.
foreach (var (projectOrDocumentId, previousDiagnosticParams) in documentIdToPreviousDiagnosticParams)
{
if (!seenDiagnosticSourceIds.Contains(projectOrDocumentId) &&
!removedDocuments.Contains(previousDiagnosticParams))
{
progress.Report(CreateRemovedReport(previousDiagnosticParams.TextDocument));
Copy link
Member

@dibarbet dibarbet Apr 10, 2024

Choose a reason for hiding this comment

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

Might be good to note here that we're creating a removed report because as far as the client is concerned for diagnostics - the document is removed.

We don't want to create an empty report as that might mean the client thinks we're still reporting diagnostics for it (but with none).

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Apr 10, 2024

Choose a reason for hiding this comment

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

sounds good.

}
}

// Clear out the solution context to avoid retaining memory
// https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1809058
context.ClearSolutionContext();
Expand All @@ -222,36 +249,32 @@ await ComputeAndReportCurrentDiagnosticsAsync(
}

return CreateReturn(progress);
}

private static Dictionary<ProjectOrDocumentId, PreviousPullResult> GetIdToPreviousDiagnosticParams(
RequestContext context, ImmutableArray<PreviousPullResult> previousResults, out ImmutableArray<PreviousPullResult> removedDocuments)
{
Contract.ThrowIfNull(context.Solution);

var result = new Dictionary<ProjectOrDocumentId, PreviousPullResult>();
using var _ = ArrayBuilder<PreviousPullResult>.GetInstance(out var removedDocumentsBuilder);
foreach (var diagnosticParams in previousResults)
static void ProcessPreviousResults(
Solution solution,
ImmutableArray<PreviousPullResult> previousResults,
Dictionary<ProjectOrDocumentId, PreviousPullResult> idToPreviousDiagnosticParams,
HashSet<PreviousPullResult> removedResults)
{
if (diagnosticParams.TextDocument != null)
foreach (var diagnosticParams in previousResults)
{
var id = GetIdForPreviousResult(diagnosticParams.TextDocument, context.Solution);
if (id != null)
{
result[id.Value] = diagnosticParams;
}
else
if (diagnosticParams.TextDocument != null)
{
// The client previously had a result from us for this document, but we no longer have it in our solution.
// Record it so we can report to the client that it has been removed.
removedDocumentsBuilder.Add(diagnosticParams);
var id = GetIdForPreviousResult(diagnosticParams.TextDocument, solution);
if (id != null)
{
idToPreviousDiagnosticParams[id.Value] = diagnosticParams;
}
else
{
// The client previously had a result from us for this document, but we no longer have it in our solution.
// Record it so we can report to the client that it has been removed.
removedResults.Add(diagnosticParams);
}
}
}
}

removedDocuments = removedDocumentsBuilder.ToImmutable();
return result;

static ProjectOrDocumentId? GetIdForPreviousResult(TextDocumentIdentifier textDocumentIdentifier, Solution solution)
{
var document = solution.GetDocument(textDocumentIdentifier);
Expand Down Expand Up @@ -306,7 +329,7 @@ private async Task ComputeAndReportCurrentDiagnosticsAsync(
progress.Report(report);
}

private void HandleRemovedDocuments(RequestContext context, ImmutableArray<PreviousPullResult> removedPreviousResults, BufferedProgress<TReport> progress)
private void HandleRemovedDocuments(RequestContext context, HashSet<PreviousPullResult> removedPreviousResults, BufferedProgress<TReport> progress)
{
foreach (var removedResult in removedPreviousResults)
{
Expand Down
6 changes: 1 addition & 5 deletions src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/AbstractPullDiagnosticTestsBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,12 +35,8 @@ namespace Microsoft.CodeAnalysis.LanguageServer.UnitTests.Diagnostics
using DocumentDiagnosticPartialReport = SumType<RelatedFullDocumentDiagnosticReport, RelatedUnchangedDocumentDiagnosticReport, DocumentDiagnosticReportPartialResult>;
using WorkspaceDiagnosticPartialReport = SumType<WorkspaceDiagnosticReport, WorkspaceDiagnosticReportPartialResult>;

public abstract class AbstractPullDiagnosticTestsBase : AbstractLanguageServerProtocolTests
public abstract class AbstractPullDiagnosticTestsBase(ITestOutputHelper testOutputHelper) : AbstractLanguageServerProtocolTests(testOutputHelper)
{
protected AbstractPullDiagnosticTestsBase(ITestOutputHelper testOutputHelper) : base(testOutputHelper)
{
}

private protected override TestAnalyzerReferenceByLanguage CreateTestAnalyzersReference()
{
var builder = ImmutableDictionary.CreateBuilder<string, ImmutableArray<DiagnosticAnalyzer>>();
Expand Down
Loading