Reorder checks to report issue as early as possible #79915
Conversation
| { | ||
| // it is not, let project system take care. | ||
| throw new NotImplementedException(); | ||
| return; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah yes, this bit:
That should be skipping this code at this point.
There was a problem hiding this comment.
You're right! dotnet/project-system#9734
There was a problem hiding this comment.
Do you know if the feature flag is still in VS and whether it's used anywhere or should be removed?
There was a problem hiding this comment.
@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).
There was a problem hiding this comment.
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
| int iEndLine, | ||
| int iEndColumn, | ||
| string bstrFileName) | ||
| { |
There was a problem hiding this comment.
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.
| !await DiagnosticProvider.IsSupportedDiagnosticIdAsync( | ||
| _projectId, bstrErrorId, cancellationToken).ConfigureAwait(false)) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I will do that Monday
5 participants
Possible fix for #79914
Validation: https://dev.azure.com/devdiv/DevDiv/_git/VS/pullrequest/661971