Enable LSP pull diagnostics from Run Code Analysis command #70186

mavasani · 2023-09-29T08:21:20Z

Prior to this change, Run Code Analysis command force executed all the analyzers on the selected project/solution, but the reported analyzer diagnostics were not getting reported from LSP pull handlers. This PR makes the following changes to ensure these diagnostics get reported as workspace pull diagnostics:

First commit: 5e21c04
1. Modify WorkspacePullDiagnosticHandler so that it reports diagnostics when either of the following conditions are met:
  1. Full solution analysis is enabled (it was already doing so prior to this PR)
  2. Project was already force analyzed and has cached diagnostics (it could be via Run Code Analysis command or any other client in future).
2. Update the Workspace and Project diagnostic sources added by the Workspace pull handler to do the following:
  1. If full solution analysis is enabled, compute and report the closed file diagnostics for the latest solution snapshot
  2. Otherwise, return the already cached closed file diagnostics, which may be from a prior snapshot (this matches the non-LSP behavior when Run Code Analysis command is executed)
3. Update the DiagnosticAnalyzerService.ForceAnalyzeAsync command that is invoked during Run Code Analysis to keep track of projects that were force analyzed. Additionally, also send a workspace diagnostic refresh request to the LSP client after each project has finished analysis.
With the above changes, Run Code Analysis command executes and reports diagnostics as expected in LSP pull diagnostics mode. There is still a considerable delay (10 sec) in error list getting populated with diagnostics from this command as the workspace diagnostic refresh is triggered at a delay. We will be addressing this in a separate follow-up change, where in we will trigger a new kind of diagnostic refresh request specifically catered to code analysis execution, such that the client does the workspace pull without any delay. We are still unsure if we need to pass in the project context around in this refresh request OR let it be a full refresh, given the server is only going to attempt to return workspace diagnostics from the cache, so a full refresh request may not incur any significant cost.
Second commit c892a83: Fix Run Code Analysis when background analysis has been disabled via project options or global options

TBD: Add unit tests for this scenario. I am going to do this in a follow-up PR so that #70040 does not stay blocked on the product changes here.

Non-LSP pull diags experience

LSP pull diags experience (with this change)

Fixes dotnet#65967 Prior to this change, Run Code Analysis command force executed all the analyzers on the selected project/solution, but the reported analyzer diagnostics were not getting reported from LSP pull handlers. This PR makes the following changes to ensure these diagnostics get reported as workspace pull diagnostics: 1. Modify WorkspacePullDiagnosticHandler so that it reports diagnostics when either of the following conditions are met: 1. Full solution analysis is enabled (it was already doing so prior to this PR) 2. Project was already force analyzed and has cached diagnostics (it could be via Run Code Analysis command or any other client in future). 2. Update the Workspace and Project diagnostic sources added by the Workspace pull handler to do the following: 1. If full solution analysis is enabled, compute and report the closed file diagnostics for the latest solution snapshot 2. Otherwise, return the already cached closed file diagnostics, which may be from a prior snapshot (this matches the non-LSP behavior when Run Code Analysis command is executed) 3. Update the `DiagnosticAnalyzerService.ForceAnalyzeAsync` command that is invoked during Run Code Analysis to keep track of projects that were force analyzed. Additionally, also send a workspace diagnostic refresh request to the LSP client after each project has finished analysis. With the above changes, Run Code Analysis command executes and reports diagnostics as expected in LSP pull diagnostics mode. There is still a considerable delay (10 sec) in error list getting populated with diagnostics from this command as the workspace diagnostic refresh is triggered at a delay. We will be addressing this in a separate follow-up change, where in we will trigger a new kind of diagnostic refresh request specifically catered to code analysis execution, such that the client does the workspace pull without any delay. We are still unsure if we need to pass in the project context around in this refresh request OR let it be a full refresh, given the server is only going to attempt to return workspace diagnostics from the cache, so a full refresh request may not incur any significant cost. TBD: Add unit tests for this scenario. I am going to do this in a follow-up PR so that dotnet#70040 does not stay blocked on the product changes here.

src/Features/LanguageServer/Protocol/Handler/Diagnostics/WorkspacePullDiagnosticHandler.cs

…project options or global options

src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs

src/Features/LanguageServer/Protocol/Features/Diagnostics/DiagnosticAnalyzerService.cs

...res/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticSources/ProjectDiagnosticSource.cs

...geServer/Protocol/Handler/Diagnostics/DiagnosticSources/WorkspaceDocumentDiagnosticSource.cs

