Fix for issues around analyzer exception diagnostics getting suppressed or not reported #673

mavasani · 2015-02-19T10:34:44Z

This change addresses #259: below issues related to diagnostics generated for analyzer exceptions from third party analyzers.

Suppression of duplicate exception diagnostics: Current mechanism did the suppression in SuppressMessageState based on unique reported messages. This is obviously incorrect as an exception diagnostic will be reported non-suppressed and suppressed on subsequent queries to SuppressMessageState.IsDiagnosticSuppressed.
The IDE diagnostic service has multiple layers where document/project diagnostics are filtered and these analyzer exception diagnostics were getting dropped at various places.

So this change moves the exception diagnostics generation + reporting out of the regular analyzer diagnostic pipeline and in line with analyzer load failure diagnostics reporting in VS:

Add an event handler to AnalyzerDriverHelper to report analyzer exception diagnostics to interested clients.
Listen to these diagnostic events in IDE diagnostic service and wrap them with relevant workspace/project argument and generate updated events.
Add an AbstractHostDiagnosticUpdateSource in Features layer to listen and report analyzer exception diagnostic events from diagnostic service. Additionally, removal of an analyzer reference in workspace will clean up the diagnostics for the analyzers belonging to that analyzer reference.
Listen to exception diagnostic events in command line compiler and report as regular diagnostics.

Added typw AbstractHostDiagnosticUpdateSource can be extended in future to report other kind of host diagnostics which are not related to a project/document/analyzer.

@heejaechang @JohnHamby @srivatsn can you please review?

…iagnostics

mavasani · 2015-02-19T10:37:47Z

src/Features/Core/Diagnostics/EngineV1/DiagnosticAnalyzerDriver.cs

-                        var exceptionDiagnostics = AnalyzerExceptionToDiagnostics(analyzer, e, _cancellationToken);
-                        return model == null ? exceptionDiagnostics : GetFilteredDocumentDiagnostics(exceptionDiagnostics, model.Compilation);
+                        var exceptionDiagnostics = AnalyzerExceptionToDiagnostic(analyzer, e, _cancellationToken);
+                        ReportAnalyzerExceptionDiagnostic(analyzer, exceptionDiagnostics, compilation);


Needs a null check for value returned from AnalyzerExceptionToDiagnostic. I'll also fix the redundant spaces added in various files in this PR.

…iagnostics

…ensure Diagnostic equality works as expected for Descriptor based diagnostics. 2) Additionally, also refactor code a bit more to move register/unregister code for analyzer exception diagnostics down to the core AnalyzerDriverHelper. 3) Fix test failures.

…iagnostics

mavasani · 2015-02-19T23:22:26Z

src/Features/Core/Diagnostics/AbstractHostDiagnosticUpdateSource.cs

