Roslyn: 'Convert anonymous type to tuple' not working.

[CyrusNajmabadi]

I cannot get the callback code AbstractConvertAnonymousTypeToTupleDiagnosticAnalyzer.AnalyzeSyntax to get called at all.

I've tried debugging why, but got utterly lost in the analyzer codepaths. I would often be making progress only to hit code that would bail out due to state being set elsewhere. but it wasn't clear what state was setting that. After about an hour of rinse/repeat trying to just determine why this analyzer was not being called, i gave up.

@mavasani can you take a look and see why the callback is not getting hit? The analyzer is simple. It just does 'RegisterSyntaxAction' with SyntaxKind.AnonymousObjectCreationExpression. And you shoudl be able to hit this trivially with an anonymous type as simple as new { A = 1, B = 2 }. Thanks!

[mavasani]

@CyrusNajmabadi just to make sure, did you confirm the analyzer is running in-proc? At times I have ended up attaching to devenv.exe only to find out that the analyzer executed in service hub process. I can take a look later today...

[sharwell]

This was also hit by @jinujoseph and Kendra in testing.

[mavasani]

Also looking at the analyzer implementation, it seems it is declaring itself as a syntax-only analyzer with DiagnosticAnalyzerCategory.SyntaxAnalysis, but that category is only meant for analyzers that register only actions which don't require a semantic model, currently only such action is a syntax tree action:

        /// <summary>
        /// Analyzer reports syntax diagnostics (i.e. registers a SyntaxTree action).
        /// </summary>
        SyntaxAnalysis = 0x0001,

[mavasani]

I believe either changing the analyzer to return DiagnosticAnalyzerCategory.SemanticSpanAnalysis OR changing it to register a syntax tree action that then gets all descendant nodes of the required type to report diagnostics will fix it.

I feel as a long term solution, we should get rid of DiagnosticAnalyzerCategory and the IDE diagnostic analyzer executor should look at the registered actions of an analyzer (by looking at the AnalyzerTelemetryInfo) to automatically deduce this. This way even the third party analyzer execution is on par. This will also be needed to pull out one more IBuiltInAnalyzer hack for @sharwell's work.

[CyrusNajmabadi]

but that category is only meant for analyzers that register only actions which don't require a semantic model

Oh interesting. That was never stated. The messaging around that option was that it was for anything registering syntax actions (i.e. syntax tree, or syntax nodes).

This also explains what i saw where at one point in the engine we bailed out on this analyzer as having no more 'work' to do. And it looked like it was looking up something syntactic.

Looks like we'll need an audit on this sort of thing.

--

BTW, if this is how it works, then why do things work in tests? That seems busted to me. Thanks!

[sharwell]

Looks like we'll need an audit on this sort of thing.

Easiest way is just get rid of DiagnosticAnalyzerCategory in a blanket ban. If it didn't have enough of a performance impact to include in the public analyzer API, then it doesn't have enough of a performance impact to use for the built-in analyzers.

[CyrusNajmabadi]

Easiest way is just get rid of DiagnosticAnalyzerCategory in a blanket ban.

I would be fine with that. :)

[mavasani]

BTW, if this is how it works, then why do things work in tests? That seems busted to me. Thanks!

The IDE diagnostic service has a thin shim over the core diagnostic service that filters out built in analyzers for specific syntax/semantic/compilation diagnostic phases for live analysis. This is purely a perf optimization for IBuiltInAnalyzers executed in IDE. The test layer invokes directly into the core diagnostic service.

I agree with Sam, that we should get rid of it, but we also need to make sure that it does not regress perf for built in analyzer execution. If it does, we need to implement such filtering for all analyzers based on registered action counts.

[mavasani]

I suggest we do following things as part of _this_ specific bug fix:

1. Fix the analyzer as mentioned here.
2. Rename DiagnosticAnalyzerCategory.SyntaxAnalysis to DiagnosticAnalyzerCategory.SyntaxTreeAnalysis for clarity.
3. Open a separate issue to remove DiagnosticAnalyzerCategory, with required performance validation to ensure we don't regress perf for executing built in analyzers in IDE.

[CyrusNajmabadi]

The test layer invokes directly into the core diagnostic service.

This seems unfortunate, and it's something we usually try to avoid. In general, a lot of tests invoke the highest point in ROslyn they can (or, when sensible, they even invoke from the editor layer). This way they're both testing as much of hte roslyn scenario as possible, and they're ensuring that the tests reveal things that would be hit in the real VS.

[mavasani]

Tagging @heejaechang

[CyrusNajmabadi]

I suggest we do following things as part of this specific bug fix:

That seems sensible. I would also add:

1. have IDE tests exercise a higher layer of the analysis stack as well.

[CyrusNajmabadi]

I'm happy to do '1' and '2'.