Show generator produced diagnostics by jasonmalinowski · Pull Request #60474 · dotnet/roslyn

jasonmalinowski · 2022-03-30T01:15:47Z

This adds support for showing generator-produced diagnostics in the error list. The implementation generally follows how we handle diagnostics for files that we were unable to read in the IDE -- there is a placeholder analyzer that represents the diagnostics in our various state sets.

The one interesting thing this changes a bit is our handling of the full-solution analysis flag. While testing this (and seeing how the file load diagnostics worked), I noticed that we had an early bail in "analyze project" that meant two things:

Unless you had FSA on, there was no way to actually view file load diagnostics; this seemed odd since you then might have diagnostics in your error list about missing types, but you wouldn't see the root problem.
If you turned FSA back off, nothing would ever run to make the FSA-produced diagnostics go away, short of you manually opening files or something.

Thus I am changing the FSA handling a bit: if FSA is off, rather than skip everything we are only skipping the analysis now; this ensures that we are clearing out old/stale diagnostics, and we have an opportunity to still show the generator and file load diagnostics that we may have on hand. We only still produce those for other files (or things in the dependency graph) for files that are open though -- the key here is unlike regular diagnostics, these are diagnostics we know that came from the very production of the compilation itself. If the file is open, we're already going to have compilations being created for it (indeed, at least for open file diagnostics!), so we're not creating any extra work than we already had.

Fixes #49664
Fixes #58435
Fixes #54098

Nothing exciting here: when we are asked for diagnostics, we grab it off of the GeneratorDriver after running generators.

This is potentially going to make things slightly less efficient in that the loop across all projets may be calling GetProjectsThatTransitivelyDependOnThisProject multiple times; the first time will do the actual work and cache it, but we're spending the costs of extra lookups again. I don't think we had specific evidence for that optimization being critical, so I'm aiming for simpler code here.

jasonmalinowski · 2022-04-14T00:49:57Z

