Run Code Analysis command does not report any violations when LSP pull diagnostics is enabled

[mavasani]

Documentation: https://learn.microsoft.com/visualstudio/code-quality/how-to-run-code-analysis-manually-for-managed-code

Version Used: Latest 17.5 Preview2

Steps to Reproduce:

1. Ensure LSP pull diagnostics is not enabled
2. Open Roslyn.sln, right click on Microsoft.CodeAnalysis.csproj in the solution explorer and run Analyze -> Run Code Analysis
3. Wait for code analysis to complete running
4. Notice 100s of info messages in the Messages tab
5. Now enable LSP pull diagnostics from Tools Options
6. Close and re-open VS
7. Repeat steps 2 and 3

Expected Behavior:
Same set of code analysis message diagnostics are reported as in Step 4.

Actual Behavior:
No code analysis message diagnostics are reported.

Run code analysis command ends up invoking the below method which basically force completes analyzer execution for the selected project/solution node:

roslyn/src/VisualStudio/Core/Def/Diagnostics/VisualStudioDiagnosticAnalyzerService.cs

Lines 304 to 351 in 6392f2f

	public void RunAnalyzers(IVsHierarchy? hierarchy)
	{
	var project = GetProject(hierarchy);
	var solution = _workspace.CurrentSolution;
	var projectOrSolutionName = project?.Name ?? PathUtilities.GetFileName(solution.FilePath);
	bool isAnalysisDisabled;
	if (project != null)
	{
	isAnalysisDisabled = _globalOptions.IsAnalysisDisabled(project.Language);
	}
	else
	{
	isAnalysisDisabled = true;
	foreach (var language in solution.Projects.Select(p => p.Language).Distinct())
	{
	isAnalysisDisabled = isAnalysisDisabled && _globalOptions.IsAnalysisDisabled(language);
	}
	}
	// Add a message to VS status bar that we are running code analysis.
	var statusBar = _serviceProvider?.GetService(typeof(SVsStatusbar)) as IVsStatusbar;
	var totalProjectCount = project != null ? 1 : (uint)solution.ProjectIds.Count;
	var statusBarUpdater = statusBar != null
	? new StatusBarUpdater(statusBar, _threadingContext, projectOrSolutionName, totalProjectCount)
	: null;
	// Force complete analyzer execution in background.
	var asyncToken = _listener.BeginAsyncOperation($"{nameof(VisualStudioDiagnosticAnalyzerService)}_{nameof(RunAnalyzers)}");
	Task.Run(async () =>
	{
	try
	{
	var onProjectAnalyzed = statusBarUpdater != null ? statusBarUpdater.OnProjectAnalyzed : (Action<Project>)((Project _) => { });
	await _diagnosticService.ForceAnalyzeAsync(solution, onProjectAnalyzed, project?.Id, CancellationToken.None).ConfigureAwait(false);
	// If user has disabled live analyzer execution for any project(s), i.e. set RunAnalyzersDuringLiveAnalysis = false,
	// then ForceAnalyzeAsync will not cause analyzers to execute.
	// We explicitly fetch diagnostics for such projects and report these as "Host" diagnostics.
	HandleProjectsWithDisabledAnalysis();
	}
	finally
	{
	statusBarUpdater?.Dispose();
	}
	}).CompletesAsyncOperation(asyncToken);
	return;

[mavasani]

@dibarbet @arkalyanms

[dibarbet]

Yep this definitely will need support in some way on the client side. I can think of a couple ways to tackle this, depending on if this command is more widely applicable to various languages

1. We implement this as our own custom client (similar to doc outline) and call Roslyn with a custom request for diagnostics. However we'd still need to be able to add errors to the list from that client that don't disappear, perhaps via calling the brokered service? (tbd - check with editor)
2. The command is implemented via the VS client and it passes some kind of flag in the workspace diagnostics request saying that it was invoked via that command and that we need to calculate everything

I kind of lean towards 1 if this only applicable to C#, especially since the UI behavior doesn't seem to be that complex. Open to other thoughts

[CyrusNajmabadi]

I'm amenable to us doing something specialized in Roslyn. Esp. as a stopgap to unblock pull diags. So '1' is ok with me.

However, the main way I would think about this though is: does this command make sense for other languages (like f#/c++/TS/etc. If so, Roslyn should likely not own it).

[mavasani]

However, the main way I would think about this though is: does this command make sense for other languages (like f#/c++/TS/etc. If so, Roslyn should likely not own it).

This command is used by all languages. I know for sure that C++ uses it.

[CyrusNajmabadi]

@mavasani do you think this is something you can do? We really need to get on pull diagnostics, and chasing this tail had been going on for several years now.

[mavasani]

@CyrusNajmabadi We need to first figure out how we'd like to implement this functionality and where all the pieces will live.
Run Code Analysis is basically a special case of full solution analysis, only that it an explicit one time analysis request instead of something that continuously runs in the background, and also it can be scoped to only a specific project. As this is a user triggered operation, we also need some UX for progress and completion - current non-LSP implementation reports progress in VS Status bar and finally an analysis completed message once the analysis completes. The LSP diagnostic client needs to understand that this feature means that diagnostics for closed files need to be retained in the error list, even when FSA is disabled.

I'm amenable to us doing something specialized in Roslyn. Esp. as a stopgap to unblock pull diags. So '1' is ok with me.

I feel @dibarbet is probably the right person to make this call, as I am not aware of how doc outline custom client is implemented and if it is simple enough for us to go that route. We will need to ensure that the diagnostics reported from this custom client play well with normal live and build diagnostics, with all the de-duping and priority things sorted out. I can definitely help in the implementation once we have figured out the exact way we would like to implement this.

In the longer term, I believe the following pieces are required:

• DiagnosticAnalyzerService already provides an API to force execute analyzers for a given project or solution, and provides callbacks as and when each project in the scope is analyzed. So, the core functionality is re-usable.
• Something in the Roslyn LSP layer, probably WorkspacePullDiagnosticHandler, would need to invoke this API and report diagnostics for the requested project/solution
• I believe the LSP client at the VS platform side would require to hook up a command handler for this command, identify the IVsHierarchy project or solution node on which the command was triggered, make the appropriate call into the LSP layer to trigger analysis, report analysis progress to user in the VS Status bar via some handshake with the LSP layer, and also ensure it respects closed file diagnostics being reported even in FSA disabled mode.

[CyrusNajmabadi]

@mavasani in the short term, can we still just have roslyn handle this command? As you mentioned, it can just call through the relevant apis and populate the error list itself.

That way we can ship pull diagnostics for everything else (squiggles, error list, suggestions, fading) in the short term, while not regressing this.

Then, in the medium term, we work with the platform to lsp-ify this command.

Would that be possible?

[mavasani]

We talked about this a bit more offline, and have a proposal on how to implement this command for LSP pull diagnostics mode. First, let me start with a brief overview of how this command is implemented today in non-LSP pull diagnostics world.

Documentation for Run Code Analysis: https://learn.microsoft.com/visualstudio/code-quality/how-to-run-code-analysis-manually-for-managed-code

Current Implementation

1. We hook up command handlers for Run Code Analysis commands in the Solution explorer context menus and top level Analyze menus in StanCore:
   1. Run Code Analysis on Solution: http://index/?leftProject=StanCore&leftSymbol=ibyprfayxdet
   2. Run Code Analysis on Project: http://index/?leftProject=StanCore&leftSymbol=bleicsy0qo9x
2. This command handler calls into the appropriate language specific functionality to execute analysis.
   1. For C++ projects, it invokes build command with a special MSBuild property set to trigger post build code analysis.
   2. For managed projects, it invokes RunAnalyzers API into the Roslyn language service, passing in an optional IVsHierarchy node, which is non-null when asked to run analysis on a specific project, and null otherwise: http://index/?leftProject=Microsoft.VisualStudio.LanguageServices&leftSymbol=srr0zxphoz1k&file=ExternalAccess%5CLegacyCodeAnalysis%5CApi%5CILegacyCodeAnalysisVisualStudioDiagnosticAnalyzerServiceAccessor.cs&rightSymbol=cjger1iyq24j
3. RunAnalyzers API in Roslyn VS layer calls into Roslyn's DiagnosticAnalyzerService (available at the LSP layer) to force complete analyzer execution on the requested project or solution: http://index/?leftProject=Microsoft.VisualStudio.LanguageServices&leftSymbol=b0ftpow2yxmw&file=Diagnostics%5CVisualStudioDiagnosticAnalyzerService.cs&line=304
   1. UX progress: We display code analysis progress in the status bar in terms of number of projects analyzed and finally when analysis completes
   2. Diagnostics source: We push diagnostics from this analyzer execution into Roslyn's live diagnostic error list source. Note that these diagnostics are similar to Workspace pull diagnostics, in the sense that they are computed for the entire project or solution, without taking into account whether each document is active/open/closed, etc.
   3. Lifetime: Diagnostics reported from this analysis are retained in the error list, until the user opens/activates a specific document, at which point we recompute document diagnostics for it and throw away the diagnostics reported on that document from running code analysis command
   4. Perf Note: For running code analysis on entire solution, we run analyzers on all project sequentially and not concurrently. This is because this analysis runs in the background while user can keep editing their code, and we don't want to stress the CPU for any background operations.

Proposal

1. Change StanCore functionality mentioned in 2ii. above to not invoke into Roslyn's RunAnalyzers API but instead a new API exposed by LSP client.
2. Most of the functionality from Roslyn's RunAnalyzers implementation mentioned in 3. above can be ported into the LSP client as follows:
   1. It computes the required project ID(s) for project/solution analysis command based on the given optional IVsHierarchy node.
   2. For each project in analysis scope, the client pulls onto Roslyn's Workspace pull diagnostics handler with a special flag and project ID (this is coming from run code analysis command execution and needs force analysis). Not sure if it is cleaner to just define a new kind of pull diagnostics handler or re-use the Workspace one with the project ID and this flag, either way should be fine.
   3. Update the VS Status bar to reflect project analysis progress and completion
   4. Feed the computed diagnostics into LSP client's live diagnostics source, ensuring these diagnostics are preserved for the required lifetime (i.e. document is opened/activated OR another run code analysis command is executed).
   5. I think nothing special would be needed to de-dupe these diagnostics with build diagnostics - these are just regular live diagnostics computed via different means, so the existing live-build de-duping should just work

I believe majority of the work here is porting over existing code from Roslyn into LSP client, and hooking up correct calls into Roslyn's pull handler. Roslyn side implementation should be a very trivial change once we know the diagnostics params that will be passed in to indicate the special flag and the project ID to scope analysis.