dotnet / roslyn

The Roslyn .NET compiler provides C# and Visual Basic languages with rich code analysis APIs.
https://docs.microsoft.com/dotnet/csharp/roslyn-sdk/
MIT License
18.95k stars 4.02k forks source link

Error squiggles do not appear for Compilation End analyzers in Full Solution Analysis mode #67276

Open sharwell opened 1 year ago

sharwell commented 1 year ago

Version Used: Version 17.6.0 Preview 3.0 [33505.110.main]

Steps to Reproduce:

  1. Enable full solution analysis
  2. Open a file with a diagnostic reported by a Compilation End analyzer (e.g. RS1010)

Diagnostic Id:

N/A

Expected Behavior:

Squiggles appear in the editor.

Actual Behavior:

Squiggles do not appear in the editor.

mavasani commented 1 year ago

Tagging @CyrusNajmabadi.

I verified that this issue repros not just for compilation end diagnostics, but also for build-only compiler diagnostics, such as unused field warning (CS0169) and also for other non-local analyzer diagnostics.

I believe this regressed with removal of push diagnostic tagging #67505. Push diagnostic tagger listened to diagnostics changed events from all the diagnostic sources - intellisense diagnostics for open documents, build-only diagnostics from build-only source, non-local intellisense diagnostics (which includes compilation end diagnostics), and any other diagnostic sources registered with the IDiagnosticService in the IDE. Pull diagnostic tagger seems to only handle the first bucket here, i.e. local intellisense diagnostics for open documents: https://github.com/dotnet/roslyn/blob/1cabbfeac0719ba84689348cc8db2a7a167ae14b/src/EditorFeatures/Core/Diagnostics/AbstractDiagnosticsTaggerProvider.SingleDiagnosticKindPullTaggerProvider.cs#L110-L115

mavasani commented 1 year ago

Also tagging @dibarbet

I think we need to add tagger logic to allow additional pull diagnostic tagging requests for other kind of diagnostics. Basically, something similar to the WorkspacePulllDiagnosticsHandler.

CyrusNajmabadi commented 1 year ago

So this is me reiterating a recent request that we redo our apis for diagnostics. We get this wrong because the current crop of apis are too complicated and too unclear on how you are expected to use them to get the entire set of diagnostics for all scenarios.

Because of this, we always get things wrong because the feature code looks good and sensible, but it's actually incomplete for some subset of use cases.

We need our abstraction that we expose to be the simple one, where all the complexity of these use cases is hidden behind it. Then, both pull tagging, or lsp pull diags would not need to do anything but just call into that API, producing the same complete experience. Things like "full solution" options, or aggregating/filtering build diagnostics, would all be something that would not have to be aware of.

mavasani commented 1 year ago

Reported at https://developercommunity.visualstudio.com/t/Errors-not-showing-in-source-code/10458218 (AB#1881111)

nwalker-et commented 5 months ago

This issue has become somewhat of a blocker for a project my company is currently working on. We need to be able to emit diagnostics at specific locations based on aggregated data from analysis of the entire compilation. Based on the recommended approach for this kind of stateful analyzer, we created one action that collects the relevant information during analysis of symbols/syntax/operations, then we created a CompilationEnd action that emits diagnostics at the necessary locations based on that aggregated information. We have also written code fix providers to go with those diagnostics. However, because CompilationEnd diagnostics are not given error squiggles in the code, there does not seem to be any way for developers to apply the provided code fix.

We have found some hacky workarounds, but we're concerned that they might not scale well for larger solutions. Are there any suggested workarounds for this issue? Is it possible that this issue could be given greater priority by the Roslyn team?

sharwell commented 5 months ago

I am also able to reproduce similar issues currently. Diagnostics never appear, even when Full Solution Analysis is enabled and/or Run Code Analysis on Project/Solution is executed manually.

CyrusNajmabadi commented 5 months ago

I need Manish to weigh in on the history here. I thought this was an intentional decision, but I don't know all the particulars.

Specifically, under what conditions do we actually run compilation end analyzers.

I'm fine with never running them in the ide and simply stating that it's too complex a feature to support along with code fixes.

nwalker-et commented 5 months ago

To clarify, the diagnostics that I've emitted from Compilation End actions do appear in Visual Studio's "Error List" window. They do not, however, cause any of the code in the editor to be underlined with error or warning squiggles, despite the fact that you can double-click on the error in the list and be taken to the corresponding location in the code. Additionally, it seems like code fixes can only be applied when the code is underlined with squiggles for the corresponding diagnostic.

While explicitly documenting that this use case is not supported would be better than the current situation, I would also much rather this be properly supported. As the example I previously cited shows, there are situations where you simply do not have the information necessary when analyzing individual symbols, syntax nodes, etc., to know whether you need to emit important diagnostics. And in our particular use case, providing a code fix is a rather significant part of our analyzer's usability.

At the very least, I would like some ability to apply a code fix from the error list, even in the absence of error or warning squiggles. I don't have enough knowledge of your code base, though, to know whether that is any easier to accomplish.

sharwell commented 5 months ago

@CyrusNajmabadi It has always been possible to enable Full Solution as the analysis scope to run compilation end diagnostics inside the IDE. These didn't always tie in to code fixes, but they certainly worked for squiggles and error list.

The performance for these analyzers varies by solution. In many solutions, the overhead is completely acceptable. This is one of the primary reasons we added #52789.

CyrusNajmabadi commented 5 months ago

@sharwell We still support FSA. Workspace-pull-diags respects those flags and should be analyzing everything based on that. These reported diagnostics then go to the client, which owns displaying them in squiggles and error list.