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

Fix issues with pull diagnostics in VSCode #73248

Merged
merged 6 commits into from
Apr 27, 2024
Merged
Show file tree
Hide file tree
Changes from 3 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 @@ -74,8 +74,8 @@ public override ServerCapabilities GetCapabilities(ClientCapabilities clientCapa
serverCapabilities.DiagnosticProvider ??= new();

// VS does not distinguish between document and workspace diagnostics, so we need to merge them.
var diagnosticSourceNames = _diagnosticSourceManager.GetDocumentSourceProviderNames()
.Concat(_diagnosticSourceManager.GetWorkspaceSourceProviderNames())
var diagnosticSourceNames = _diagnosticSourceManager.GetDocumentSourceProviderNames(clientCapabilities)
.Concat(_diagnosticSourceManager.GetWorkspaceSourceProviderNames(clientCapabilities))
.Distinct();
serverCapabilities.DiagnosticProvider = serverCapabilities.DiagnosticProvider with
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -120,16 +120,6 @@ public ServerCapabilities GetCapabilities(ClientCapabilities clientCapabilities)
// Using VS server capabilities because we have our own custom client.
capabilities.OnAutoInsertProvider = new VSInternalDocumentOnAutoInsertOptions { TriggerCharacters = ["'", "/", "\n"] };

if (!supportsVsExtensions)
{
capabilities.DiagnosticOptions = new DiagnosticOptions
Copy link
Member Author

@dibarbet dibarbet Apr 26, 2024

Choose a reason for hiding this comment

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

this was the root cause of duplicate diagnostics in vscode - we were registering for requests with no identifier and also for requests with an identifier later on in dynamic registration

{
InterFileDependencies = true,
WorkDoneProgress = true,
WorkspaceDiagnostics = true,
};
}

return capabilities;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ public DiagnosticSourceManager([ImportMany] IEnumerable<IDiagnosticSourceProvide
.ToImmutableDictionary(kvp => kvp.Name, kvp => kvp);
}

public ImmutableArray<string> GetDocumentSourceProviderNames()
=> _nameToDocumentProviderMap.Keys.ToImmutableArray();
public ImmutableArray<string> GetDocumentSourceProviderNames(ClientCapabilities clientCapabilities)
Copy link
Member Author

Choose a reason for hiding this comment

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

Allows us to filter the diagnostic sources based on the client (e.g. no task diagnostics in vscode).

=> _nameToDocumentProviderMap.Where(kvp => kvp.Value.IsEnabled(clientCapabilities)).SelectAsArray(kvp => kvp.Key);

public ImmutableArray<string> GetWorkspaceSourceProviderNames()
=> _nameToWorkspaceProviderMap.Keys.ToImmutableArray();
public ImmutableArray<string> GetWorkspaceSourceProviderNames(ClientCapabilities clientCapabilities)
=> _nameToWorkspaceProviderMap.Where(kvp => kvp.Value.IsEnabled(clientCapabilities)).SelectAsArray(kvp => kvp.Key);

public ValueTask<ImmutableArray<IDiagnosticSource>> CreateDocumentDiagnosticSourcesAsync(RequestContext context, string? providerName, CancellationToken cancellationToken)
=> CreateDiagnosticSourcesAsync(context, providerName, _nameToDocumentProviderMap, isDocument: true, cancellationToken);
Expand All @@ -69,7 +69,10 @@ private static async ValueTask<ImmutableArray<IDiagnosticSource>> CreateDiagnost
// VS does not distinguish between document and workspace sources. Thus it can request
// document diagnostics with workspace source name. We need to handle this case.
if (nameToProviderMap.TryGetValue(providerName, out var provider))
{
Contract.ThrowIfFalse(provider.IsEnabled(context.GetRequiredClientCapabilities()));
return await provider.CreateDiagnosticSourcesAsync(context, cancellationToken).ConfigureAwait(false);
}

return [];
}
Expand All @@ -79,12 +82,13 @@ private static async ValueTask<ImmutableArray<IDiagnosticSource>> CreateDiagnost
using var _ = ArrayBuilder<IDiagnosticSource>.GetInstance(out var sourcesBuilder);
foreach (var (name, provider) in nameToProviderMap)
{
// Exclude Task diagnostics from the aggregated sources.
if (name != PullDiagnosticCategories.Task)
if (!provider.IsEnabled(context.GetRequiredClientCapabilities()))
{
var namedSources = await provider.CreateDiagnosticSourcesAsync(context, cancellationToken).ConfigureAwait(false);
sourcesBuilder.AddRange(namedSources);
continue;
}

var namedSources = await provider.CreateDiagnosticSourcesAsync(context, cancellationToken).ConfigureAwait(false);
sourcesBuilder.AddRange(namedSources);
}