CyrusNajmabadi · 2023-09-29T14:25:23Z

I'll be honest, i was wary of this PR from the description, but i wanted to see if the code change at least made a lot of sense and was very clear in how it worked.

Right now it's not that way for me. First, it couples two separate features together "RCA" and "Workspace Pull". In abstract, i don't mind that. However, it's done in a way that is very unclear (to me) as to what the semantics are. Effectively, there appears to be a cache that says "at some point, an RCA was done for this project" at which point, the entire behavior for that project changes (for workspace daignsotics) from that point on to the future.

--

I thought from our last teams meeting that we were going with something much simpler. Effectively, a users was only in one of the following states:

FSA is on
FSA is off

Then, we would have two diagnostic entrypoints:

workspace/diagnostics
workspace/codeanalysis

Then we had the following 4 cases:

FSA-on and workspace/diagnostics. In this case, workspace/diagnostics returns all the diagnostics for the entire workspace
FSA-off and workspace/diagnostics. In this case, workspace/diagnostics returns nothing.
FSA-on and workspaces/codeanalysis. In this case, workspace/codeanalysis returns nothing (since workspace/diagnostic) handles this.
FSA-off nd workspaces/codeanalysis. In this case, workspace/codeanalysis returns all the diagnostics for the solution (or requested project).

The benefit of this is that we don't have the two systems fighting each other. For example, we don't get the results from workspace/codeanalysis, only to have it wiped away when then next workspace/diagnostics goes through.

And we still get the benefit that when you open a document, it supercedes the diagnostics produced by either workspace/diagnostics or workspace/codeananalysis.

workspace/codeanalysis It's also very simple to implement. On the server side, it's effectively the same as workspace/diagnostics except that:

it reverses the processing of hte FSA flag.
it has some way to pass in a project guid to scope the work.

From a code perspective, we'd likely share a common base class for this, and just tweak those pieces.

From teh user perspective, things would work well.

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs

...res/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticSources/ProjectDiagnosticSource.cs

src/EditorFeatures/Test/CodeFixes/CodeFixServiceTests.cs

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs

src/Features/Core/Portable/Diagnostics/ICodeAnalysisDiagnosticAnalyzerService.cs

...res/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticSources/ProjectDiagnosticSource.cs

...geServer/Protocol/Handler/Diagnostics/DiagnosticSources/WorkspaceDocumentDiagnosticSource.cs

src/Features/LanguageServer/Protocol/Handler/Diagnostics/WorkspacePullDiagnosticHandler.cs

mavasani · 2023-10-05T11:39:02Z

@CyrusNajmabadi - Addressed all your feedback and also added unit tests. Please take a look, thanks!

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs

CyrusNajmabadi · 2023-10-05T22:52:38Z

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs

+        /// Running code analysis on the project force computes and caches the diagnostics on the DiagnosticAnalyzerService.
+        /// We return these cached document diagnostics here, including both local and non-local document diagnostics.
+        /// </summary>
+        public Task<ImmutableArray<DiagnosticData>> GetLastComputedDocumentDiagnosticsAsync(DocumentId documentId, CancellationToken cancellationToken)


note: if these are docs on hte interface, you don't need anything here.

CyrusNajmabadi · 2023-10-05T22:53:32Z

src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs

+        /// Indicates if non-local document diagnostics must be returned.
+        /// Non-local diagnostics are the ones reported by analyzers either at compilation end callback OR
+        /// in a different file from which the callback was made. Entire project must be analyzed to get the
+        /// complete set of non-local document diagnostics.


suggestion for future: make an options object for all of this.

CyrusNajmabadi · 2023-10-05T22:54:05Z

src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs

+        /// in a different file from which the callback was made. Entire project must be analyzed to get the
+        /// complete set of non-local document diagnostics.
+        /// </param>
+        /// <param name="cancellationToken">Cancellation token.</param>


note/nit: you can leave docs off. i don't include basic stuff like this.

CyrusNajmabadi · 2023-10-05T22:54:29Z

src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs

+        /// Indicates if non-local document diagnostics must be returned.
+        /// Non-local diagnostics are the ones reported by analyzers either at compilation end callback OR
+        /// in a different file from which the callback was made. Entire project must be analyzed to get the
+        /// complete set of non-local document diagnostics.


a good reason for this to be an options object. would just be one set of docs :)

can be done in the future.

CyrusNajmabadi · 2023-10-05T22:55:34Z

...tures/LanguageServer/Protocol/Features/Diagnostics/DiagnosticAnalyzerService_UpdateSource.cs

