Fix state bugs in active file analysis #52807
Conversation
|
What bug does this change fix? |
|
We've had occasional reports of Current File analysis scope not behaving as expected, but without clear repro steps. While reviewing uses of this internally (mostly during work on #52789), I found cases where state was not correctly updated and caches were populated using information that needs to change when the analysis scope changes. This pull request fixes the cases I found. |
| @@ -639,15 +676,17 @@ internal async Task Document_Change(BackgroundAnalysisScope analysisScope, bool | |||
|
|
|||
| var worker = await ExecuteOperation(workspace, w => w.ChangeDocument(document.Id, SourceText.From("//"))); | |||
|
|
|||
| var expectedDocumentEvents = 1; | |||
There was a problem hiding this comment.
Why all these changes to tests? Do we now expect the same events in all the analysis modes?
There was a problem hiding this comment.
Yes. Incremental analyzers are now responsible for choosing whether or not to act (and the scope of actions) for each analysis mode. Previously this was the case for 2 of the 3 analysis scopes, but now it's a uniform concept.
| // change it to check active file (or visible files), not open files if active file tracking is enabled. | ||
| // otherwise, use open file. | ||
| return document.IsOpen() && document.SupportsDiagnostics(); | ||
| if (SolutionCrawlerOptions.GetBackgroundAnalysisScope(document.Project) == BackgroundAnalysisScope.ActiveFile) |
There was a problem hiding this comment.
So active file analysis scope is no longer a solution crawler concept for all incremental analyzers, but just something that diagnostic incremental analyzer respects?
There was a problem hiding this comment.
Yes. The same already applied to OpenFilesAndProjects and FullSolution.
| @@ -77,7 +77,7 @@ private partial class IncrementalAnalyzerProcessor | |||
| } | |||
|
|
|||
| // event and worker queues | |||
| _documentTracker = _registration.Workspace.Services.GetService<IDocumentTrackingService>(); | |||
| _documentTracker = _registration.Workspace.Services.GetRequiredService<IDocumentTrackingService>(); | |||
There was a problem hiding this comment.
I thought this was possible to be null in the OOP process.
There was a problem hiding this comment.
➡️ It is no longer possible to be null. A default implementation is provided in 126947c in which no document is ever active. Note that OOP likely needs to provide an implementation of this service which either uses a brokered service to get the current value, or throws with the intent to reveal cases where a "stateless" OOP operation is attempting to rely on state that was not captured in the request.
|
Given the race conditions in the current version of implementation, I think it would be fine to take this approach. |
ghost
added this to the 
Review commit-by-commit recommended