Remove flag controlling if non-local diagnostics should be returned when getting diagnostics for a document. #80044
Conversation
|
|
||
| var document = GetDocumentAndSelectSpan(workspace, out var span); | ||
| var allDiagnostics = await DiagnosticProviderTestUtilities.GetAllDiagnosticsAsync(workspace, document, span, includeNonLocalDocumentDiagnostics: parameters.includeNonLocalDocumentDiagnostics); | ||
| var allDiagnostics = await DiagnosticProviderTestUtilities.GetAllDiagnosticsAsync(workspace, document, span); |
There was a problem hiding this comment.
tests were all over the map about what value they passed along. Even though the value was often false, changing this to be true didn't break a single thing. so tests were basically just passing along a value to satisfy the signature, with no one really knowing what value was appropriate.
| var service = project.Solution.Services.GetRequiredService<IDiagnosticAnalyzerService>(); | ||
| var diagnostics = Filter(await service.GetDiagnosticsForIdsAsync( | ||
| project, documentId: null, _diagnosticIds, shouldIncludeAnalyzer: null, includeLocalDocumentDiagnostics: true, includeNonLocalDocumentDiagnostics: false, cancellationToken).ConfigureAwait(false)); | ||
| project, documentId: null, _diagnosticIds, shouldIncludeAnalyzer: null, includeLocalDocumentDiagnostics: true, cancellationToken).ConfigureAwait(false)); |
There was a problem hiding this comment.
these changed. but not in a way that should matter.
- we're doing fix-all. So we are running the analyzer fully to compute results.
- this will now catch the case that the analyzer reports diagnostics for different files, having the fixer attempt to fix those as well.
| var allDiagnostics = await service.GetDiagnosticsForIdsAsync( | ||
| Document.Project, documentId: null, diagnosticIds: null, shouldIncludeAnalyzer, | ||
| includeLocalDocumentDiagnostics: true, includeNonLocalDocumentDiagnostics: true, cancellationToken).ConfigureAwait(false); | ||
| includeLocalDocumentDiagnostics: true, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
no change in behavior here. pull-diags was always asking for the diags that should show up in a document, even if it came from running analyzers on other docs.
| var diagnostics = await service.GetDiagnosticsForIdsAsync( | ||
| Document.Project, Document.Id, diagnosticIds: null, _shouldIncludeAnalyzer, | ||
| includeLocalDocumentDiagnostics: false, includeNonLocalDocumentDiagnostics: true, cancellationToken).ConfigureAwait(false); | ||
| includeLocalDocumentDiagnostics: false, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
If non local diagnostics are always being returned, it seems like we need to delete this source? This was a separate source because the normal diagnostic sources called GetDiagnosticsForSpan which did not report these. But if it is now reporting them, we'll be reporting them both from the other document sources and this one?
| var diagnosticService = _workspace.Services.GetRequiredService<IDiagnosticAnalyzerService>(); | ||
| var latestProjectDiagnostics = (await diagnosticService.GetDiagnosticsForIdsAsync(project, documentId: null, | ||
| diagnosticIds: uniqueDiagnosticIds, shouldIncludeAnalyzer: null, includeLocalDocumentDiagnostics: true, includeNonLocalDocumentDiagnostics: true, cancellationToken) | ||
| diagnosticIds: uniqueDiagnosticIds, shouldIncludeAnalyzer: null, includeLocalDocumentDiagnostics: true, cancellationToken) |
There was a problem hiding this comment.
no change here or below.
CyrusNajmabadi
changed the title
dibarbet
left a comment
dibarbet
left a comment
There was a problem hiding this comment.
The pull diagnostics non local source doesn't seem quite right
| var diagnostics = await service.GetDiagnosticsForIdsAsync( | ||
| Document.Project, Document.Id, diagnosticIds: null, _shouldIncludeAnalyzer, | ||
| includeLocalDocumentDiagnostics: false, includeNonLocalDocumentDiagnostics: true, cancellationToken).ConfigureAwait(false); | ||
| includeLocalDocumentDiagnostics: false, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
If non local diagnostics are always being returned, it seems like we need to delete this source? This was a separate source because the normal diagnostic sources called GetDiagnosticsForSpan which did not report these. But if it is now reporting them, we'll be reporting them both from the other document sources and this one?
|
|
@dibarbet Actually no, and this is super subtle. For an open file: DocDiagnostics does indeed only get the documents for anlyzers running on the file. That hasn't changed. GetDiagnosticsForSpan still doesn't return non-local diags. WorkspaceDiags returns nothing (since this is an open file). the NonLocalDiagSource calls thruogh GetDiagnosticsForIdsAsync which now unilaterally gets non-local diags. WHich is the same behavior as before. -- So these scenarios continue to work and your tests still pass. What's happening is that we are ending up effectively with two entrypoints on IDiagAnalService: There is Then there is It is the latter method which i'm touching. You used to be able to say "i do/don't want non-local diags for the doc being requested". Now you always get the non-local diags. I will update the docs for these methods. |
| sourceDocument.Project, sourceDocument.Id, diagnosticIds: null, shouldIncludeAnalyzer: null, | ||
| includeLocalDocumentDiagnostics: true, includeNonLocalDocumentDiagnostics: true, CancellationToken.None); | ||
| includeLocalDocumentDiagnostics: true, CancellationToken.None); | ||
| // await diagnosticIncrementalAnalyzer.GetTestAccessor().TextDocumentOpenAsync(sourceDocument); |
There was a problem hiding this comment.
not you, but could be deleted?
| // await diagnosticIncrementalAnalyzer.GetTestAccessor().TextDocumentOpenAsync(sourceDocument); |
3 participants
Non-local diagnostics are diagnostics reported by an analyzer for file A.cs when the analyzer is actually analyzing B.cs.
Currently, almost mainline features calling into that particular API were having this be the desired result, so removing the flag controlling this was a no-op for them.
One feature was passing in 'false' for this behavior, indicating that it did not want this. However, that was fix-all, which is already analyzing everything anyways, and as such would only be losing information if it didn't collect the diagnostics from tehse analyzers.
Note: this flag only affects which diagnostics are returned. It doesn't get passed through to affect analysis. SO all the same work is being done. IN other words, this flag has no impact on perf, just what is returned. And we do want to always return tehse diags no matter who reported them.