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.

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

Do not report unchanged files in workspace-pull diagnostics #72529

Merged
merged 7 commits into from
Mar 15, 2024
Merged

Do not report unchanged files in workspace-pull diagnostics #72529

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
35abb42
Do not report unchanged files in workspace-pull diagnostics
CyrusNajmabadi Mar 13, 2024
ac8e437
Update tests
CyrusNajmabadi Mar 13, 2024
3704535
Merge branch 'main' into workspacePull
CyrusNajmabadi Mar 13, 2024
ad9182d
Change how we report htings
CyrusNajmabadi Mar 13, 2024
f057dc9
Merge branch 'workspacePull' of https://github.com/CyrusNajmabadi/ros…
CyrusNajmabadi Mar 13, 2024
b77e470
Fix tests
CyrusNajmabadi Mar 15, 2024
616fc28
Remove stale test
CyrusNajmabadi Mar 15, 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
29 changes: 19 additions & 10 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 @@ -20,16 +20,17 @@

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics
{
internal abstract class AbstractDocumentPullDiagnosticHandler<TDiagnosticsParams, TReport, TReturn>
: AbstractPullDiagnosticHandler<TDiagnosticsParams, TReport, TReturn>, ITextDocumentIdentifierHandler<TDiagnosticsParams, TextDocumentIdentifier?>
internal abstract class AbstractDocumentPullDiagnosticHandler<TDiagnosticsParams, TReport, TReturn>(
IDiagnosticAnalyzerService diagnosticAnalyzerService,
IDiagnosticsRefresher diagnosticRefresher,
IGlobalOptionService globalOptions)
: AbstractPullDiagnosticHandler<TDiagnosticsParams, TReport, TReturn>(
diagnosticAnalyzerService,
diagnosticRefresher,
globalOptions), ITextDocumentIdentifierHandler<TDiagnosticsParams, TextDocumentIdentifier?>
where TDiagnosticsParams : IPartialResultParams<TReport>
{
public AbstractDocumentPullDiagnosticHandler(
IDiagnosticAnalyzerService diagnosticAnalyzerService,
IDiagnosticsRefresher diagnosticRefresher,
IGlobalOptionService globalOptions) : base(diagnosticAnalyzerService, diagnosticRefresher, globalOptions)
{
}
protected sealed override bool IsWorkspacePullDiagnosticsHandler => false;

public abstract LSP.TextDocumentIdentifier? GetTextDocumentIdentifier(TDiagnosticsParams diagnosticsParams);
}
Expand Down Expand Up @@ -80,6 +81,8 @@ protected AbstractPullDiagnosticHandler(
GlobalOptions = globalOptions;
}

protected abstract bool IsWorkspacePullDiagnosticsHandler { get; }

protected virtual string? GetDiagnosticSourceIdentifier(TDiagnosticsParams diagnosticsParams) => null;

/// <summary>
Expand Down Expand Up @@ -187,8 +190,14 @@ await ComputeAndReportCurrentDiagnosticsAsync(
// Nothing changed between the last request and this one. Report a (null-diagnostics,
// same-result-id) response to the client as that means they should just preserve the current
// diagnostics they have for this file.
var previousParams = documentToPreviousDiagnosticParams[diagnosticSource.GetId()];
progress.Report(CreateUnchangedReport(previousParams.TextDocument, previousParams.PreviousResultId));
//
// 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.
if (!this.IsWorkspacePullDiagnosticsHandler)
Copy link
Member

Choose a reason for hiding this comment

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

Alternative approach - allow CreateUnchangedReport to return a nullable value, and in workspace diagnostics return a null value always.

Copy link
Member Author

Choose a reason for hiding this comment

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

fair. it seemed about the same... i'll noodle on it.

{
var previousParams = documentToPreviousDiagnosticParams[diagnosticSource.GetId()];
progress.Report(CreateUnchangedReport(previousParams.TextDocument, previousParams.PreviousResultId));
Copy link

Choose a reason for hiding this comment

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

Do you have the before/after previousParams.TextDocument int versions to compare too? If the version is different, we need to be careful about skipping.

Copy link
Member Author

Choose a reason for hiding this comment

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

We do not.

}
}
}

Expand Down
3 changes: 3 additions & 0 deletions ...es/LanguageServer/Protocol/Handler/Diagnostics/AbstractWorkspacePullDiagnosticsHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics;

internal abstract class AbstractWorkspacePullDiagnosticsHandler<TDiagnosticsParams, TReport, TReturn>
: AbstractPullDiagnosticHandler<TDiagnosticsParams, TReport, TReturn>, IDisposable
where TDiagnosticsParams : IPartialResultParams<TReport>
Expand All @@ -33,6 +34,8 @@ internal abstract class AbstractWorkspacePullDiagnosticsHandler<TDiagnosticsPara
/// </summary>
private int _lspChanged = 0;