+namespace Microsoft.CodeAnalysis.Diagnostics
+{
+    internal abstract class AbstractHostDiagnosticUpdateSource : IDiagnosticUpdateSource
+    {


This type probably needs some comments, I'll add something here.

msJohnHamby · 2015-02-20T00:10:59Z

I sign off on these changes.

heejaechang · 2015-02-20T00:26:32Z

👍

but I am guessing this will be cleaned up once we remove driver and helper?

heejaechang · 2015-02-20T00:29:11Z

src/Features/Core/Diagnostics/WorkspaceAnalyzerManager_HostDiagnostics.cs

+
+namespace Microsoft.CodeAnalysis.Diagnostics
+{
+    internal sealed partial class WorkspaceAnalyzerManager


I really don't like adding all these static methods and event. this was never meant to used as helper. these should be an instance that passed around.

all these static looks like a hack.

if it is just static stuff, why is it even at WorkspaceAnalyzerManager? just create AnalyzerExceptionStaticEvent type.

I am going to just move these methods directly to AbstractHostDiagnosticUpdateSource. I just parked it onto WorkspaceAnalyzerManager thinking it could be central place for analyzer related things to look for, but you are right it probably doesn't lie here.

mavasani · 2015-02-20T00:49:41Z

Well lot of this static stuff is to avoid threading it down to DiagnosticAnalyzerDriver. If you see change in our V2 engine, it is very straightforward and can easily be switched to instance event once we delete V1 engine and IDE DiagnosticAnalyzerDriver.

heejaechang · 2015-02-20T00:57:09Z

Yep, that is why I did +1 ☺

```
     Heejae
```

From: Manish Vasani [mailto:notifications@github.com]
Sent: Thursday, February 19, 2015 4:50 PM
To: dotnet/roslyn
Cc: HeeJae Chang
Subject: Re: [roslyn] Fix for issues around analyzer exception diagnostics getting suppressed or not reported (#673)

Well lot of this static stuff is to avoid threading it down to DiagnosticAnalyzerDriver. If you see change in our V2 engine, it is very straightforward and can easily be switched to instance event once we delete V1 engine and IDE DiagnosticAnalyzerDriver.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/673#issuecomment-75170490.

This change addresses #259: below issues related to diagnostics generated for analyzer exceptions from third party analyzers. 1.Suppression of duplicate exception diagnostics: Current mechanism did the suppression in SuppressMessageState based on unique reported messages. This is obviously incorrect as an exception diagnostic will be reported non-suppressed and suppressed on subsequent queries to SuppressMessageState.IsDiagnosticSuppressed. 2.The IDE diagnostic service has multiple layers where document/project diagnostics are filtered and these analyzer exception diagnostics were getting dropped at various places. So this change moves the exception diagnostics generation + reporting out of the regular analyzer diagnostic pipeline and in line with analyzer load failure diagnostics reporting in VS: 1.Add an event handler to AnalyzerDriverHelper to report analyzer exception diagnostics to interested clients. 2.Listen to these diagnostic events in IDE diagnostic service and wrap them with relevant workspace/project argument and generate updated events. 3.Add an AbstractHostDiagnosticUpdateSource in Features layer to listen and report analyzer exception diagnostic events from diagnostic service. Additionally, removal of an analyzer reference in workspace will clean up the diagnostics for the analyzers belonging to that analyzer reference. 4.Listen to exception diagnostic events in command line compiler and report as regular diagnostics. Added typw AbstractHostDiagnosticUpdateSource can be extended in future to report other kind of host diagnostics which are not related to a project/document/analyzer.

My prior change #673 made to address issues with analyzer exception diagnostics introduced a few leaks in product and test code. The primary reason was that I attempted to use static events to track state as the existing AnalyzerDriverHelper type which did the core analyzer execution was a static type with all static methods. Additionally, I added a static event to the new host diagnostic update source added for reporting analyzer specific diagnostics. These were holding onto projects/workspaces in test runs causing leaks. I have reverted the approach of static events and instead refactored the code in AnalyzerDriver project to simplify the whole design: 1. Renamed AnalyzerDriverHelper to AnalyzerExecutor and made it a non-static type, which has instance fields for all the configuration parameters for analyzer callbacks. 2. Move all the core analyzer callbacks (actions/Initialize method/supported diagnostics) into AnalyzerExecutor. Command line compiler just creates a single instance of the executor, while IDE driver creates instances per analyzer. 3. Both these changes simplified the API a lot, and I just had to add an additional field "Action addExceptionDiagnostic" to AnalyzerExecutor to configure how to handle exception diagnostics. 4. MEF import AbstractHostDiagnosticUpdateSource in DiagnosticAnalyzerService and thread it down to IDE analyzer driver. 5. Changes 3 and 4 above meant that in the IDE driver, delegate "addExceptionDiagnostic" just asks the HostDiagnosticUpdateSource to report the exception diagnostic produced by the AnalyzerExecutor. This change also re-enables the tests skipped by #761 and fixes #759.

mavasani added 5 commits February 17, 2015 22:33

Initial Checkin for AnalyzerException diagnostics

6776636

Initial Checkin for AnalyzerException diagnostics

9c1cb0b

Merge remote-tracking branch 'upstream/master' into AnalyzerSpecificD…

817a7ae

…iagnostics

AnalyzerDiagnosticUpdateSource - Working implementation

8921fa9

Merge latest and additional code refactoring.

493a8f8

dnfclas added the cla-not-required label Feb 19, 2015

mavasani reviewed Feb 19, 2015
View reviewed changes

mavasani added 7 commits February 19, 2015 10:16

Some code refactoring.

58e9516

Merge remote-tracking branch 'upstream/master' into AnalyzerSpecificD…

3da136e

…iagnostics

Merge remote-tracking branch 'upstream/master' into AnalyzerSpecificD…

611ca93

…iagnostics

Minor fix

58263e6

Merge remote-tracking branch 'upstream/master' into AnalyzerSpecificD…

94e3abf

…iagnostics

Fe more minor fixes.

824fae9

mavasani reviewed Feb 19, 2015
View reviewed changes

heejaechang reviewed Feb 20, 2015
View reviewed changes

mavasani added 2 commits February 20, 2015 01:07

Address CR feedback.

fee3a48

Merge to latest.

0faaab4

mavasani merged commit 19f62c4 into dotnet:master Feb 20, 2015

mavasani deleted the AnalyzerSpecificDiagnostics branch February 20, 2015 09:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for issues around analyzer exception diagnostics getting suppressed or not reported #673

Fix for issues around analyzer exception diagnostics getting suppressed or not reported #673

mavasani commented Feb 19, 2015

mavasani Feb 19, 2015

mavasani Feb 19, 2015

heejaechang Feb 19, 2015

msJohnHamby commented Feb 20, 2015

heejaechang commented Feb 20, 2015

heejaechang Feb 20, 2015

heejaechang Feb 20, 2015

mavasani Feb 20, 2015

mavasani commented Feb 20, 2015

heejaechang commented Feb 20, 2015

Fix for issues around analyzer exception diagnostics getting suppressed or not reported #673

Fix for issues around analyzer exception diagnostics getting suppressed or not reported #673

Conversation

mavasani commented Feb 19, 2015

mavasani Feb 19, 2015

Choose a reason for hiding this comment

mavasani Feb 19, 2015

Choose a reason for hiding this comment

heejaechang Feb 19, 2015

Choose a reason for hiding this comment

msJohnHamby commented Feb 20, 2015

heejaechang commented Feb 20, 2015

heejaechang Feb 20, 2015

Choose a reason for hiding this comment

heejaechang Feb 20, 2015

Choose a reason for hiding this comment

mavasani Feb 20, 2015

Choose a reason for hiding this comment

mavasani commented Feb 20, 2015

heejaechang commented Feb 20, 2015