Avoid cyclic references during incremental project update #58720
Conversation
src/Workspaces/Remote/ServiceHub/Host/RemoteWorkspace.SolutionCreator.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Fix looks solid, and the tests looks good. I privately had some concern with @sharwell about whether this is producing more updates to the project dependency graph than we'd want; we have optimizations for addition/removal but the .With() that just blows away all the references won't have any of those running. He correctly observed that's more or less what we're doing today, so if there's performance wins here this may not make things any worse than they are now.
|
@sharwell @jinujoseph Given we've had a bunch of dupes of this, do we want to take this for 17.1? It seems nice and low risk. |
This change operates under the assumption that neither the starting state nor the final state include any cycles. By removing all project references from projects that change their reference sets prior to other work, we ensure extraneous references cannot lead to an invalid intermediate state. This increases the amount of work required to complete an incremental update when project references change, but does not impact the more common updates where all project references are unchanged. Fixes dotnet#52578
sharwell
force-pushed
the
avoid-cycles
branch
from
c7673d6 to
bcc2112
Compare
ghost
added this to the 
|
📝 Commit bcc2112 is based on 17.1 so we can merge it there without any rebase/porting if we need to. |
|
Which VS update will contain this fix? |
|
17.2 Preview 1 will contain this fix. @vdevappa Since you're asking, I'm assuming you've hit this issue. If this is the case, I'm curious how you are hitting it. It seems extremely difficult to hit this accidentally. |
|
I have a large .net solution that was migrated over time and now contains .net 4.8 and .net core projects. I hit this when I change between x64 and AnyCPU. I have to reboot VS every single time after this. Also, the .NET Core migration tool also throws this error and dies. Will this fix the .net core migration tool as well? I did that right now and see this error in VS yellow bar on top: StreamJsonRpc.RemoteInvocationException: Adding project reference from '(ProjectId, #511c784e-0a6f-4eaa-aa92-a07a958c2a43 - Locus.MongoDB.Core)' to '(ProjectId, #ed174d50-8aad-498e-8c9a-702f4070aec7 - Locus.Models.Public)' will cause a circular reference. |
|
@vdevappa have you submitted a feedback ticket to developer community on this in the past? Can you either create a new one or update the existing one (and link me to it) to contain the following:
Somewhere in one (or both) of these is a conditional |
|
"Since the past six months, Visual Studio has been showing errors at the top which really disrupt my DEV experience." |
|
@vdevappa that issue is too old and all the attached diagnostics were cleared out. I think we'll need a new one filed instead. |
This change operates under the assumption that neither the starting state nor the final state include any cycles. By removing all project references from projects that change their reference sets prior to other work, we ensure extraneous references cannot lead to an invalid intermediate state. In terms of graph theory, this change relies on the fact that any subset of edges from an acyclic directed graph will also be an acyclic directed graph.
This increases the amount of work required to complete an incremental update when project references change, but does not impact the more common updates where all project references are unchanged.
Fixes #52578