Decouple FixAll from the workspace #1962
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Merged
Today, the RunFixAllInCodeActionService is tightly coupled to the current workspace, applying its changes to the workspace as it processes the request and counting on the updates from doing so to invalidate diagnostic caches. While this does work, it's has 2 major downsides: 1. It's a race condition waiting to happen, where the propagation of the changed documents invalidating the diagnostic provider's caches races with the next fix all operation attempting to get the diagnostics it needs to fix. 2. It has bad interations with the buffer updater from clients. The service returns the needed changes to clients, who then apply the changes. The client will then see that changes have been applied, and send them back to the server. The server's workspace has already been updated with these changes, so it ends up double applying the changes, leaving the server's workspace in a bad state with garbage in many files. This commit breaks the dependency of the FixAll service on the current workspace. We now have the fixes occur on top of the previous fix's changed solution. At the end, if the client requested the changes be applied on the workspace side, then we apply the changes to the workspace at that point. In order for this work, I had to modify the diagnostic workers a bit to add an option to force a document to be analyzed, skipping the cache entirely. This is necessary because the cache is only invalidated on document add/remove/change in the workspace itself. The documents we work on linearly in the fix all provider do not get added/changed/removed in the workspace, so we need to analyze them again, right then and there. To support this, the AnalyzerWorkQueue is now based on documents, not document ids, though the caches in the workers are still based on ids.
333fred
force-pushed
the
decouple-fixall
branch
from
d88cbb9 to
8aa8252
Compare
333fred
added a commit
to 333fred/vscode-csharp
that referenced
this pull request
Sep 28, 2020
|
@filipw I'm not sure if any of these test failures could be related to this change. Can you advise? |
|
no, all three separate failures are "known" to be flaky already... let's see the retry |
JoeRobich
approved these changes
Sep 30, 2020
src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs
Outdated
Show resolved
Hide resolved
src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs
Outdated
Show resolved
Hide resolved
src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs
Outdated
Show resolved
Hide resolved
david-driscoll
requested changes
Oct 1, 2020
src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs
Show resolved
Hide resolved
src/OmniSharp.Roslyn.CSharp/Services/Refactoring/V2/BaseCodeActionService.cs
Outdated
Show resolved
Hide resolved
src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs
Outdated
Show resolved
Hide resolved
src/OmniSharp.Roslyn.CSharp/Workers/Diagnostics/CSharpDiagnosticWorkerWithAnalyzers.cs
Outdated
Show resolved
Hide resolved
…n DocumentId->Diagnostics. This is done via a ConditionalWeakTable to ensure we don't root Documents and prevent garbage collection.
|
@david-driscoll I believe I've addressed your feedback. |
filipw
reviewed
Oct 3, 2020
src/OmniSharp.Roslyn.CSharp/Services/Refactoring/RunFixAllCodeActionService.cs
Show resolved
Hide resolved
|
@david-driscoll Would you like to give this PR your blessing? =) |
JoeRobich
pushed a commit
to dotnet/vscode-csharp
that referenced
this pull request
Oct 15, 2020
* Readd incremental changes. * React to OmniSharp/omnisharp-roslyn#1962
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Today, the RunFixAllInCodeActionService is tightly coupled to the current workspace, applying its changes to the workspace as it processes the request and counting on the updates from doing so to invalidate diagnostic caches. While this does work, it's has 2 major downsides:
This commit breaks the dependency of the FixAll service on the current workspace. We now have the fixes occur on top of the previous fix's changed solution. At the end, if the client requested the changes be applied on the workspace side, then we apply the changes to the workspace at that point. In order for this work, I had to modify the diagnostic workers a bit to add an option to force a document to be analyzed, skipping the cache entirely. This is necessary because the cache is only invalidated on document add/remove/change in the workspace itself. The documents we work on linearly in the fix all provider do not get added/changed/removed in the workspace, so we need to analyze them again, right then and there. To support this, the AnalyzerWorkQueue is now based on documents, not document ids, though the caches in the workers are still based on ids.
In the process of doing this PR, I also fixed #1960.