Skip to content

Reorder checks to report issue as early as possible #79915

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 15, 2025
Merged

Reorder checks to report issue as early as possible #79915

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 15 additions & 25 deletions src/VisualStudio/Core/Def/TaskList/ProjectExternalErrorReporter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,27 +82,6 @@ await func(cancellationToken)
}
}

private async Task<bool> CanHandleAsync(string errorId, CancellationToken cancellationToken)
{
// make sure we have error id, otherwise, we simple don't support
// this error
if (errorId == null)
{
// record NFW to see who violates contract.
FatalError.ReportAndCatch(new Exception("errorId is null"));
return false;
}

// we accept all compiler diagnostics
if (errorId.StartsWith(_errorCodePrefix))
{
return true;
}

return await DiagnosticProvider.IsSupportedDiagnosticIdAsync(
_projectId, errorId, cancellationToken).ConfigureAwait(false);
}

private static ImmutableArray<ExternalError> GetExternalErrors(IVsEnumExternalErrors pErrors)
{
using var _ = ArrayBuilder<ExternalError>.GetInstance(out var allErrors);
Expand Down Expand Up @@ -282,15 +261,26 @@ public void ReportError2(
int iEndColumn,
string bstrFileName)
{
Copy link
Member

@jasonmalinowski jasonmalinowski Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// make sure we have error id, otherwise, we simple don't support
// this error
if (bstrErrorId == null)
{
// record NFW to see who violates contract.
FatalError.ReportAndCatch(new Exception("errorId is null"));
return;
}

_taskQueue.AddWork(ReportErrorAsync);

async Task ReportErrorAsync(CancellationToken cancellationToken)
{
// first we check whether given error is something we can take care.
if (!await CanHandleAsync(bstrErrorId, cancellationToken).ConfigureAwait(false))
// we accept all compiler diagnostics
if (!bstrErrorId.StartsWith(_errorCodePrefix) &&
!await DiagnosticProvider.IsSupportedDiagnosticIdAsync(
_projectId, bstrErrorId, cancellationToken).ConfigureAwait(false))
Comment on lines +280 to +281
Copy link
Member

@jasonmalinowski jasonmalinowski Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will do that Monday

{
// it is not, let project system take care.
throw new NotImplementedException();
return;
Copy link
Member

@dibarbet dibarbet Aug 15, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

@jasonmalinowski jasonmalinowski Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@jasonmalinowski jasonmalinowski Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@drewnoakes drewnoakes Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@drewnoakes drewnoakes Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@CyrusNajmabadi CyrusNajmabadi Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

Copy link
Member

@drewnoakes drewnoakes Aug 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

if ((iEndLine >= 0 && iEndColumn >= 0) &&
Expand Down
Loading