SemanticModel.GetDiagnostics(FullSpan) doesn't always produce symbol declared events for all declarations in the tree

[mavasani]

1. Create an analyzer with our template analyzer + code fix project ("Type name cannot have lowercase letters")
2. Create a C# class library with below 2 files and add this analyzer NuGet package to it.
3. Open file1 in VS, put cursor after namespace N1.
4. Hit backspace and then Ctrl + Z and do this in a loop (namespace N -> Namespace N1 -> Namespace N...)

// file 1
namespace N1
{
    partial class Class
    {
        private void F() { }
    }
} 

// file 2
namespace N1
{
    partial class Class
    {
    }
} 

Expected: There is always at least one analyzer diagnostic "Type name contains lower case letters" in error list. When you have single namespace N1 - there should be exactly one analyzer diagnostic for N1.Class. When you have namespace N and N1 - there should be 2 diagnostics: N.Class and N1.Class.

Got: For cases where you have both files with namespace N1, analyzer diagnostic is intermittent. It shows up sometimes, but for most occasions it doesn't show up, even if you wait for minutes.

Cause: New IDE analyzer driver in VS2015 Update1 relies on compilation events for driving analysis. Basic assumption that invoking SemanticModel.GetDiagnostics(fullSpan) will generate symbol declared events for all symbols declared in the file is getting violated intermittently by the compiler layer. More specifically, for the above repro, SemanticModel.GetDiagnostics(fullSpan) doesn't produce symbol declared events for either N1 or Class on some occasions when both files have N1, and we end up with no analyzer diagnostics for those cases.

[mavasani]

FYI: @JohnHamby @gafter @srivatsn

Compiler seems to produce symbol declared event only when all the containing member declarations across all its declaring syntax references have been completed. See here for example. We probably need a dedicated completion part for symbol declared events and do it irrespective of whether its members have completed or not. Invoking force complete on a symbol should always guarantee symbol declared event to be generated for it.

Note that I haven't tried the repro on VB yet, but I presume it has similar implementation and will repro there as well.

[gafter]

I think the compiler implements an invariant that we do not produce a symbol declared event until all members have had their events produced. We can change that, as I suspect nobody depends on it.

[sharwell]

Is there any workaround we can implement in the meantime, or is this something where we just have to provide release notes and wait for Update 2?

[mavasani]

@gafter Thanks - yes we need to change the compiler's invariant. I will add couple of skipped unit tests today for this scenario.

Unfortunately, any symbol/syntax node analyzer can be badly affected by this race in IDE live analysis. @sharwell I think the following workarounds might work, but you should verify if it doesn't regress typing perf:

1. Invoke Compilation.GetDeclarationDiagnostics in analyzer's Compilation start action. This would be the easiest and safest workaround if it doesn't regress perf.
2. Alternatively, add a shim layer to your analyzers that registers a semantic model action which explicitly walks the nodes in the tree to identify the nodes/symbols of interest and then creates a shim SyntaxNodeAnalysisContext or SymbolAnalysisContext and invokes the registered syntax node/symbol action by the diagnostic analyzer.

[mavasani]

@sharwell I think your other reports about missing StyleCop diagnostics SA1201, SA1204, SA1516 might also be due to this issue. Can you see if the above workaround fixes it?

[mavasani]

I just verified that workaround 1 fixes the issue for the repro analyzer as well SA1200 in StyleCopAnalyzers.sln - @sharwell you probably just need to verify perf.

[mavasani]

@gafter I am not sure why this got changed from a bug to a feature request:

1. IDE live analysis is broken due to this issue - to an end user this is just a high priority Roslyn bug.
2. The title of this issue states the primary contract on which the work for the new IDE Analyzer driver was based - the fact that compiler has a different invariant that meets the requirements of the original client (batch compilation) is irrelevant.

Regardless of whether this gets tracked as a bug or feature request, we must ensure that it gets fixed soon, and surely before 1.2.

FYI: I have sent PR #7472 with skipped unit tests for this issue.

[gafter]

@mavasani I changed it to a feature request because the original implementation correctly implements the original specification, and this is a request to change the specification. I agree that from the end user's point-of-view the result is that the system as a whole does not work as expected, and I agree that it should be changed (and I will change it) as soon as possible.

[mavasani]

Thanks Neal!

[mavasani]

Yes, Neal is correct. This fix is right and infact necessary, otherwise IDE will miss running analyzers and reporting diagnostics. I will investigate how to reduce the allocations on top of this change.

[mavasani]

Another item for @KevinH-MS to verify the perf on latest Roslyn bits - please re-assign to me if the regression still exists.

[gafter]

@KevinH-MS This issue is no longer actionable, as the fix has been checked in.

[gafter]

Oh, I see you've assigned it to yourself for perf work.

[jaredpar]

What is the status of this bug? It is labeled as fixed in December and reassigned for perf work. No updates since then.

Moving out of update 2 as this doesn't seem to be relevant here.

[mavasani]

I think @KevinH-MS already confirmed perf was fine again with a follow up change - he can probably confirm and close this out.

[jaredpar]

Moving to Area-Performance based on the conversation history here.

[KevinH-MS]

Yes, to the best of my knowledge, this is no longer an issue.