Update "Current Document" background analysis semantics and fix Full Solution analysis with project config changes #69750
Conversation
Currently, setting the background analysis scope for analyzer diagnostics to `Current Document` only refreshes the diagnostics for the current document on all operations such as document edit, option changes, project configuration changes etc. This causes issues where any of these changes render these diagnostics as stale. We already account for this behavior for compiler diagnostic background analysis scope by processing all visible documents AND open documents which had at least one non-hidden diagnostic reported. We now do the same for analyzer background analysis scope, and hence avoid analyzer stale diagnostics in non-active open documents with `Current Document` scope.
ghost
added
the
untriaged
| // Analyzers are enabled for active document. | ||
| BackgroundAnalysisScope.ActiveFile => isActiveDocument, | ||
| // Analyzers are enabled for visible documents and open documents which had errors/warnings in prior snapshot. | ||
| BackgroundAnalysisScope.VisibleFilesAndFilesWithPreviouslyReportedDiagnostics => IsVisibleDocumentOrOpenDocumentWithPriorReportedVisibleDiagnostics(isVisibleDocument, isOpenDocument, previousData), |
There was a problem hiding this comment.
This is the core fix - we now have consistent semantics between compiler and analyzer diagnostics refresh for Current Document scope.
Rest of the changes in this PR are just cascading rename changes as the enum field name has also been renamed to match the field name within CompilerDiagnosticScope enum
|
I feel like this is what the Open Files setting is for. Or put another way, Current Document was added to provide more strict control over the performance characteristics of the analyzer engine even knowing that it would be less accurate than the other available modes. |
|
What's the general end user experience goal here? It's not to produce new diagnostics in the non-current file, right? If it just to remove old stale diagnostics that may have been fixed? |
Yes, that is correct. However, we cannot keep stale diagnostics around in error list for common user scenarios. The change here is to ensure that we keep re-running only those analyzers that reported one or more non-hidden diagnostics in any of the open documents, so stale error list entries can be avoided. I don't believe this will add any noticeable performance hit. We can even go more stringent here and do so only for analyzers that reported warnings or errors, I think stale messages for non-active documents are not that big a deal. I can make that change if you'd prefer and that should further reduce the number of analyzers we run for non-active documents.
Exactly - we have prior diagnostics reported in the error list for documents that are not part of the current background analysis scope, and we want to ensure that we remove the stale ones that might have been fixed or suppressed. |
|
this seems reasonable to me. eventually old diagnostics/analyzers will stop running for non-current documents as the diagnostics disappear. And new ones will only appear for the current doc. overall i think this experience will be more sensible for users (and in line with compiler-diags) while not costing much more for CPU/memory, and being 'close enough' to the intent/verbiage of the options. |
|
@sharwell any concerns here? Let me know if doing the below would make this PR feel more palatable:
|
|
I believe that the current behavior already reflects the user's intent. The diagnostics will be refreshed the next time the user changes to the document containing the diagnostics. |
That is a reasonable stance. However, note that "Current Document" is the default value for the analyzer background diagnostics scope. The common scenario here is user is not even aware such an option exists. They open a bunch of source files one by one, error list populates with analyzer diagnostics for each one as they open them, edit their editorconfig, say suppresses some diagnostic IDs, but all the diagnostics of these IDs stay around in the error list. User needs to click error list entries and open each file one by one to remove the stale diagnostics. With this change, there will be a very small perf hit, likely not even noticeable, and we avoid stale entries in the error list. |
| using var _ = PooledHashSet<DocumentId>.GetInstance(out var documentIdsToProcess); | ||
| documentIdsToProcess.AddRange(_lastResult.DocumentIdsOrEmpty); | ||
| documentIdsToProcess.AddRange(result.DocumentIdsOrEmpty); |
mavasani
changed the title
|
@CyrusNajmabadi @sharwell Can I get reviews on this? It addresses customer reported stale diagnostic issues. |
|
On vacation till Thursday! |
...guageServer/Protocol/Features/Diagnostics/EngineV2/DiagnosticIncrementalAnalyzer.Executor.cs
Outdated
Show resolved
Hide resolved
CyrusNajmabadi
left a comment
CyrusNajmabadi
left a comment
There was a problem hiding this comment.
this looks fine to me. would like my feedback nits addressed.
4 participants
Current Document scope change
Currently, setting the background analysis scope for analyzer diagnostics to
Current Documentonly refreshes the diagnostics for the current document on all operations such as document edit, option changes, project configuration changes etc. This causes issues where any of these changes render these diagnostics as stale.We already account for this behavior for compiler diagnostic background analysis scope by processing all visible documents AND open documents which had at least one non-hidden diagnostic reported in the prior document snapshot (i.e. has an entry in error list).
We now do the same for analyzer background analysis scope, and hence avoid analyzer stale diagnostics in non-active open documents with
Current Documentscope.FSA fix
Fixes #69763 with 23517e1