Only cache compilation if we have the same set of analyzers

[CyrusNajmabadi]

The core issue hee is that we were caching the CompilationWithAnalyzers for a compilation with all analyzers. Then, when being asked to compute again with, say, a single analyzer, we'd reuse the CompialtionWithAnalyzers.

This would then take ages to run all analyzers, even when running a fix-all for a single analyzer.

[CyrusNajmabadi]

note: i dont' like this design at all. it means on the client side we end up makign the compilatino, then create the compilationWithAnalyzersPair only to eventually call over to oop with the set of analyzers we want, where it does the same thing over ther eas well.

we should make the call to oop much higher in stack.

[CyrusNajmabadi]

NOte: i designed it this way so that we dont' toss out a very good 'full compilation with analyzers' and replace it with a 'compilation with analyzers with a single analyzer in it'.

[CyrusNajmabadi]

Note: from talking to @mavasani it sounds like he thinks the issue is that FixAll is going through these paths at all, and reusing a CompilationWithAnalyzers that was created for the full set of analyzers.

I don't disagree that we could fix things by having FixAll not go through that path. However, i find the reasoning a bit suspec here.

SPecifically, the code i'm modifying specifically and clearly was stating that if someone called in with a subset of analyzers, that the existing cached CompilationWithAnalyzers could be reused. However, what scenarios exist where that happens? Normal diagnostics will use teh same set of analyzers when calling into the compilation. So the only subset case would be something like fix-all, which ends up just using a single analyzer. Given that, it seems like the intention here strongly was to reuse the CompWithAnalyzers. But that ends up just being incredibly broken given that it runs everything.

--

To me, the fix in this pr is fairly reasonable. It simply says that the same Comp+Analyzers gives you back the same CompWithAnalyzers instance. And if you have a different set of Analyzers, you get a different instance. That seems entirely fine (esp. since the alternative proposed fix would create a new CompWithAnalyzers instance as well).

It is the case that in this PR we do cache that in the CWT. HOwever, as the CWT is keyed by ProjectState, it will drop away very shortly once the FixAll completes.

--

Note: the diag subsystem still needs many waves of cleanup. It continues to be massively complex and difficult ot reason about. APIs are sprawling and complex. And it's very unclear what any particular layer should call. It's then trivial to not call the right thing, and end up not passing along all the information that is needed to do things in the right way.

[mavasani]

Functionally, your PR seems fine to me, my only concern would be a perf regression where earlier if we computed full document diagnostics for all analyzers using a CompWithAnalyzers instance, and later a call comes in to get diagnostics for a single analyzer for that document, prior to this PR we would have reused the same CompWithAnalyzers instance and fetched cached diagnostics for that analyzer from prior computation, but now we will create a new CompWithAnalyzers and add binding + analyzer execution cost for the second request. Theoretically, we need a new CompWithAnalyzers instance only if there is at least one analyzer in the new request that is not part of the analyzers passed into the constructor of the original CompWithAnayzers instance.

IMO, FixAll was earlier using a separate code path to compute whole compilation diagnostics for a single analyzer. It is fine to use this code path now but instead of requesting all analyzer diagnostics on entire compilation for this CompilationWithAnalyzers instance, we should request diagnostics for only that analyzer for the compilation.

i.e. currently we are invoking the below API on CompilationWithAnalyzers for FixAll path:

roslyn/src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

Line 263 in 528decf

public async Task<AnalysisResult> GetAnalysisResultAsync(CancellationToken cancellationToken)

Instead we should be invoking the below API where the single analyzer corresponding to FixAll diagnostic ID is passed as the argument:

roslyn/src/Compilers/Core/Portable/DiagnosticAnalyzer/CompilationWithAnalyzers.cs

Line 273 in 528decf

public async Task<AnalysisResult> GetAnalysisResultAsync(ImmutableArray<DiagnosticAnalyzer> analyzers, CancellationToken cancellationToken)

[CyrusNajmabadi]

So I can try that. But it will def be a bunch of work to thread that information from where we are, all the way down to this call.

Let me start with this as it is still much better to just have a fresh CompilationWithAnalyzers, just with the same compilation and a different set of analyzers versus using the one with all analyzers (140+ here).

As you mentioned in our offline convo, you also thought the original design was that FixAll would have a fresh CompWithAnal anyways, so this at least aligns us with that.

I will continue trying to simplify and clean up things here.

In particular, I think the OOP boundary is too low level. We should likely push it all the way up to the top of the IDiagService, remote immediately, and then do all the work on the remote side.

Right now, we're creating lots of things (like CompWithAnal) on the host side, only to not use at all since the computation happens on oop.