Push diagnostic suppression filtering flag to feature levels #76849
Conversation
dotnet-issue-labeler
bot
added
Area-IDE
untriaged
| @@ -128,7 +128,6 @@ private async Task ProduceTagsAsync( | |||
| document, | |||
| requestedSpan.Span.ToTextSpan(), | |||
| diagnosticKind: _diagnosticKind, | |||
| includeSuppressedDiagnostics: true, | |||
There was a problem hiding this comment.
Note: if a featuree passes includeSuppressedDiagnostics: true, it needs no change. We always return all diagnostics by default, including suppressed ones.
| @@ -69,7 +69,8 @@ public async Task TestHasSuccessfullyLoadedBeingFalse() | |||
|
|
|||
| var diagnostics = await analyzer.GetDiagnosticsForIdsAsync( | |||
| workspace.CurrentSolution, projectId: null, documentId: null, diagnosticIds: null, shouldIncludeAnalyzer: null, getDocuments: null, | |||
| includeSuppressedDiagnostics: false, includeLocalDocumentDiagnostics: true, includeNonLocalDocumentDiagnostics: false, CancellationToken.None); | |||
| includeLocalDocumentDiagnostics: true, includeNonLocalDocumentDiagnostics: false, CancellationToken.None); | |||
| diagnostics = diagnostics.WhereAsArray(d => !d.IsSuppressed); | |||
There was a problem hiding this comment.
note: code that passes includeSuppressedDiagnostics: false needs to filter the results. I do that for features. HOwever, for tests, it doesn't seem to have any impact on results. so for simplicity i'm not doing it.
| @@ -69,7 +69,8 @@ public async Task TestHasSuccessfullyLoadedBeingFalse() | |||
|
|
|||
| var diagnostics = await analyzer.GetDiagnosticsForIdsAsync( | |||
| workspace.CurrentSolution, projectId: null, documentId: null, diagnosticIds: null, shouldIncludeAnalyzer: null, getDocuments: null, | |||
| includeSuppressedDiagnostics: false, includeLocalDocumentDiagnostics: true, includeNonLocalDocumentDiagnostics: false, CancellationToken.None); | |||
There was a problem hiding this comment.
note: code that passes includeSuppressedDiagnostics: false needs to filter the results. I do that for features. HOwever, for tests, it doesn't seem to have any impact on results. so for simplicity i'm not doing it.
| var diagnostics = await _diagnosticAnalyzerService.GetCachedDiagnosticsAsync( | ||
| _workspace, documentId.ProjectId, documentId, includeLocalDocumentDiagnostics: true, | ||
| includeNonLocalDocumentDiagnostics: true, cancellationToken).ConfigureAwait(false); | ||
| return diagnostics.WhereAsArray(d => !d.IsSuppressed); |
There was a problem hiding this comment.
any place that previously passed includeSuppressedDiagnostics: false into the service, now just filters the result.
Note: i wouldn't mind having this not be here, and pushignt his even higher all the way to the feature layer. but this is a simple starting step.
| /// Flag indicating if suppressed diagnostics should be returned. | ||
| /// </summary> | ||
| [DataMember(Order = 0)] | ||
| public bool ReportSuppressedDiagnostics; |
There was a problem hiding this comment.
not needed. they are always returned.
| TextDocument document, TextSpan? range, DiagnosticKind diagnosticKind, CancellationToken cancellationToken) | ||
| => service.GetDiagnosticsForSpanAsync( | ||
| document, range, | ||
| diagnosticId: null, | ||
| priorityProvider: new DefaultCodeActionRequestPriorityProvider(), | ||
| diagnosticKind, isExplicit: false, cancellationToken); |
There was a problem hiding this comment.
any code that just passed the flag along just has it removed.
| callback(diagnostic); | ||
| } | ||
| } | ||
|
|
||
| private bool ShouldIncludeSuppressedDiagnostic(DiagnosticData diagnostic) | ||
| => IncludeSuppressedDiagnostics || !diagnostic.IsSuppressed; |
There was a problem hiding this comment.
this is what the flag was passed down all the way to do.
|
@ToddGrun ptal. |
src/Features/Core/Portable/Diagnostics/CodeAnalysisDiagnosticAnalyzerService.cs
Show resolved
Hide resolved
src/LanguageServer/Protocol/Features/CodeFixes/CodeFixService.cs
Outdated
Show resolved
Hide resolved
src/Workspaces/Core/Portable/Diagnostics/CompilationWithAnalyzersPair.cs
Show resolved
Hide resolved
.../Protocol/Handler/Diagnostics/DiagnosticSources/AbstractWorkspaceDocumentDiagnosticSource.cs
Outdated
Show resolved
Hide resolved
| @@ -50,7 +50,8 @@ private async Task<IEnumerable<Diagnostic>> GetDiagnosticsAsync( | |||
| var text = await document.GetTextAsync().ConfigureAwait(false); | |||
| var dxs = await _diagnosticAnalyzerService.GetDiagnosticsForIdsAsync( | |||
| project.Solution, project.Id, document.Id, diagnosticIds: null, shouldIncludeAnalyzer: null, | |||
| _includeSuppressedDiagnostics, includeLocalDocumentDiagnostics: true, _includeNonLocalDocumentDiagnostics, CancellationToken.None); | |||
| includeLocalDocumentDiagnostics: true, _includeNonLocalDocumentDiagnostics, CancellationToken.None); | |||
| dxs = dxs.WhereAsArray(d => _includeSuppressedDiagnostics || !d.IsSuppressed); | |||
There was a problem hiding this comment.
enh. this is tests. i'm fine with this.
This flag was pushed all the way down to the compiler layer. But all the compiler uses it for is for filtering out diagnostics with the .IsSuppressed bit on it. This is a lot of scaffolding through lots of complex layers (including caching layers) only to just filter the diagnostics. There are not that many features that actually consume diagnostics, so we it's fine to just compute all the diagnostics like normal and have the filtering be at the feature level.