Remaining work to get pull diagnostics + LSP fully enabled, and all push-diagnostics + error list code removed.

[CyrusNajmabadi]

1. Need to figure out how someone can configure suppressions from the error list if we don't own the error list.
2. Need to remove IDiagnosticService.DiagnosticsUpdated. Currently this is used by tagger (but is not needed if tagger is just pulling), and by typescript. David believes TS just uses this for LSP-push-diags in liveshare and can likely be removed since we have worked with them to support pull diagnostics.
3. Move IDiagnosticService.GetDiagnosticsAsync to be snapshot based. Currently it takes in Workspace+ProjId/DocId. This is non-sensical as that means you may get random results depending on the state of Workspace.CurrentSnapshot. This needs to move to take Project/Document instead.
4. Need LSP pull diagnostics to be category-based, so we can report syntax before semantics.

See also: #62741, #62735 and #62771

[CyrusNajmabadi]

@tmat @mavasani For visibility. We're very close on this :)

[tmat]

Cool. My experience with pull diagnostics in Roslyn.sln is not great though. They seem to be lagging quite a bit. That needs to be addressed imo, before we switch.

[CyrusNajmabadi]

Cool. My experience with pull diagnostics in Roslyn.sln is not great though. They seem to be lagging quite a bit. That needs to be addressed imo, before we switch.

So far i've found them pretty good. We identified a definite problem in that syntactic diagnostics are gated on semantics diagnostics being computed. This can def make things feel slower as clearly broken code still takes a while to appear if semantics are taking a bit of time. This is being tracked with: #62560

One thing i'm not clear on is if the diagnostics subsystem properly does minimal pull-diagnostics computation when a method body edit happens. @mavasani to confirm if we have that optimization, or if we need to file a tracking issue to make that that works as well for semantic diagnostics.

[dibarbet]