var sources = sourcesBuilder.ToImmutableAndClear();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.Host.Mef;
using Roslyn.LanguageServer.Protocol;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics;

Expand All @@ -19,6 +20,8 @@ internal abstract class AbstractDocumentSyntaxAndSemanticDiagnosticSourceProvide
public bool IsDocument => true;
public string Name => sourceName;

public bool IsEnabled(ClientCapabilities clientCapabilities) => true;

public ValueTask<ImmutableArray<IDiagnosticSource>> CreateDiagnosticSourcesAsync(RequestContext context, CancellationToken cancellationToken)
{
if (context.GetTrackedDocument<TextDocument>() is { } document)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Immutable;
using System.Threading;
using System.Threading.Tasks;
using Roslyn.LanguageServer.Protocol;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.DiagnosticSources;

Expand All @@ -17,12 +18,12 @@ internal interface IDiagnosticSourceManager
/// <summary>
/// Returns the names of document level <see cref="IDiagnosticSourceProvider"/>s.
/// </summary>
ImmutableArray<string> GetDocumentSourceProviderNames();
ImmutableArray<string> GetDocumentSourceProviderNames(ClientCapabilities clientCapabilities);

/// <summary>
/// Returns the names of workspace level <see cref="IDiagnosticSourceProvider"/>s.
/// </summary>
ImmutableArray<string> GetWorkspaceSourceProviderNames();
ImmutableArray<string> GetWorkspaceSourceProviderNames(ClientCapabilities clientCapabilities);

/// <summary>
/// Creates document diagnostic sources for the given <paramref name="providerName"/>.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Collections.Immutable;
using System.Threading;
using System.Threading.Tasks;
using Roslyn.LanguageServer.Protocol;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics;

Expand All @@ -23,6 +24,8 @@ internal interface IDiagnosticSourceProvider
/// </summary>
string Name { get; }

bool IsEnabled(ClientCapabilities clientCapabilities);

/// <summary>
/// Creates the diagnostic sources.
/// </summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.SolutionCrawler;
using Roslyn.LanguageServer.Protocol;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics;
Expand All @@ -28,6 +29,8 @@ internal sealed class WorkspaceDocumentsAndProjectDiagnosticSourceProvider(
public bool IsDocument => false;
public string Name => PullDiagnosticCategories.WorkspaceDocumentsAndProject;

public bool IsEnabled(ClientCapabilities clientCapabilities) => true;

/// <summary>
/// There are three potential sources for reporting workspace diagnostics:
///
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.SolutionCrawler;
using Roslyn.LanguageServer.Protocol;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.Public;

Expand All @@ -26,6 +27,8 @@ internal sealed class PublicDocumentNonLocalDiagnosticSourceProvider(
public bool IsDocument => true;
public string Name => NonLocal;

public bool IsEnabled(ClientCapabilities clientCapabilities) => true;

public ValueTask<ImmutableArray<IDiagnosticSource>> CreateDiagnosticSourcesAsync(RequestContext context, CancellationToken cancellationToken)
{
// Non-local document diagnostics are reported only when full solution analysis is enabled for analyzer execution.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Threading;
using System.Threading.Tasks;
using Roslyn.LanguageServer.Protocol;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.Public;
// A document diagnostic partial report is defined as having the first literal send = DocumentDiagnosticReport (aka changed / unchanged) followed
Expand All @@ -17,15 +18,29 @@ internal sealed partial class PublicDocumentPullDiagnosticsHandler : IOnInitiali
{
public async Task OnInitializedAsync(ClientCapabilities clientCapabilities, RequestContext context, CancellationToken cancellationToken)
{
// Dynamically register for all of our document diagnostic sources.
// Dynamically register for all relevant diagnostic sources.
if (clientCapabilities?.TextDocument?.Diagnostic?.DynamicRegistration is true)
{
// TODO: Hookup an option changed handler for changes to BackgroundAnalysisScopeOption
// to dynamically register/unregister the non-local document diagnostic source.

// Task diagnostics shouldn't be reported through VSCode (it has its own task stuff). Additional cleanup needed.
var sources = DiagnosticSourceManager.GetDocumentSourceProviderNames().Where(source => source != PullDiagnosticCategories.Task);
var registrations = sources.Select(FromSourceName).ToArray();
var documentSources = DiagnosticSourceManager.GetDocumentSourceProviderNames(clientCapabilities);
var workspaceSources = DiagnosticSourceManager.GetWorkspaceSourceProviderNames(clientCapabilities);

// All diagnostic sources have to be registered under the document pull method name,
// See https://github.com/microsoft/language-server-protocol/issues/1723
//
// Additionally if a source name is used by both document and workspace pull (e.g. enc)
// we don't want to send two registrations, instead we should send a single registration
// that also sets the workspace pull option.
//
// So we build up a unique set of source names and mark if each one is also a workspace source.
var allSources = documentSources
Copy link
Member Author

@dibarbet dibarbet Apr 26, 2024

Choose a reason for hiding this comment

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

previously the doc handler registered doc sources and the workspace handler registered workspace sources. However this meant that sources using the same identifer for doc and workspace would overwrite each other - because vscode requires us to register all identifiers as document sources (even if they are workspace only).

now we register them all once (and set the workspace option appropriately if it also participates in workspace requests).

.AddRange(workspaceSources)
.ToSet()
.Select(name => (Name: name, IsWorkspaceSource: workspaceSources.Contains(name)));

var registrations = allSources.Select(FromSourceName).ToArray();
await _clientLanguageServerManager.SendRequestAsync(
methodName: Methods.ClientRegisterCapabilityName,
@params: new RegistrationParams()
Expand All @@ -35,12 +50,14 @@ await _clientLanguageServerManager.SendRequestAsync(
cancellationToken).ConfigureAwait(false);
}

Registration FromSourceName(string sourceName)
=> new()
static Registration FromSourceName((string Name, bool IsWorkspaceSource) source)
{
return new()
{
Id = Guid.NewGuid().ToString(),
Method = Methods.TextDocumentDiagnosticName,
RegisterOptions = new DiagnosticRegistrationOptions { Identifier = sourceName }
RegisterOptions = new DiagnosticRegistrationOptions { Identifier = source.Name, InterFileDependencies = true, WorkspaceDiagnostics = source.IsWorkspaceSource, WorkDoneProgress = source.IsWorkspaceSource }
};
}
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.EditAndContinue;
using Microsoft.CodeAnalysis.Host.Mef;
using Roslyn.LanguageServer.Protocol;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics;

Expand All @@ -20,6 +21,8 @@ internal sealed class DocumentEditAndContinueDiagnosticSourceProvider() : IDiagn
public bool IsDocument => true;
public string Name => PullDiagnosticCategories.EditAndContinue;

public bool IsEnabled(ClientCapabilities capabilities) => true;

public ValueTask<ImmutableArray<IDiagnosticSource>> CreateDiagnosticSourcesAsync(RequestContext context, CancellationToken cancellationToken)
{
if (context.GetTrackedDocument<Document>() is { } document)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.EditAndContinue;
using Microsoft.CodeAnalysis.Host.Mef;
using Roslyn.LanguageServer.Protocol;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics;
Expand All @@ -21,6 +22,8 @@ internal sealed class WorkspaceEditAndContinueDiagnosticSourceProvider() : IDiag
public bool IsDocument => false;
public string Name => PullDiagnosticCategories.EditAndContinue;

public bool IsEnabled(ClientCapabilities capabilities) => true;

public ValueTask<ImmutableArray<IDiagnosticSource>> CreateDiagnosticSourcesAsync(RequestContext context, CancellationToken cancellationToken)
{
Contract.ThrowIfNull(context.Solution);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.Options;
using Roslyn.LanguageServer.Protocol;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.Tasks;

Expand All @@ -20,6 +21,8 @@ internal sealed class DocumentTaskDiagnosticSourceProvider([Import] IGlobalOptio
public bool IsDocument => true;
public string Name => PullDiagnosticCategories.Task;

public bool IsEnabled(ClientCapabilities capabilities) => capabilities.HasVisualStudioLspCapability();

public ValueTask<ImmutableArray<IDiagnosticSource>> CreateDiagnosticSourcesAsync(RequestContext context, CancellationToken cancellationToken)
{
if (context.GetTrackedDocument<Document>() is { } document)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
using Microsoft.CodeAnalysis.Options;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.TaskList;
using Roslyn.LanguageServer.Protocol;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.Tasks;
Expand All @@ -23,6 +24,8 @@ internal sealed class WorkspaceTaskDiagnosticSourceProvider([Import] IGlobalOpti
public bool IsDocument => false;
public string Name => PullDiagnosticCategories.Task;

public bool IsEnabled(ClientCapabilities capabilities) => capabilities.HasVisualStudioLspCapability();

public ValueTask<ImmutableArray<IDiagnosticSource>> CreateDiagnosticSourcesAsync(RequestContext context, CancellationToken cancellationToken)
{
Contract.ThrowIfNull(context.Solution);
Expand Down
Loading
Loading