I have a feeling that this change in general (prior to this specific PR) is going to break error reporting in certain scenarios (though I'm not 100% sure which ones). Callers seem to be relying on us to throw a not impl exception from ReportError/ReportError2 to know if we can handle the error or not (there's more on the c++ side as well, but one example is https://devdiv.visualstudio.com/DevDiv/_git/dotnet-project-system-Trusted?path=/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Build/LanguageServiceErrorListProvider.cs&version=GBmain&_a=contents&line=89&lineStyle=plain&lineEnd=90&lineStartColumn=1&lineEndColumn=1)

Since this is now happening in the queue, I don't think the callers will observe it any more.

i agree. i think we'll need to rethink this whole area for the long term. Specifically, even having this concept happen synchronously through a VS service doesn't seem like a good idea at all.

Ideally, if we do want this idea of attaching external errors to an LS's own diagnostics, then we would do so through an lsp entrypoint that can actually be fully async and do this properly.

@drewnoakes This is an unfortunate scenario where on the VS side, things are async. And on the Roslyn side, things are async. But we're funneled through a sync api that screws things over. How difficult would it be to whip up some sort of IAsyncVsLanguageServiceBuildErrorReporter that would allow us to be async end to end. We could then maintain the contract of this existing api without jumping through major hoops.

I didn't think CPS was using this anymore, or at least that code is no longer live since there was a rollout of the new diagnostics system?

This is also absolutely used by the legacy project systems, which would be challenging to make async.

Ah yes, this bit:

https://github.com/dotnet/project-system/blob/8ba13c7fa917b86462e0ff32a07ef3e15eb8dfa6/src/Microsoft.VisualStudio.ProjectSystem.Managed.VS/ProjectSystem/VS/Build/LanguageServiceErrorListProvider.cs#L62-L67

That should be skipping this code at this point.

You're right! dotnet/project-system#9734

Do you know if the feature flag is still in VS and whether it's used anywhere or should be removed?

@olegtk @dpugh @davidpugh do you guys still have any feature flags around LSP Pull Diags? At this point, i think they can all be removed (Roslyn doesn't support any other form at this point).

Took a look. The only other instance I found was in CPS, and I filed a PR to remove that here: https://dev.azure.com/devdiv/DevDiv/_git/CPS/pullrequest/662888

Just to call it out, if the concern was also the exceptions being thrown, there's a comment on this method about switching it to be PreserveSig. Not sure that changes any of the deeper discussons here. That might involve changes to our COM types though that might break the CPS code if that feature flag wasn't enabled.

What would it look like to have some code that updates the list of IDs as an in-proc cache which we can still check synchronously?

I will do that Monday