@tmat are you still hitting the issue where workspace diagnostics are being requested even with FSA off? I sent a message on it - but was curious if it was possible to debug through it on your machine (I'm unable to get the same behavior on my machine).

[tmat]

I haven't had chance to do much work in Roslyn.sln since.

[mavasani]

One thing i'm not clear on is if the diagnostics subsystem properly does minimal pull-diagnostics computation when a method body edit happens. @mavasani to confirm if we have that optimization, or if we need to file a tracking issue to make that that works as well for semantic diagnostics.

This optimization is part of the solution crawler, so I wouldn't assume it to be automatically enabled for pull diagnostics unless similar logic is added to LSP handler. Currently, the LSP handler requests diagnostics for the entire document:

roslyn/src/Features/LanguageServer/Protocol/Handler/Diagnostics/DocumentPullDiagnosticHandler.cs

Lines 112 to 118 in 5030cd4

	public async Task<ImmutableArray<DiagnosticData>> GetDiagnosticsAsync(IDiagnosticAnalyzerService diagnosticAnalyzerService, RequestContext context, DiagnosticMode diagnosticMode, CancellationToken cancellationToken)
	{
	// We call GetDiagnosticsForSpanAsync here instead of GetDiagnosticsForIdsAsync as it has faster perf characteristics.
	// GetDiagnosticsForIdsAsync runs analyzers against the entire compilation whereas GetDiagnosticsForSpanAsync will only run analyzers against the request document.
	var allSpanDiagnostics = await diagnosticAnalyzerService.GetDiagnosticsForSpanAsync(Document, range: null, cancellationToken: cancellationToken).ConfigureAwait(false);
	return allSpanDiagnostics;
	}

roslyn/src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.IncrementalAnalyzerProcessor.cs

Lines 188 to 197 in 5030cd4

	if (workItem.MustRefresh || reasons.Contains(PredefinedInvocationReasons.SemanticChanged))
	{
	await RunAnalyzersAsync(analyzers, document, workItem, (analyzer, document, cancellationToken) =>
	analyzer.AnalyzeDocumentAsync(document, null, reasons, cancellationToken), cancellationToken).ConfigureAwait(false);
	}
	else
	{
	// if we don't need to re-analyze whole body, see whether we need to at least re-analyze one method.
	await RunBodyAnalyzersAsync(analyzers, workItem, document, cancellationToken).ConfigureAwait(false);
	}

roslyn/src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.IncrementalAnalyzerProcessor.cs

Lines 267 to 302 in 5030cd4

	private async Task RunBodyAnalyzersAsync(ImmutableArray<IIncrementalAnalyzer> analyzers, WorkItem workItem, Document document, CancellationToken cancellationToken)
	{
	try
	{
	var root = await GetOrDefaultAsync(document, (d, c) => d.GetSyntaxRootAsync(c), cancellationToken).ConfigureAwait(false);
	var syntaxFactsService = document.GetLanguageService<ISyntaxFactsService>();
	var reasons = workItem.InvocationReasons;
	if (root == null || syntaxFactsService == null)
	{
	// as a fallback mechanism, if we can't run one method body due to some missing service, run whole document analyzer.
	await RunAnalyzersAsync(analyzers, document, workItem, (analyzer, document, cancellationToken) =>
	analyzer.AnalyzeDocumentAsync(document, null, reasons, cancellationToken), cancellationToken).ConfigureAwait(false);
	return;
	}
	// check whether we know what body has changed. currently, this is an optimization toward typing case. if there are more than one body changes
	// it will be considered as semantic change and whole document analyzer will take care of that case.
	var activeMember = GetMemberNode(syntaxFactsService, root, workItem.ActiveMember);
	if (activeMember == null)
	{
	// no active member means, change is out side of a method body, but it didn't affect semantics (such as change in comment)
	// in that case, we update whole document (just this document) so that we can have updated locations.
	await RunAnalyzersAsync(analyzers, document, workItem, (analyzer, document, cancellationToken) =>
	analyzer.AnalyzeDocumentAsync(document, null, reasons, cancellationToken), cancellationToken).ConfigureAwait(false);
	return;
	}
	// re-run just the body
	await RunAnalyzersAsync(analyzers, document, workItem, (analyzer, document, cancellationToken) =>
	analyzer.AnalyzeDocumentAsync(document, activeMember, reasons, cancellationToken), cancellationToken).ConfigureAwait(false);
	}
	catch (Exception e) when (FatalError.ReportAndPropagateUnlessCanceled(e, cancellationToken))
	{
	throw ExceptionUtilities.Unreachable;
	}
	}

[mavasani]

@CyrusNajmabadi @dibarbet The core logic to compute whether only method body edits took place from the last snapshot is in the solution crawler over here:

roslyn/src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.cs

Lines 538 to 555 in 5030cd4

	private async Task EnqueueChangedDocumentWorkItemAsync(Document oldDocument, Document newDocument)
	{
	var differenceService = newDocument.GetLanguageService<IDocumentDifferenceService>();
	if (differenceService == null)
	{
	// For languages that don't use a Roslyn syntax tree, they don't export a document difference service.
	// The whole document should be considered as changed in that case.
	await EnqueueDocumentWorkItemAsync(newDocument.Project, newDocument.Id, newDocument, InvocationReasons.DocumentChanged).ConfigureAwait(false);
	}
	else
	{
	var differenceResult = await differenceService.GetDifferenceAsync(oldDocument, newDocument, _shutdownToken).ConfigureAwait(false);
	if (differenceResult != null)
	await EnqueueDocumentWorkItemAsync(newDocument.Project, newDocument.Id, newDocument, differenceResult.ChangeType, differenceResult.ChangedMember).ConfigureAwait(false);
	}
	}

roslyn/src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.cs

Lines 402 to 425 in 5030cd4

	private async Task EnqueueDocumentWorkItemAsync(Project project, DocumentId documentId, TextDocument? document, InvocationReasons invocationReasons, SyntaxNode? changedMember = null)
	{
	// we are shutting down
	_shutdownToken.ThrowIfCancellationRequested();
	var priorityService = project.GetLanguageService<IWorkCoordinatorPriorityService>();
	document ??= project.GetTextDocument(documentId);
	var sourceDocument = document as Document;
	var isLowPriority = priorityService != null && sourceDocument != null && await priorityService.IsLowPriorityAsync(sourceDocument, _shutdownToken).ConfigureAwait(false);
	var currentMember = GetSyntaxPath(changedMember);
	// call to this method is serialized. and only this method does the writing.
	_documentAndProjectWorkerProcessor.Enqueue(
	new WorkItem(documentId, project.Language, invocationReasons, isLowPriority, currentMember, _listener.BeginAsyncOperation("WorkItem")));
	// enqueue semantic work planner
	if (invocationReasons.Contains(PredefinedInvocationReasons.SemanticChanged) && sourceDocument != null)
	{
	// must use "Document" here so that the snapshot doesn't go away. we need the snapshot to calculate p2p dependency graph later.
	// due to this, we might hold onto solution (and things kept alive by it) little bit longer than usual.
	_semanticChangeProcessor.Enqueue(project, documentId, sourceDocument, currentMember);
	}
	}

roslyn/src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.cs

Lines 430 to 440 in 5030cd4

	private static SyntaxPath? GetSyntaxPath(SyntaxNode? changedMember)
	{
	// using syntax path might be too expansive since it will be created on every keystroke.
	// but currently, we have no other way to track a node between two different tree (even for incrementally parsed one)
	if (changedMember == null)
	{
	return null;
	}
	return new SyntaxPath(changedMember);
	}

[dibarbet]

thanks @mavasani - I think that is an optimization I'll need to port over.

[dibarbet]

@mavasani hmm - in the incremental analyzer implementation, it looks like it ignores the method node we find during the differencing - https://sourceroslyn.io/#Microsoft.CodeAnalysis.LanguageServer.Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs,28

[mavasani]

@mavasani hmm - in the incremental analyzer implementation, it looks like it ignores the method node we find during the differencing - https://sourceroslyn.io/#Microsoft.CodeAnalysis.LanguageServer.Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs,28

Wow, that seems like a big regression. I need to look at the history to figure out when/why this was removed.

[mavasani]

in the incremental analyzer implementation, it looks like it ignores the method node we find during the differencing - https://sourceroslyn.io/#Microsoft.CodeAnalysis.LanguageServer.Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs,28

Tagging @heejaechang for historical context.

I investigated this a bit more and it seems the IDE DiagnosticIncrementalAnalyzer's v1 implementation performed method body based analysis, but this was never implemented for the v2 engine. Below are the implementations from the same repo snapshot:

roslyn/src/Features/Core/Portable/Diagnostics/EngineV1/DiagnosticIncrementalAnalyzer.cs

Lines 278 to 286 in 82c9de9

	if (bodyOpt == null)
	{
	await AnalyzeDocumentAsync(document, versions, diagnosticIds, cancellationToken).ConfigureAwait(false);
	}
	else
	{
	// only open file can go this route
	await AnalyzeBodyDocumentAsync(document, bodyOpt, versions, cancellationToken).ConfigureAwait(false);
	}

roslyn/src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs

Lines 16 to 28 in 82c9de9

	internal partial class DiagnosticIncrementalAnalyzer : BaseDiagnosticIncrementalAnalyzer
	{
	public override Task AnalyzeSyntaxAsync(Document document, CancellationToken cancellationToken)
	{
	return AnalyzeDocumentForKindAsync(document, AnalysisKind.Syntax, cancellationToken);
	}
	public override Task AnalyzeDocumentAsync(Document document, SyntaxNode bodyOpt, CancellationToken cancellationToken)
	{
	return AnalyzeDocumentForKindAsync(document, AnalysisKind.Semantic, cancellationToken);
	}
	private async Task AnalyzeDocumentForKindAsync(Document document, AnalysisKind kind, CancellationToken cancellationToken)

v2 engine was written and replaced the v1 engine about 6 years ago, so this is not a recent behavior change. @heejaechang do you recall if this method body edit optimization was removed due to bugs/flakiness or was it just pending work that never got done? If it is the latter, then it might make sense to only port this optimization to the LSP handler. The good part here is:

1. This would open up scope for quite a decent performance gain when moving to LSP pull diagnostics for method body editing scenarios.
2. This optimization would be an enhancement, rather than a parity requirement for enabling pull diagnostics by default.

I do notice that for the light-bulb path we do use IBuiltInAnalyzer.GetAnalyzerCategory() to only execute span based analyzers for the line span, which is good:

roslyn/src/Features/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_GetDiagnosticsForSpan.cs

Lines 173 to 192 in 3fe6824

	if (!await TryAddCachedDocumentDiagnosticsAsync(stateSet, AnalysisKind.Semantic, list, cancellationToken).ConfigureAwait(false))
	{
	// Check whether we want up-to-date document wide semantic diagnostics
	var spanBased = stateSet.Analyzer.SupportsSpanBasedSemanticDiagnosticAnalysis();
	if (!_blockForData && !spanBased)
	{
	containsFullResult = false;
	}
	else
	{
	var stateSets = spanBased ? semanticSpanBasedAnalyzers : semanticDocumentBasedAnalyzers;
	stateSets.Add(stateSet.Analyzer);
	}
	}
	}
	// Compute diagnostics for non-cached state sets.
	await ComputeDocumentDiagnosticsAsync(syntaxAnalyzers.ToImmutable(), AnalysisKind.Syntax, _range, list, cancellationToken).ConfigureAwait(false);
	await ComputeDocumentDiagnosticsAsync(semanticSpanBasedAnalyzers.ToImmutable(), AnalysisKind.Semantic, _range, list, cancellationToken).ConfigureAwait(false);
	await ComputeDocumentDiagnosticsAsync(semanticDocumentBasedAnalyzers.ToImmutable(), AnalysisKind.Semantic, span: null, list, cancellationToken).ConfigureAwait(false);

[CyrusNajmabadi]

@mavasani in a pull-diags world, if there is any sort of caching, i would assume that adding this optimization back in would be fairly trivial. we should know the Doc/Tree the last pulled diags were against. the next pull can attempt to diff the current tree against the previous one. If teh diff is entirely within a method, we can keep all the diags outside of that, and only recompute what is inside right?

Basically, my expectation is that pull ideally makes this easier to add as an invisible optimization (outside of perf speedup).

[heejaechang]

https://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/SolutionCrawler/WorkCoordinator.IncrementalAnalyzerProcessor.cs,263

this should call the body only analysis for open file?

[sharwell]

If we did end up shipping for 6 years without the method body edit detection enabled, I would encourage we just make that state permanent, since it allows us to finally remove IBuiltInAnalyzer.

[CyrusNajmabadi]

note that in that time we have had an enormous amount of complaints about performance :)

