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

Document diagnostics supersede workspace diagnostics #49408

Merged
5 commits merged into from Nov 17, 2020
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Expand Up @@ -24,6 +24,13 @@ namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics
internal abstract class AbstractPullDiagnosticHandler<TDiagnosticsParams, TReport> : IRequestHandler<TDiagnosticsParams, TReport[]?>
where TReport : DiagnosticReport
{
/// <summary>
/// Special value we use to designate workspace diagnostics vs document diagnostics. Document diagnostics
/// should always <see cref="DiagnosticReport.Supersedes"/> a workspace diagnostic as the former are 'live'
/// while the latter are cached and may be stale.
/// </summary>
protected const int WorkspaceDiagnosticIdentifier = 1;

protected readonly IDiagnosticService DiagnosticService;

/// <summary>
Expand Down Expand Up @@ -78,6 +85,11 @@ internal abstract class AbstractPullDiagnosticHandler<TDiagnosticsParams, TRepor
/// </summary>
protected abstract Task<ImmutableArray<DiagnosticData>> GetDiagnosticsAsync(RequestContext context, Document document, Option2<DiagnosticMode> diagnosticMode, CancellationToken cancellationToken);

/// <summary>
/// Generate the right diagnostic tags for a particular diagnostic.
/// </summary>
protected abstract DiagnosticTag[] ConvertTags(DiagnosticData diagnosticData);

private void OnDiagnosticsUpdated(object? sender, DiagnosticsUpdatedArgs updateArgs)
{
if (updateArgs.DocumentId == null)
Expand Down Expand Up @@ -290,7 +302,7 @@ private static LSP.DiagnosticSeverity ConvertDiagnosticSeverity(DiagnosticSeveri
/// If you make change in this method, please also update the corresponding file in
/// src\VisualStudio\Xaml\Impl\Implementation\LanguageServer\Handler\Diagnostics\AbstractPullDiagnosticHandler.cs
/// </summary>
private static DiagnosticTag[] ConvertTags(DiagnosticData diagnosticData)
protected DiagnosticTag[] ConvertTags(DiagnosticData diagnosticData, bool potentialDuplicate)
{
using var _ = ArrayBuilder<DiagnosticTag>.GetInstance(out var result);

Expand All @@ -305,6 +317,9 @@ private static DiagnosticTag[] ConvertTags(DiagnosticData diagnosticData)
result.Add(VSDiagnosticTags.VisibleInErrorList);
}

if (potentialDuplicate)
result.Add(VSDiagnosticTags.PotentialDuplicate);

result.Add(diagnosticData.CustomTags.Contains(WellKnownDiagnosticTags.Build)
? VSDiagnosticTags.BuildError
: VSDiagnosticTags.IntellisenseError);
Expand Down
Expand Up @@ -10,7 +10,6 @@
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.VisualStudio.LanguageServer.Protocol;
using Roslyn.Utilities;

Expand All @@ -35,14 +34,26 @@ internal class DocumentPullDiagnosticHandler : AbstractPullDiagnosticHandler<Doc
=> diagnosticsParams.TextDocument;

protected override DiagnosticReport CreateReport(TextDocumentIdentifier? identifier, VSDiagnostic[]? diagnostics, string? resultId)
=> new DiagnosticReport { Diagnostics = diagnostics, ResultId = resultId };
=> new DiagnosticReport
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we set an Identifier here just in case? I worry that the workspace report technically has Supersedes = 0, and this has an identifier of 0, and whilst I'm sure the platform handles that, it still makes me nervous.

Copy link
Member Author

Choose a reason for hiding this comment

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

sure. i dont' mind being explicit.

{
Diagnostics = diagnostics,
ResultId = resultId,
// Mark these diagnostics as superseding any diagnostics for the same document from the
// WorkspacePullDiagnosticHandler. We are always getting completely accurate and up to date diagnostic
// values for a particular file, so our results should always be preferred over the workspace-pull
// values which are cached and may be out of date.
Supersedes = WorkspaceDiagnosticIdentifier,
};

protected override DiagnosticParams[]? GetPreviousResults(DocumentDiagnosticsParams diagnosticsParams)
=> new[] { diagnosticsParams };

protected override IProgress<DiagnosticReport[]>? GetProgress(DocumentDiagnosticsParams diagnosticsParams)
=> diagnosticsParams.PartialResultToken;

protected override DiagnosticTag[] ConvertTags(DiagnosticData diagnosticData)
=> ConvertTags(diagnosticData, potentialDuplicate: false);

protected override ImmutableArray<Document> GetOrderedDocuments(RequestContext context)
{
// For the single document case, that is the only doc we want to process.
Expand Down
Expand Up @@ -31,14 +31,29 @@ public WorkspacePullDiagnosticHandler(IDiagnosticService diagnosticService)
=> null;

protected override WorkspaceDiagnosticReport CreateReport(TextDocumentIdentifier? identifier, VSDiagnostic[]? diagnostics, string? resultId)
=> new WorkspaceDiagnosticReport { TextDocument = identifier, Diagnostics = diagnostics, ResultId = resultId };
=> new WorkspaceDiagnosticReport
{
TextDocument = identifier,
Diagnostics = diagnostics,
ResultId = resultId,
// Mark these diagnostics as having come from us. They will be superseded by any diagnostics for the
// same file produced by the DocumentPullDiagnosticHandler.
Identifier = WorkspaceDiagnosticIdentifier,
};

protected override IProgress<WorkspaceDiagnosticReport[]>? GetProgress(WorkspaceDocumentDiagnosticsParams diagnosticsParams)
=> diagnosticsParams.PartialResultToken;

protected override DiagnosticParams[]? GetPreviousResults(WorkspaceDocumentDiagnosticsParams diagnosticsParams)
=> diagnosticsParams.PreviousResults;

protected override DiagnosticTag[] ConvertTags(DiagnosticData diagnosticData)
{
// All workspace diagnostics are potential duplicates given that they can be overridden by the diagnostics
// produced by document diagnostics.
return ConvertTags(diagnosticData, potentialDuplicate: true);
}

protected override ImmutableArray<Document> GetOrderedDocuments(RequestContext context)
{
// If we're being called from razor, we do not support WorkspaceDiagnostics at all. For razor, workspace
Expand Down