protected sealed override bool IsWorkspacePullDiagnosticsHandler => true;

protected AbstractWorkspacePullDiagnosticsHandler(
LspWorkspaceManager workspaceManager,
LspWorkspaceRegistrationService registrationService,
Expand Down
48 changes: 15 additions & 33 deletions src/Features/LanguageServer/ProtocolUnitTests/Diagnostics/PullDiagnosticTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1347,14 +1347,8 @@ public async Task TestNoChangeIfWorkspaceDiagnosticsCalledTwice(bool useVSDiagno

var results2 = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: CreateDiagnosticParamsFromPreviousReports(results));

Assert.Equal(3, results2.Length);
Assert.Null(results2[0].Diagnostics);
Assert.Null(results2[1].Diagnostics);
Assert.Null(results2[2].Diagnostics);

Assert.Equal(results[0].ResultId, results2[0].ResultId);
Assert.Equal(results[1].ResultId, results2[1].ResultId);
Assert.Equal(results[2].ResultId, results2[2].ResultId);
// 'no changes' will be reported as an empty array.
Assert.Empty(results2);
}

[Theory, CombinatorialData]
Expand Down Expand Up @@ -1657,19 +1651,14 @@ public class {|caret:|} { }
var previousResultIds = CreateDiagnosticParamsFromPreviousReports(results);
results = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResultIds);
AssertEx.NotNull(results);
Assert.Equal(4, results.Length);

// Verify the diagnostic result for A.cs is unchanged as A.cs does not reference CSProj2.
Assert.Null(results[0].Diagnostics);
Assert.Equal(previousResultIds[0].resultId, results[0].ResultId);
Assert.Null(results[1].Diagnostics);
Assert.Equal(previousResultIds[1].resultId, results[1].ResultId);
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.
Assert.Empty(results[2].Diagnostics);
Assert.NotEqual(previousResultIds[2].resultId, results[2].ResultId);
Assert.Empty(results[3].Diagnostics);
Assert.NotEqual(previousResultIds[3].resultId, results[3].ResultId);
Assert.Empty(results[0].Diagnostics);
Assert.NotEqual(previousResultIds[2].resultId, results[0].ResultId);
Assert.Empty(results[1].Diagnostics);
Assert.NotEqual(previousResultIds[3].resultId, results[1].ResultId);
}

[Theory, CombinatorialData]
Expand Down Expand Up @@ -1728,7 +1717,7 @@ public class {|caret:|} { }
}

[Theory, CombinatorialData]
public async Task TestWorkspaceDiagnosticsWithDependentProjectReloadedUnChanged(bool useVSDiagnostics, bool mutatingLspWorkspace)
public async Task TestWorkspaceDiagnosticsWithDependentProjectReloadedUnchanged(bool useVSDiagnostics, bool mutatingLspWorkspace)
{
var markup1 =
@"namespace M
Expand Down Expand Up @@ -1774,14 +1763,10 @@ public class {|caret:|} { }
results = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: previousResultIds);

// Verify that since no actual changes have been made we report unchanged diagnostics.
// We get an empty array here as this is workspace diagnostics, and we do not report unchanged
// docs there for efficiency.
AssertEx.NotNull(results);
Assert.Equal(4, results.Length);

// Diagnostics should be unchanged as the referenced project was only unloaded / reloaded, but did not actually change.
Assert.Null(results[0].Diagnostics);
Assert.Equal(previousResultIds[0].resultId, results[0].ResultId);
Assert.Null(results[2].Diagnostics);
Assert.Equal(previousResultIds[2].resultId, results[2].ResultId);
Assert.Empty(results);
}

[Theory, CombinatorialData]
Expand Down Expand Up @@ -1830,13 +1815,10 @@ class A : B { }
results = await RunGetWorkspacePullDiagnosticsAsync(testLspServer, useVSDiagnostics, previousResults: previousResults);

// Verify that since no actual changes have been made we report unchanged diagnostics.
// We get an empty array here as this is workspace diagnostics, and we do not report unchanged
// docs there for efficiency.
AssertEx.NotNull(results);
Assert.Equal(6, results.Length);

// Diagnostics should be unchanged as a referenced project was unloaded and reloaded. Order should not matter.
Assert.Null(results[0].Diagnostics);
Assert.All(results, result => Assert.Null(result.Diagnostics));
Assert.All(results, result => Assert.True(previousResultIds.Contains(result.ResultId)));
Assert.Empty(results);
}

[Theory, CombinatorialData]
Expand Down
Loading