.../Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs


        public async Task AnalyzeProjectAsync(Project project, bool semanticsChanged, InvocationReasons reasons, CancellationToken cancellationToken)
        {
-            // Perf optimization. check whether we want to analyze this project or not.


This early-out seems incorrect to do here. If nothing else, if you turned full-solution-analysis on, and then got diagnostics in the error list, and turned FSA back off again, we'd immediately hit the fast path here so nothing would ever cause the FSA diagnostics to disappear until you opened each file. This also meant that our analyzer that reports file load issues wouldn't report visible diagnostics unless you had FSA on.

There are still checks for FSA being on in GetProjectAnalysisDataAsync; we still rely on that being there and not actually doing full analysis unless needed.

(see the PR text for more details)

This sounds reasonable, but reasonable things can still be scary! I presume project.GetSourceGeneratorDiagnosticsAsync will still be expensive in terms of forcing the compilation, symbols and running all generators?

@mavasani: it's expensive in that the first person to do it isn't cheap. But anybody asking for a compilation is paying that same cost; my goal here is we're only going to be asking for compilations that are already open documents or in their dependency chain. In essence, the analysis of open documents will have already ran all of that code -- and once it's asked for once, it's fast for everybody else.

jasonmalinowski · 2022-04-14T22:49:57Z

I still want to ideally get more tests on this, but still unsure where to add them.

mavasani · 2022-04-15T03:39:30Z

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

+                // We will count generator diagnostics as semantic diagnostics; some filtering to either syntax/semantic is necessary or else we'll report diagnostics twice.
+                if (kind == AnalysisKind.Semantic)
+                {
+                    var generatorDiagnostics = await textDocument.Project.GetSourceGeneratorDiagnosticsAsync(cancellationToken).ConfigureAwait(false);


So this method executes in-proc, do we want to run GetSourceGeneratorDiagnosticsAsync also in-proc?

We're practically running generators in-proc too (since we need compilations there), but happy to move the work OOP for consistency here. I guess I must have missed then for the document analysis case where we were jumping OOP then? Where is that?

for the document analysis case where we were jumping OOP then

We run the internal DocumentDiagnosticAnalyzers and ProjectDiagnosticAnalyzers in-proc, as this would need the partner teams that have authored these analyzers to also support running out of proc. We do run all the first party and third party analyzers out-of-proc:

roslyn/src/Features/LanguageServer/Protocol/Features/Diagnostics/DocumentAnalysisExecutor.cs

Line 29 in acb47af

private readonly InProcOrRemoteHostAnalyzerRunner _diagnosticAnalyzerRunner;

roslyn/src/Features/LanguageServer/Protocol/Features/Diagnostics/DocumentAnalysisExecutor.cs

Lines 151 to 166 in acb47af

private async Task<ImmutableDictionary<DiagnosticAnalyzer, DiagnosticAnalysisResult>> GetAnalysisResultAsync(DocumentAnalysisScope analysisScope, CancellationToken cancellationToken)

{

RoslynDebug.Assert(_compilationWithAnalyzers != null);

try

{

var resultAndTelemetry = await _diagnosticAnalyzerRunner.AnalyzeDocumentAsync(analysisScope, _compilationWithAnalyzers,

_logPerformanceInfo, getTelemetryInfo: false, cancellationToken).ConfigureAwait(false);

return resultAndTelemetry.AnalysisResult;

}

catch

{

_onAnalysisException?.Invoke();

throw;

}

}

Chatting with @mavasani we realized cleaning this up properly is going to require a bit more work here: when we're doing FSA analysis OOP, we are creating in-proc Compilations even if we don't end up using them. Before the assumption was that relatively cheap (since creating a Compilation didn't really do anything if you didn't ask it for anything), but now with source generators producing a Compilation actually is going to do work. Since we don't have many users using FSA, and that problem isn't new with this PR, we're going to count fixing that as out of scope here, but will follow up next week.

mavasani · 2022-04-15T04:05:09Z

...guageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.Executor.cs

+                        // If we are producing document diagnostics for some other document in this project, we still want to show
+                        // certain project-level diagnostics that would cause file-level diagnostics to be broken. We will only do this though if
+                        // some file that's open is depending on this project though -- that way we're going to only be analyzing projects
+                        // that have already had compilations produced for.


Tagging @CyrusNajmabadi. Cyrus and me have been discussing about how to reduce the number of compilations getting produced via background diagnostic computation by avoiding analyzing all open files for compiler diagnostics. We are hoping to soon introduce a new background analysis mode that would only refresh compiler diagnostics for visible documents (we will dogfood + design this further to make sure it doesn't break user experience).

However, your change below would mean that we will always produces compilations for projects which have at least one open document or are being referenced by a project which has one open document, which will nullify any of the above mentioned perf optimizations as per the current implementation below. GetProjectAnalysisDataAsync does get invoked by the solution crawler at a longer delay though, so we may be fine in terms of responsiveness hit by doing this additional compute, but something that we certainly need to keep an eye on in terms of performance.

@mavasani, @CyrusNajmabadi: I'd have no problem with this code here then also changing to "visible" documents as well as a part of that change if you wanted to go ahead with that. Honestly if there's a function I should be calling that is "FSA is off, what documents will be analyzed?" that I should be calling here now, happy to switch it.

Also this made me realize that here:

roslyn/src/Features/Core/Portable/Workspace/BackgroundCompiler.cs

Line 101 in 4814028

var allOpenProjects = _workspace.GetOpenDocumentIds().Select(d => d.ProjectId).ToSet();

We are asking for compilations for all open documents's projects there, and that'll be more aggressive than here anyways. Going to look at making an experiment to either change that to visible files, or just get rid of the background compiler entirely.

Sounds fine, we can take this up as a follow-up work item.

I very frequently open a group of related files, make a change, and watch to see the error list update for all the open documents. Changing the compiler diagnostics to only run on visible documents would be severely disruptive to what seems like a common workflow.

Changing the compiler diagnostics to only run on visible documents would be severely disruptive to what seems like a common workflow

@sharwell Our current plan is to have the default still be "Open Files" for compiler diagnostics. We will add the new option for "Visible Files", have folks on the IDE team dogfood that mode and determine if there is value in keeping in and/or making it the default mode. I was planning to bring this up for discussion sometime this week or early next within our team.

mavasani

LGTM.

We can add more tests as a follow-up commit or PR. Thank you for filing #60843 for the cleanup work!

This adds support for showing generator-produced diagnostics in the error list. The implementation generally follows how we handle diagnostics for files that we were unable to read in the IDE -- there is a placeholder analyzer that represents the diagnostics in our various state sets. The one interesting thing this changes a bit is our handling of the full-solution analysis flag. While testing this (and seeing how the file load diagnostics worked), I noticed that we had an early bail in "analyze project" that meant two things: Unless you had FSA on, there was no way to actually view file load diagnostics; this seemed odd since you then might have diagnostics in your error list about missing types, but you wouldn't see the root problem. If you turned FSA back off, nothing would ever run to make the FSA-produced diagnostics go away, short of you manually opening files or something. Thus I am changing the FSA handling a bit: if FSA is off, rather than skip everything we are only skipping the analysis now; this ensures that we are clearing out old/stale diagnostics, and we have an opportunity to still show the generator and file load diagnostics that we may have on hand. We only still produce those for other files (or things in the dependency graph) for files that are open though -- the key here is unlike regular diagnostics, these are diagnostics we know that came from the very production of the compilation itself. If the file is open, we're already going to have compilations being created for it (indeed, at least for open file diagnostics!), so we're not creating any extra work than we already had. Fixes dotnet#49664 Fixes dotnet#58435 Fixes dotnet#54098

ghost added the Area-IDE label Mar 30, 2022

jasonmalinowski self-assigned this Mar 31, 2022

jasonmalinowski force-pushed the show-generator-produced-diagnostics branch from 5d8cf75 to bad27ae Compare April 12, 2022 00:44

jasonmalinowski added 4 commits April 13, 2022 15:54

Delete unused code

88f730d

Add an internal API for getting the generator diagnostics

186787c

Nothing exciting here: when we are asked for diagnostics, we grab it off of the GeneratorDriver after running generators.

Convert documentation for members to use XML doc comments

922be4f

jasonmalinowski force-pushed the show-generator-produced-diagnostics branch from bad27ae to e3ca355 Compare April 14, 2022 00:32

jasonmalinowski commented Apr 14, 2022

View reviewed changes

jasonmalinowski force-pushed the show-generator-produced-diagnostics branch from e3ca355 to 1a13ae5 Compare April 14, 2022 22:14

jasonmalinowski marked this pull request as ready for review April 14, 2022 22:20

jasonmalinowski requested review from a team as code owners April 14, 2022 22:20

jasonmalinowski requested a review from mavasani April 14, 2022 22:30

jasonmalinowski mentioned this pull request Apr 14, 2022

Report exceptions thrown by generators in Error List as warnings #54098

Closed

mavasani reviewed Apr 15, 2022

View reviewed changes

mavasani approved these changes Apr 20, 2022

View reviewed changes

jasonmalinowski force-pushed the show-generator-produced-diagnostics branch from 1a13ae5 to 9c9b52e Compare April 20, 2022 19:00

jasonmalinowski merged commit 2700e31 into dotnet:main Apr 20, 2022

jasonmalinowski deleted the show-generator-produced-diagnostics branch April 20, 2022 21:52

ghost added this to the Next milestone Apr 20, 2022

dibarbet modified the milestones: Next, 17.3.P1 Apr 25, 2022

sharwell mentioned this pull request Apr 29, 2022

Source Generators Not Generating Sources #49249

Open

	private async Task<ImmutableDictionary<DiagnosticAnalyzer, DiagnosticAnalysisResult>> GetAnalysisResultAsync(DocumentAnalysisScope analysisScope, CancellationToken cancellationToken)
	{
	RoslynDebug.Assert(_compilationWithAnalyzers != null);

	try
	{
	var resultAndTelemetry = await _diagnosticAnalyzerRunner.AnalyzeDocumentAsync(analysisScope, _compilationWithAnalyzers,
	_logPerformanceInfo, getTelemetryInfo: false, cancellationToken).ConfigureAwait(false);
	return resultAndTelemetry.AnalysisResult;
	}
	catch
	{
	_onAnalysisException?.Invoke();
	throw;
	}
	}

Conversation

jasonmalinowski commented Mar 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasonmalinowski Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski commented Apr 14, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani Apr 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonmalinowski Apr 15, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mavasani left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jasonmalinowski commented Mar 30, 2022 •

edited

Loading

jasonmalinowski Apr 14, 2022 •

edited

Loading

mavasani Apr 16, 2022 •

edited

Loading

jasonmalinowski Apr 15, 2022 •

edited

Loading