Remaining work to get pull diagnostics + LSP fully enabled, and all push-diagnostics + error list code removed.

[CyrusNajmabadi]

1. Need to figure out how someone can configure suppressions from the error list if we don't own the error list.
2. Need to remove IDiagnosticService.DiagnosticsUpdated. Currently this is used by tagger (but is not needed if tagger is just pulling), and by typescript. David believes TS just uses this for LSP-push-diags in liveshare and can likely be removed since we have worked with them to support pull diagnostics.
3. Move IDiagnosticService.GetDiagnosticsAsync to be snapshot based. Currently it takes in Workspace+ProjId/DocId. This is non-sensical as that means you may get random results depending on the state of Workspace.CurrentSnapshot. This needs to move to take Project/Document instead.
4. Need LSP pull diagnostics to be category-based, so we can report syntax before semantics.

See also: https://github.com/dotnet/roslyn/pull/62741, https://github.com/dotnet/roslyn/pull/62735 and https://github.com/dotnet/roslyn/pull/62771

[CyrusNajmabadi]

@tmat @mavasani For visibility. We're very close on this :)

[tmat]

Cool. My experience with pull diagnostics in Roslyn.sln is not great though. They seem to be lagging quite a bit. That needs to be addressed imo, before we switch.

[CyrusNajmabadi]

Cool. My experience with pull diagnostics in Roslyn.sln is not great though. They seem to be lagging quite a bit. That needs to be addressed imo, before we switch.

So far i've found them pretty good. We identified a definite problem in that syntactic diagnostics are gated on semantics diagnostics being computed. This can def make things feel slower as clearly broken code still takes a while to appear if semantics are taking a bit of time. This is being tracked with: https://github.com/dotnet/roslyn/issues/62560

One thing i'm not clear on is if the diagnostics subsystem properly does minimal pull-diagnostics computation when a method body edit happens. @mavasani to confirm if we have that optimization, or if we need to file a tracking issue to make that that works as well for semantic diagnostics.

[dibarbet]

