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

Implement MEF based pull diagnostics #73073

Merged
merged 27 commits into from
Apr 25, 2024
Merged
Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
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 @@ -8,6 +8,7 @@
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.LanguageServer;
using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics;
using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.DiagnosticSources;
using Microsoft.CodeAnalysis.Options;

namespace Microsoft.CodeAnalysis.ExternalAccess.VSTypeScript;
Expand All @@ -17,8 +18,9 @@ namespace Microsoft.CodeAnalysis.ExternalAccess.VSTypeScript;
[method: Obsolete(MefConstruction.ImportingConstructorMessage, error: true)]
internal class VSTypeScriptDocumentPullDiagnosticHandlerFactory(
IDiagnosticAnalyzerService analyzerService,
IDiagnosticSourceManager diagnosticSourceManager,
IDiagnosticsRefresher diagnosticsRefresher,
IGlobalOptionService globalOptions) : DocumentPullDiagnosticHandlerFactory(analyzerService, diagnosticsRefresher, globalOptions)
IGlobalOptionService globalOptions) : DocumentPullDiagnosticHandlerFactory(analyzerService, diagnosticSourceManager, diagnosticsRefresher, globalOptions)
{
}

Expand All @@ -28,7 +30,8 @@ internal class VSTypeScriptDocumentPullDiagnosticHandlerFactory(
internal class VSTypeScriptWorkspacePullDiagnosticHandler(
LspWorkspaceRegistrationService registrationService,
IDiagnosticAnalyzerService analyzerService,
IDiagnosticSourceManager diagnosticSourceManager,
IDiagnosticsRefresher diagnosticsRefresher,
IGlobalOptionService globalOptions) : WorkspacePullDiagnosticHandlerFactory(registrationService, analyzerService, diagnosticsRefresher, globalOptions)
IGlobalOptionService globalOptions) : WorkspacePullDiagnosticHandlerFactory(registrationService, analyzerService, diagnosticSourceManager, diagnosticsRefresher, globalOptions)
{
}
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
using Microsoft.CodeAnalysis.Host.Mef;
using Microsoft.CodeAnalysis.LanguageServer;
using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics;
using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.DiagnosticSources;
using Microsoft.CodeAnalysis.LanguageServer.Handler.SemanticTokens;
using Microsoft.CodeAnalysis.Options;
using Microsoft.VisualStudio.Composition;
Expand Down Expand Up @@ -40,9 +41,11 @@ internal class AlwaysActivateInProcLanguageClient(
ILspServiceLoggerFactory lspLoggerFactory,
IThreadingContext threadingContext,
ExportProvider exportProvider,
IDiagnosticSourceManager diagnosticSourceManager,
[ImportMany] IEnumerable<Lazy<ILspBuildOnlyDiagnostics, ILspBuildOnlyDiagnosticsMetadata>> buildOnlyDiagnostics) : AbstractInProcLanguageClient(lspServiceProvider, globalOptions, lspLoggerFactory, threadingContext, exportProvider)
{
private readonly ExperimentalCapabilitiesProvider _experimentalCapabilitiesProvider = defaultCapabilitiesProvider;
private readonly IDiagnosticSourceManager _diagnosticSourceManager = diagnosticSourceManager;
private readonly IEnumerable<Lazy<ILspBuildOnlyDiagnostics, ILspBuildOnlyDiagnosticsMetadata>> _buildOnlyDiagnostics = buildOnlyDiagnostics;

protected override ImmutableArray<string> SupportedLanguages => ProtocolConstants.RoslynLspLanguages;
Expand All @@ -69,28 +72,15 @@ public override ServerCapabilities GetCapabilities(ClientCapabilities clientCapa

serverCapabilities.SupportsDiagnosticRequests = true;
serverCapabilities.DiagnosticProvider ??= new();

// VS does not distinguish between document and workspace diagnostics, so we need to merge them.
var diagnosticSourceNames = _diagnosticSourceManager.GetSourceNames(true)
etvorun marked this conversation as resolved.
Show resolved Hide resolved
.Concat(_diagnosticSourceManager.GetSourceNames(false))
.Distinct();
serverCapabilities.DiagnosticProvider = serverCapabilities.DiagnosticProvider with
{
SupportsMultipleContextsDiagnostics = true,
DiagnosticKinds =
[
// Support a specialized requests dedicated to task-list items. This way the client can ask just
// for these, independently of other diagnostics. They can also throttle themselves to not ask if
// the task list would not be visible.
new(PullDiagnosticCategories.Task),
new(PullDiagnosticCategories.EditAndContinue),
// Dedicated request for workspace-diagnostics only. We will only respond to these if FSA is on.
new(PullDiagnosticCategories.WorkspaceDocumentsAndProject),
// Fine-grained diagnostics requests. Importantly, this separates out syntactic vs semantic
// requests, allowing the former to quickly reach the user without blocking on the latter. In a
// similar vein, compiler diagnostics are explicitly distinct from analyzer-diagnostics, allowing
// the former to appear as soon as possible as they are much more critical for the user and should
// not be delayed by a slow analyzer.
new(PullDiagnosticCategories.DocumentCompilerSyntax),
new(PullDiagnosticCategories.DocumentCompilerSemantic),
new(PullDiagnosticCategories.DocumentAnalyzerSyntax),
new(PullDiagnosticCategories.DocumentAnalyzerSemantic),
],
DiagnosticKinds = diagnosticSourceNames.Select(n => new VSInternalDiagnosticKind(n)).ToArray(),
BuildOnlyDiagnosticIds = _buildOnlyDiagnostics
.SelectMany(lazy => lazy.Metadata.BuildOnlyDiagnostics)
.Distinct()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.ExternalAccess.Razor" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.ExternalAccess.Razor.UnitTests" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.ExternalAccess.UnitTesting" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.ExternalAccess.VisualDiagnostics" />
etvorun marked this conversation as resolved.
Show resolved Hide resolved
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.ExternalAccess.Xaml" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.Features.DiagnosticsTests.Utilities" />
<InternalsVisibleTo Include="Microsoft.CodeAnalysis.Features.Test.Utilities" />
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
{
InterFileDependencies = true,
WorkDoneProgress = true,
WorkspaceDiagnostics = true,
};
}
etvorun marked this conversation as resolved.
Show resolved Hide resolved

return capabilities;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,6 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.FindUsages;
using Microsoft.CodeAnalysis.LanguageServer.Handler;
using Microsoft.CodeAnalysis.PooledObjects;
using Microsoft.CodeAnalysis.Shared.Collections;
using Microsoft.CodeAnalysis.Shared.Extensions;
using Microsoft.CodeAnalysis.Text;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ private sealed class OpenDocumentSource(Document document) : AbstractDocumentDia
public override bool IsLiveSource()
=> true;

public override async Task<ImmutableArray<DiagnosticData>> GetDiagnosticsAsync(IDiagnosticAnalyzerService diagnosticAnalyzerService, RequestContext context, CancellationToken cancellationToken)
public override async Task<ImmutableArray<DiagnosticData>> GetDiagnosticsAsync(RequestContext context, CancellationToken cancellationToken)
{
var designTimeDocument = Document;
var designTimeSolution = designTimeDocument.Project.Solution;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ private sealed class ProjectSource(Project project, ImmutableArray<DiagnosticDat
public override bool IsLiveSource()
=> true;

public override Task<ImmutableArray<DiagnosticData>> GetDiagnosticsAsync(IDiagnosticAnalyzerService diagnosticAnalyzerService, RequestContext context, CancellationToken cancellationToken)
public override Task<ImmutableArray<DiagnosticData>> GetDiagnosticsAsync(RequestContext context, CancellationToken cancellationToken)
=> Task.FromResult(diagnostics);
}

Expand All @@ -31,7 +31,7 @@ private sealed class ClosedDocumentSource(TextDocument document, ImmutableArray<
public override bool IsLiveSource()
=> true;

public override Task<ImmutableArray<DiagnosticData>> GetDiagnosticsAsync(IDiagnosticAnalyzerService diagnosticAnalyzerService, RequestContext context, CancellationToken cancellationToken)
public override Task<ImmutableArray<DiagnosticData>> GetDiagnosticsAsync(RequestContext context, CancellationToken cancellationToken)
=> Task.FromResult(diagnostics);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using System.Threading;
using System.Threading.Tasks;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics;

internal abstract class AbstractDocumentDiagnosticSourceProvider(string name) : IDiagnosticSourceProvider
{
public bool IsDocument => true;
public string Name => name;

public abstract ValueTask<ImmutableArray<IDiagnosticSource>> CreateDiagnosticSourcesAsync(RequestContext context, CancellationToken cancellationToken);

protected static TextDocument? GetOpenDocument(RequestContext context)
etvorun marked this conversation as resolved.
Show resolved Hide resolved
{
// Note: context.Document may be null in the case where the client is asking about a document that we have
// since removed from the workspace. In this case, we don't really have anything to process.
// GetPreviousResults will be used to properly realize this and notify the client that the doc is gone.
//
// Only consider open documents here (and only closed ones in the WorkspacePullDiagnosticHandler). Each
// handler treats those as separate worlds that they are responsible for.
var textDocument = context.TextDocument;
if (textDocument is null)
{
context.TraceInformation("Ignoring diagnostics request because no text document was provided");
return null;
}

if (!context.IsTracking(textDocument.GetURI()))
{
context.TraceWarning($"Ignoring diagnostics request for untracked document: {textDocument.GetURI()}");
return null;
}

return textDocument;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,34 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Immutable;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Diagnostics;
using Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics.DiagnosticSources;
using Microsoft.CodeAnalysis.Options;
using Microsoft.CommonLanguageServerProtocol.Framework;
using Roslyn.LanguageServer.Protocol;
using LSP = Roslyn.LanguageServer.Protocol;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics;

internal abstract class AbstractDocumentPullDiagnosticHandler<TDiagnosticsParams, TReport, TReturn>(
IDiagnosticAnalyzerService diagnosticAnalyzerService,
IDiagnosticsRefresher diagnosticRefresher,
IDiagnosticSourceManager diagnosticSourceManager,
IGlobalOptionService globalOptions)
: AbstractPullDiagnosticHandler<TDiagnosticsParams, TReport, TReturn>(
diagnosticAnalyzerService,
diagnosticRefresher,
globalOptions), ITextDocumentIdentifierHandler<TDiagnosticsParams, TextDocumentIdentifier?>
where TDiagnosticsParams : IPartialResultParams<TReport>
{
public abstract LSP.TextDocumentIdentifier? GetTextDocumentIdentifier(TDiagnosticsParams diagnosticsParams);
protected readonly IDiagnosticSourceManager DiagnosticSourceManager = diagnosticSourceManager;

protected override ValueTask<ImmutableArray<IDiagnosticSource>> GetOrderedDiagnosticSourcesAsync(TDiagnosticsParams diagnosticsParams, string? requestDiagnosticCategory, RequestContext context, CancellationToken cancellationToken)
{
return DiagnosticSourceManager.CreateDiagnosticSourcesAsync(context, requestDiagnosticCategory, isDocument: true, cancellationToken);
}

public abstract TextDocumentIdentifier? GetTextDocumentIdentifier(TDiagnosticsParams diagnosticsParams);
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.Collections.Immutable;
Expand Down Expand Up @@ -68,8 +67,6 @@ protected AbstractPullDiagnosticHandler(
GlobalOptions = globalOptions;
}

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

/// <summary>
/// Retrieve the previous results we reported. Used so we can avoid resending data for unchanged files. Also
/// used so we can report which documents were removed and can have all their diagnostics cleared.
Expand All @@ -80,7 +77,7 @@ protected AbstractPullDiagnosticHandler(
/// Returns all the documents that should be processed in the desired order to process them in.
/// </summary>
protected abstract ValueTask<ImmutableArray<IDiagnosticSource>> GetOrderedDiagnosticSourcesAsync(
TDiagnosticsParams diagnosticsParams, RequestContext context, CancellationToken cancellationToken);
TDiagnosticsParams diagnosticsParams, string? requestDiagnosticCategory, RequestContext context, CancellationToken cancellationToken);

/// <summary>
/// Creates the appropriate LSP type to report a new set of diagnostics and resultId.
Expand All @@ -106,7 +103,7 @@ protected abstract ValueTask<ImmutableArray<IDiagnosticSource>> GetOrderedDiagno
/// </summary>
protected abstract DiagnosticTag[] ConvertTags(DiagnosticData diagnosticData, bool isLiveSource);

protected abstract string? GetDiagnosticCategory(TDiagnosticsParams diagnosticsParams);
protected abstract string? GetRequestDiagnosticCategory(TDiagnosticsParams diagnosticsParams);

/// <summary>
/// Used by public workspace pull diagnostics to allow it to keep the connection open until
Expand Down Expand Up @@ -134,9 +131,8 @@ protected virtual Task WaitForChangesAsync(RequestContext context, CancellationT
Contract.ThrowIfNull(context.Solution);

var clientCapabilities = context.GetRequiredClientCapabilities();
var category = GetDiagnosticCategory(diagnosticsParams) ?? "";
var sourceIdentifier = GetDiagnosticSourceIdentifier(diagnosticsParams) ?? "";
etvorun marked this conversation as resolved.
Show resolved Hide resolved
var handlerName = $"{this.GetType().Name}(category: {category}, source: {sourceIdentifier})";
var category = GetRequestDiagnosticCategory(diagnosticsParams);
var handlerName = $"{this.GetType().Name}(category: {category})";
context.TraceInformation($"{handlerName} started getting diagnostics");

var versionedCache = _categoryToVersionedCache.GetOrAdd(handlerName, static handlerName => new(handlerName));
Expand All @@ -160,7 +156,7 @@ protected virtual Task WaitForChangesAsync(RequestContext context, CancellationT
// 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.
var orderedSources = await GetOrderedDiagnosticSourcesAsync(
diagnosticsParams, context, cancellationToken).ConfigureAwait(false);
diagnosticsParams, category, context, cancellationToken).ConfigureAwait(false);

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

Expand Down Expand Up @@ -295,7 +291,7 @@ private async Task ComputeAndReportCurrentDiagnosticsAsync(
CancellationToken cancellationToken)
{
using var _ = ArrayBuilder<LSP.Diagnostic>.GetInstance(out var result);
var diagnostics = await diagnosticSource.GetDiagnosticsAsync(DiagnosticAnalyzerService, context, cancellationToken).ConfigureAwait(false);
var diagnostics = await diagnosticSource.GetDiagnosticsAsync(context, cancellationToken).ConfigureAwait(false);

// If we can't get a text document identifier we can't report diagnostics for this source.
// This can happen for 'fake' projects (e.g. used for TS script blocks).
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.

using System.Collections.Generic;
using System.Collections.Immutable;
using System.Linq;
using System.Threading;
using System.Threading.Tasks;
using Microsoft.CodeAnalysis.Host;
using Roslyn.Utilities;

namespace Microsoft.CodeAnalysis.LanguageServer.Handler.Diagnostics;

internal abstract class AbstractWorkspaceDiagnosticSourceProvider(string name) : IDiagnosticSourceProvider
Copy link
Member

Choose a reason for hiding this comment

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

not sure how i feel about abstract types here. For example, i woudl not want disparate features (TODO/EnC) subclassing these to get functionality. I would be ok with them implementing the interfaces fully, and then having helper static types with utilities they could benefit from (like GetProjectsInPriorityOrder).

{
public bool IsDocument => false;
public string Name => name;

public abstract ValueTask<ImmutableArray<IDiagnosticSource>> CreateDiagnosticSourcesAsync(RequestContext context, CancellationToken cancellationToken);

protected static bool ShouldIgnoreContext(RequestContext context)
etvorun marked this conversation as resolved.
Show resolved Hide resolved
{
// If we're being called from razor, we do not support WorkspaceDiagnostics at all. For razor, workspace
// diagnostics will be handled by razor itself, which will operate by calling into Roslyn and asking for
// document-diagnostics instead.
return context.ServerKind == WellKnownLspServerKinds.RazorLspServer;
}

protected static IEnumerable<Project> GetProjectsInPriorityOrder(
Solution solution, ImmutableArray<string> supportedLanguages)
etvorun marked this conversation as resolved.
Show resolved Hide resolved
{
return GetProjectsInPriorityOrderWorker(solution)
.WhereNotNull()
.Distinct()
.Where(p => supportedLanguages.Contains(p.Language));

static IEnumerable<Project?> GetProjectsInPriorityOrderWorker(Solution solution)
{
var documentTrackingService = solution.Services.GetRequiredService<IDocumentTrackingService>();

// Collect all the documents from the solution in the order we'd like to get diagnostics for. This will
// prioritize the files from currently active projects, but then also include all other docs in all projects
// (depending on current FSA settings).

var activeDocument = documentTrackingService.GetActiveDocument(solution);
var visibleDocuments = documentTrackingService.GetVisibleDocuments(solution);

yield return activeDocument?.Project;
foreach (var doc in visibleDocuments)
yield return doc.Project;

foreach (var project in solution.Projects)
yield return project;
}
}

protected static bool ShouldSkipDocument(RequestContext context, TextDocument document)
{
// Only consider closed documents here (and only open ones in the DocumentPullDiagnosticHandler).
// Each handler treats those as separate worlds that they are responsible for.
if (context.IsTracking(document.GetURI()))
{
context.TraceInformation($"Skipping tracked document: {document.GetURI()}");
return true;
}

// Do not attempt to get workspace diagnostics for Razor files, Razor will directly ask us for document diagnostics
// for any razor file they are interested in.
return document.IsRazorDocument();
}
}
Loading
Loading