Fix removal and addition of analyzer in same batch#58024
Conversation
| else | ||
| { | ||
| _workspace.ApplyChangeToWorkspace(w => w.OnAnalyzerReferenceRemoved(Id, visualStudioAnalyzer.GetReference())); | ||
| visualStudioAnalyzer.Dispose(); |
There was a problem hiding this comment.
Yeah, I should get some tests for disposal too.
There was a problem hiding this comment.
Now with DivideByZeroException more tests!
| Id, | ||
| newSolution: solutionChanges.Solution.RemoveAnalyzerReference(Id, analyzerReference.GetReference())); | ||
|
|
||
| analyzerReference.Dispose(); |
There was a problem hiding this comment.
should this either be a using or try/finally block to make sure it's always disposed?
There was a problem hiding this comment.
So the object came from a previous addition which was "a long time ago" so using won't work. My first thought was your suggestion did make sense, but if we were to throw somewhere earlier it means the analyzer reference is still within _analyzersRemovedInBatch, so if a later call to close the batch works then maybe we'd dispose it then?
More generally we really don't expect applying a batch to ever really throw, since we're in a pretty funky state -- we don't have a good mechanism to roll back to the point "before the failure" since everything is very stateful here.
| { | ||
| _analyzersAddedInBatch.Add(visualStudioAnalyzer); | ||
| _analyzersRemovedInBatch.Remove(analyzerPendingRemoval); | ||
| _analyzerPathsToAnalyzers.Add(fullPath, analyzerPendingRemoval); |
There was a problem hiding this comment.
Do we remove the path before the batch is complete? That seems odd... but maybe correct.
There was a problem hiding this comment.
Yeah, it's a bit strange. The _analyzerPathsToAnalyzers is added/removed to represent the "current" state of the world at all points; this means that any further add/remove can first check it for it being a valid action, i.e. the throw you see happening back a few lines before all of this if you're adding a duplicate.
In the synchronous dispose path, we were asserting that the task completed; if it completed due to a fault, we were silently catching that exception. This ensures we propagate the exception. Under no "normal" circumstance we do we expect an exception to throw when a batch is applied, so this is only needed for diagnosing faults.
If we had an analyzer reference being removed and re-added in the same batch, we would throw exceptions. Fixes https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1264575
The VisualStudioAnalyzer also pushes load diagnostics to the workspace so failing to do this would mean they stay around.
23a1983 to
359f24a
Compare
If we had an analyzer reference being removed and re-added in the same batch, we would throw exceptions.
Fixes https://dev.azure.com/devdiv/DevDiv/_workitems/edit/1264575
While doing this, discovered a few other issues so commit-at-a-time recommended.