@@ -89,7 +89,7 @@ ValueTask<ImmutableArray<DiagnosticData>> IDiagnosticUpdateSource.GetDiagnostics
                return new ValueTask<ImmutableArray<DiagnosticData>>(GetSpecificCachedDiagnosticsAsync(workspace, id, includeSuppressedDiagnostics, includeNonLocalDocumentDiagnostics: true, cancellationToken));
            }

-            return new ValueTask<ImmutableArray<DiagnosticData>>(GetCachedDiagnosticsAsync(workspace, projectId, documentId, includeSuppressedDiagnostics, includeNonLocalDocumentDiagnostics: true, cancellationToken));
+            return new ValueTask<ImmutableArray<DiagnosticData>>(GetCachedDiagnosticsAsync(workspace, projectId, documentId, includeSuppressedDiagnostics, includeLocalDocumentDiagnostics: true, includeNonLocalDocumentDiagnostics: true, cancellationToken));


generally speaking, doc'ing why these are the right values would be helpful. but doesn't have to happen now.

CyrusNajmabadi · 2023-10-05T22:57:59Z

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

-                {
-                    isAnalysisDisabled = isAnalysisDisabled && _globalOptions.IsAnalysisDisabled(language);
-                }
-            }


what happened here?

This was the change done for fixing the issue that Sam pointed out when background code analysis is completely disabled and Run code analysis does nothing - both in LSP and non-LSP mode. c892a83 is the core fix, which mostly deletes a bunch of code and makes a small change in ForceAnalyzeAsync to handle this correctly.

Perfect. Thanks!

CyrusNajmabadi · 2023-10-05T22:58:13Z

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

-                        _hostDiagnosticUpdateSource.UpdateDiagnosticsForProject(project.Id, key: this, diagnostics);
-                    }
-                }
-            }


what happened here?

CyrusNajmabadi

LGTM, had some questions, but i trust you. :)

sharwell · 2023-10-09T13:41:55Z

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs

+
+namespace Microsoft.CodeAnalysis.Diagnostics;
+
+[ExportWorkspaceServiceFactory(typeof(ICodeAnalysisDiagnosticAnalyzerService), ServiceLayer.Host), Shared]


📝 This should be ServiceLayer.Default (it's defined in the same assembly as the interface it provides an implementation of)

mavasani added Area-IDE Area-Analyzers labels Sep 29, 2023

mavasani requested review from kayle, CyrusNajmabadi and dibarbet September 29, 2023 08:21

mavasani requested a review from a team as a code owner September 29, 2023 08:21

dotnet-issue-labeler bot added the untriaged Issues and PRs which have not yet been triaged by a lead label Sep 29, 2023

mavasani commented Sep 29, 2023

View reviewed changes

src/Features/LanguageServer/Protocol/Handler/Diagnostics/WorkspacePullDiagnosticHandler.cs Outdated Show resolved Hide resolved

Fix Run Code Analysis when background analysis has been disabled via …

c892a83

…project options or global options

CyrusNajmabadi reviewed Sep 29, 2023

View reviewed changes

src/Features/Core/Portable/Diagnostics/IDiagnosticAnalyzerService.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Sep 29, 2023

View reviewed changes

src/Features/LanguageServer/Protocol/Features/Diagnostics/DiagnosticAnalyzerService.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Sep 29, 2023

View reviewed changes

...res/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticSources/ProjectDiagnosticSource.cs Outdated Show resolved Hide resolved

CyrusNajmabadi reviewed Sep 29, 2023

View reviewed changes

...geServer/Protocol/Handler/Diagnostics/DiagnosticSources/WorkspaceDocumentDiagnosticSource.cs Outdated Show resolved Hide resolved

mavasani added 2 commits October 4, 2023 17:42

Address feedback

a2d6777

Remove unnecessary field

fd94130

mavasani commented Oct 4, 2023

View reviewed changes

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs Outdated Show resolved Hide resolved

mavasani commented Oct 4, 2023

View reviewed changes

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs Outdated Show resolved Hide resolved

mavasani commented Oct 4, 2023

View reviewed changes

src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs Outdated Show resolved Hide resolved

mavasani commented Oct 4, 2023

View reviewed changes

...res/LanguageServer/Protocol/Handler/Diagnostics/DiagnosticSources/ProjectDiagnosticSource.cs Outdated Show resolved Hide resolved

mavasani commented Oct 4, 2023

View reviewed changes

src/EditorFeatures/Test/CodeFixes/CodeFixServiceTests.cs Show resolved Hide resolved