[mavasani]

https://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/SolutionCrawler/WorkCoordinator.IncrementalAnalyzerProcessor.cs,263
this should call the body only analysis for open file?

@heejaechang It does, and solution crawler is fine here. The issue is the DiagnosticIncrementalAnalyzer ignores the body node:

roslyn/src/Features/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs

Lines 28 to 29 in 843f8db

	public Task AnalyzeDocumentAsync(Document document, SyntaxNode bodyOpt, InvocationReasons reasons, CancellationToken cancellationToken)
	=> AnalyzeDocumentForKindAsync(document, AnalysisKind.Semantic, cancellationToken);

If we did end up shipping for 6 years without the method body edit detection enabled, I would encourage we just make that state permanent, since it allows us to finally remove IBuiltInAnalyzer.

@sharwell Note that for light-bulb path we do use IBuiltInAnalyzer.GetAnalyzerCategory() to only execute span based analyzers for the line span:

roslyn/src/Features/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_GetDiagnosticsForSpan.cs

Lines 173 to 192 in 3fe6824

	if (!await TryAddCachedDocumentDiagnosticsAsync(stateSet, AnalysisKind.Semantic, list, cancellationToken).ConfigureAwait(false))
	{
	// Check whether we want up-to-date document wide semantic diagnostics
	var spanBased = stateSet.Analyzer.SupportsSpanBasedSemanticDiagnosticAnalysis();
	if (!_blockForData && !spanBased)
	{
	containsFullResult = false;
	}
	else
	{
	var stateSets = spanBased ? semanticSpanBasedAnalyzers : semanticDocumentBasedAnalyzers;
	stateSets.Add(stateSet.Analyzer);
	}
	}
	}
	// Compute diagnostics for non-cached state sets.
	await ComputeDocumentDiagnosticsAsync(syntaxAnalyzers.ToImmutable(), AnalysisKind.Syntax, _range, list, cancellationToken).ConfigureAwait(false);
	await ComputeDocumentDiagnosticsAsync(semanticSpanBasedAnalyzers.ToImmutable(), AnalysisKind.Semantic, _range, list, cancellationToken).ConfigureAwait(false);
	await ComputeDocumentDiagnosticsAsync(semanticDocumentBasedAnalyzers.ToImmutable(), AnalysisKind.Semantic, span: null, list, cancellationToken).ConfigureAwait(false);

If we remove that logic, we will start executing SimplifyTypeName analyzer on the the entire file every time Ctrl + Dot is invoked. We certainly don't want that.

[CyrusNajmabadi]

I also wouldn't say that we haven't noticed. Perf has been a perpetual problem. It's just never been clear what is at fault. I know that i had to add back smarts to scope thigns back to method bodies to help speed up perf in completion lists for example :) i think we're just not in a good place perf-wise, with lots of components being problematic, so it's easy for regressions to happen that just get added to the pile of problems :)

[heejaechang]

interesting... what I remember is in proc diagnostic analyzer uses method body (since in proc handles open file case) and out of proc doesn't care about method body (since it only handles closed file and compilation diagnostics). and I believe out of proc doesn't even have concept of open or visible files ..

but certainly, a bug could have introduced..

[CyrusNajmabadi]

Closing as this has all been done.