@tmat are you still hitting the issue where workspace diagnostics are being requested even with FSA off? I sent a message on it - but was curious if it was possible to debug through it on your machine (I'm unable to get the same behavior on my machine).

[tmat]

I haven't had chance to do much work in Roslyn.sln since.

[mavasani]

One thing i'm not clear on is if the diagnostics subsystem properly does minimal pull-diagnostics computation when a method body edit happens. @mavasani to confirm if we have that optimization, or if we need to file a tracking issue to make that that works as well for semantic diagnostics.

This optimization is part of the solution crawler, so I wouldn't assume it to be automatically enabled for pull diagnostics unless similar logic is added to LSP handler. Currently, the LSP handler requests diagnostics for the entire document:

https://github.com/dotnet/roslyn/blob/5030cd43fa32bd67f4da3dededd205110ad800b4/src/Features/LanguageServer/Protocol/Handler/Diagnostics/DocumentPullDiagnosticHandler.cs#L112-L118

https://github.com/dotnet/roslyn/blob/5030cd43fa32bd67f4da3dededd205110ad800b4/src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.IncrementalAnalyzerProcessor.cs#L188-L197

https://github.com/dotnet/roslyn/blob/5030cd43fa32bd67f4da3dededd205110ad800b4/src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.IncrementalAnalyzerProcessor.cs#L267-L302

[mavasani]

@CyrusNajmabadi @dibarbet The core logic to compute whether only method body edits took place from the last snapshot is in the solution crawler over here:

https://github.com/dotnet/roslyn/blob/5030cd43fa32bd67f4da3dededd205110ad800b4/src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.cs#L538-L555

https://github.com/dotnet/roslyn/blob/5030cd43fa32bd67f4da3dededd205110ad800b4/src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.cs#L402-L425

https://github.com/dotnet/roslyn/blob/5030cd43fa32bd67f4da3dededd205110ad800b4/src/Features/Core/Portable/SolutionCrawler/WorkCoordinator.cs#L430-L440

[dibarbet]

thanks @mavasani - I think that is an optimization I'll need to port over.

[dibarbet]

@mavasani hmm - in the incremental analyzer implementation, it looks like it ignores the method node we find during the differencing - https://sourceroslyn.io/#Microsoft.CodeAnalysis.LanguageServer.Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs,28

[mavasani]

@mavasani hmm - in the incremental analyzer implementation, it looks like it ignores the method node we find during the differencing - https://sourceroslyn.io/#Microsoft.CodeAnalysis.LanguageServer.Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs,28

Wow, that seems like a big regression. I need to look at the history to figure out when/why this was removed.

[mavasani]

in the incremental analyzer implementation, it looks like it ignores the method node we find during the differencing - https://sourceroslyn.io/#Microsoft.CodeAnalysis.LanguageServer.Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs,28

Tagging @heejaechang for historical context.

I investigated this a bit more and it seems the IDE DiagnosticIncrementalAnalyzer's v1 implementation performed method body based analysis, but this was never implemented for the v2 engine. Below are the implementations from the same repo snapshot:

https://github.com/dotnet/roslyn/blob/82c9de92110a90088e3e03d5c86b072a795ff4e8/src/Features/Core/Portable/Diagnostics/EngineV1/DiagnosticIncrementalAnalyzer.cs#L278-L286

https://github.com/dotnet/roslyn/blob/82c9de92110a90088e3e03d5c86b072a795ff4e8/src/Features/Core/Portable/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs#L16-L28

v2 engine was written and replaced the v1 engine about 6 years ago, so this is not a recent behavior change. @heejaechang do you recall if this method body edit optimization was removed due to bugs/flakiness or was it just pending work that never got done? If it is the latter, then it might make sense to only port this optimization to the LSP handler. The good part here is:

1. This would open up scope for quite a decent performance gain when moving to LSP pull diagnostics for method body editing scenarios.
2. This optimization would be an enhancement, rather than a parity requirement for enabling pull diagnostics by default.

I do notice that for the light-bulb path we do use IBuiltInAnalyzer.GetAnalyzerCategory() to only execute span based analyzers for the line span, which is good:

https://github.com/dotnet/roslyn/blob/3fe68247686c879325eabdb3eddc4f11acc8ecb7/src/Features/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_GetDiagnosticsForSpan.cs#L173-L192

[CyrusNajmabadi]

@mavasani in a pull-diags world, if there is any sort of caching, i would assume that adding this optimization back in would be fairly trivial. we should know the Doc/Tree the last pulled diags were against. the next pull can attempt to diff the current tree against the previous one. If teh diff is entirely within a method, we can keep all the diags outside of that, and only recompute what is inside right?

Basically, my expectation is that pull ideally makes this easier to add as an invisible optimization (outside of perf speedup).

[heejaechang]

https://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/SolutionCrawler/WorkCoordinator.IncrementalAnalyzerProcessor.cs,263

this should call the body only analysis for open file?

[sharwell]

If we did end up shipping for 6 years without the method body edit detection enabled, I would encourage we just make that state permanent, since it allows us to finally remove IBuiltInAnalyzer.

[CyrusNajmabadi]

note that in that time we have had an enormous amount of complaints about performance :)

[mavasani]

https://sourceroslyn.io/#Microsoft.CodeAnalysis.Features/SolutionCrawler/WorkCoordinator.IncrementalAnalyzerProcessor.cs,263 this should call the body only analysis for open file?

@heejaechang It does, and solution crawler is fine here. The issue is the DiagnosticIncrementalAnalyzer ignores the body node: https://github.com/dotnet/roslyn/blob/843f8db03084db9c6e8c1abea5681caf1d535d23/src/Features/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_IncrementalAnalyzer.cs#L28-L29

If we did end up shipping for 6 years without the method body edit detection enabled, I would encourage we just make that state permanent, since it allows us to finally remove IBuiltInAnalyzer.

@sharwell Note that for light-bulb path we do use IBuiltInAnalyzer.GetAnalyzerCategory() to only execute span based analyzers for the line span:

https://github.com/dotnet/roslyn/blob/3fe68247686c879325eabdb3eddc4f11acc8ecb7/src/Features/LanguageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer_GetDiagnosticsForSpan.cs#L173-L192

If we remove that logic, we will start executing SimplifyTypeName analyzer on the the entire file every time Ctrl + Dot is invoked. We certainly don't want that.

[CyrusNajmabadi]

I also wouldn't say that we haven't noticed. Perf has been a perpetual problem. It's just never been clear what is at fault. I know that i had to add back smarts to scope thigns back to method bodies to help speed up perf in completion lists for example :) i think we're just not in a good place perf-wise, with lots of components being problematic, so it's easy for regressions to happen that just get added to the pile of problems :)

[heejaechang]

interesting... what I remember is in proc diagnostic analyzer uses method body (since in proc handles open file case) and out of proc doesn't care about method body (since it only handles closed file and compilation diagnostics). and I believe out of proc doesn't even have concept of open or visible files ..

but certainly, a bug could have introduced..