-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Remove 'filters' from IDiagService and make it a pure data api. #80177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
@@ -1060,7 +1060,7 @@ await VerifyCachedDiagnosticsAsync( | |||
: root.DescendantNodes().OfType<CodeAnalysis.CSharp.Syntax.InvocationExpressionSyntax>().First().Span; | |||
|
|||
await analyzerService.GetDiagnosticsForIdsAsync( | |||
sourceDocument.Project, [sourceDocument.Id], diagnosticIds: null, shouldIncludeAnalyzer: null, | |||
sourceDocument.Project, [sourceDocument.Id], diagnosticIds: null, AnalyzerFilter.All, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
callback replaced with pure-data enum. 'null' means 'include everything'.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filter appears to be required and never nullable, or did I miss something
/// <param name="additionalFilter">An additional filter that can accept or reject analyzers that the default | ||
/// rules have accepted.</param> | ||
Func<DiagnosticAnalyzer, bool> GetDefaultAnalyzerFilter( | ||
Project project, ImmutableHashSet<string>? diagnosticIds, Func<DiagnosticAnalyzer, bool>? additionalFilter = null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filter callbacks removed from api, as ti requires processing by teh caller (which is on the VS side, not the OOP side).
|
||
var analyzersForProject = GetProjectAnalyzers(project); | ||
return analyzersForProject.WhereAsArray(shouldIncludeAnalyzer); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these apis still exist. but because they can now immediately ccall over to OOP, they move into teh _RemoteOrLocalDispatch.cs file.
@@ -22,84 +22,81 @@ public async ValueTask<ImmutableArray<DiagnosticData>> ForceRunCodeAnalysisDiagn | |||
// | |||
// As such, we are very intentionally not calling into this.GetDefaultAnalyzerFilter | |||
// here. We want to control the rules entirely when this is called. | |||
var filter = GetDiagnosticAnalyzerFilter(project, _analyzerInfoCache); | |||
var analyzers = GetProjectAnalyzers(project); | |||
var filteredAnalyzers = analyzers.WhereAsArray(ShouldIncludeAnalyzer); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
note, we are in an InProcessAsync method, so it's ok to call GetProjectAnalyzers and do this in-memory processing.
if (analyzer == FileContentLoadAnalyzer.Instance || | ||
analyzer == GeneratorDiagnosticsPlaceholderAnalyzer.Instance || | ||
analyzer.IsCompilerAnalyzer()) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is a move.
} | ||
|
||
static Func<DiagnosticAnalyzer, bool> GetDiagnosticAnalyzerFilter( | ||
Project project, DiagnosticAnalyzerInfoCache infoCache) | ||
bool ShouldIncludeAnalyzer(DiagnosticAnalyzer analyzer) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for callback form. we can just immediately filter the array.
{ | ||
if ((analyzerFilter & AnalyzerFilter.NonCompilerAnalyzer) == 0) | ||
return false; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's below is the standard filtering logic from before. what's above is new.
internal async Task<ImmutableArray<DiagnosticData>> ProduceProjectDiagnosticsAsync( | ||
public async Task<ImmutableArray<DiagnosticData>> GetDiagnosticsForIdsAsync( | ||
Project project, ImmutableArray<DocumentId> documentIds, ImmutableHashSet<string>? diagnosticIds, AnalyzerFilter analyzerFilter, bool includeLocalDocumentDiagnostics, CancellationToken cancellationToken) | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this moved from original file to this dispatch file. Then it calls to the actual in proc impl at the bottom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment
@@ -1060,7 +1060,7 @@ await VerifyCachedDiagnosticsAsync( | |||
: root.DescendantNodes().OfType<CodeAnalysis.CSharp.Syntax.InvocationExpressionSyntax>().First().Span; | |||
|
|||
await analyzerService.GetDiagnosticsForIdsAsync( | |||
sourceDocument.Project, [sourceDocument.Id], diagnosticIds: null, shouldIncludeAnalyzer: null, | |||
sourceDocument.Project, [sourceDocument.Id], diagnosticIds: null, AnalyzerFilter.All, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
filter appears to be required and never nullable, or did I miss something
Followup to #80176
These filters meant we had to instantiate DiagnosticAnalyzers on the host, and pass those in memory objects back to callers. This clearly breaks our goal of only loading DiagAnalyzers OOP.
This moves us to a pure-data api where callers don't pass callbacks, but instead a simple enum controlling what they want. And only on OOP do we manipulate the DiagnosticAnalyzers.