Remove the 'force compute' entrypoint for diagnostics #80060
Conversation
CyrusNajmabadi
force-pushed
the
noForceAnalyze
branch
2 times, most recently
from
8f17d74 to
58f80cd
Compare
| // document diagnostics and also does not add any delay for pulling workspace diagnostics. | ||
| _diagnosticAnalyzerService.RequestDiagnosticRefresh(); | ||
|
|
||
| bool FilterAnalyzer(DiagnosticAnalyzer analyzer) |
There was a problem hiding this comment.
this is a move from teh diag analzyer service itself. i want to take it out of the job of figuring these things out.
| using Microsoft.CodeAnalysis.Collections; | ||
| using Microsoft.CodeAnalysis.Diagnostics; | ||
| using Microsoft.CodeAnalysis.Editor.Implementation.Suggestions; | ||
| using Microsoft.CodeAnalysis.Editor.UnitTests.Extensions; |
There was a problem hiding this comment.
bridging tests to new api by use of an extension method.
|
@jasonmalinowski @dibarbet ptal. |
|
|
||
| internal static class IDiagnosticServiceExtensions | ||
| { | ||
| public static Task<ImmutableArray<DiagnosticData>> ForceAnalyzeProjectAsync( |
There was a problem hiding this comment.
unit test only extension for tests that want to validate the diags that CodeAnalysisDiagnosticAnalyzerService gets back. I will do a future pr that has these features literally just test that service instead of diving into one of its helper methosd.
| private static Func<DiagnosticAnalyzer, bool> GetDiagnosticAnalyzerFilter( | ||
| Project project, DiagnosticAnalyzerInfoCache infoCache) | ||
| { | ||
| return analyzer => |
There was a problem hiding this comment.
this logic is a move. but it allows CodeAnalysisDiagnosticAnalyzerService to be entire explicit about which analyzers it runs as that is specific to its scenario. the DiagAnalService doesn't need to care or think about it.
| using System.Threading.Tasks; | ||
| using Microsoft.CodeAnalysis.CodeActions; | ||
| using Microsoft.CodeAnalysis.Host; | ||
| using Microsoft.CodeAnalysis.SolutionCrawler; |
There was a problem hiding this comment.
only used for doc comments crefs.
| void RequestDiagnosticRefresh(); | ||
|
|
||
| /// <inheritdoc cref="IRemoteDiagnosticAnalyzerService.ForceAnalyzeProjectAsync"/> | ||
| Task<ImmutableArray<DiagnosticData>> ForceAnalyzeProjectAsync(Project project, CancellationToken cancellationToken); |
There was a problem hiding this comment.
removing this was the primary goal of this PR.
| /// </summary> | ||
| /// <param name="additionalFilter">An additional filter that can accept or reject analyzers that the default | ||
| /// rules have accepted.</param> | ||
| Func<DiagnosticAnalyzer, bool> GetDefaultAnalyzerFilter( |
There was a problem hiding this comment.
codifying what this is, and making it something a caller can override if they do not want it.
| var checksum = await project.GetDiagnosticChecksumAsync(cancellationToken).ConfigureAwait(false); | ||
| if (box.Value.checksum == checksum) | ||
| return box.Value.diagnosticAnalysisResults; | ||
| } |
There was a problem hiding this comment.
technically we lose this capability. But that's fine by me. 'run code analysis' isn't a common op, and we're going to be computing diags anyways for documents even in the presence of this having been called.
There was a problem hiding this comment.
I wonder if this sort of a subset logic might be generally useful for that question you asked earlier about why we're creating so many CompilationWithAnalyzers for different analyzer sets and whether we can be more generally reusing prior answers. But I agree on removing this code -- having an optimization for specifically after this gesture seems unnecessary.
| /// <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.
It does feel a little weird that we pass another filter to retrieve the default filter. Wondering if places that use the additionalFilter should instead be creating their own single filter (which may wrap a call to the default filter), instead of it being passed in?
It might make usages more clear that even though they are getting the default filter, they may not be using the default filter behavior.
There was a problem hiding this comment.
Agreeing here -- my assumption would have been that the additional filter was only used for ones this didn't answer on, but I guess the comment says it's the other way around.
Or maybe just have a pattern that this filter method returns a nullable boolean, and then have a Compose method somewhere that takes a bunch of filters, calls them in order, and has some default true/false to fallback to if nobody answers.
There was a problem hiding this comment.
Hrmm... i'll see if there's a better way to do this. Originally, i just returned the filtetr, and then the callers had to combine it with theirs.
In practice, the pattern was always teh same. So having this pattern just helped encode both the idea of:
- get the default rules
- get the default rules, augmented with some other set.
So with this, and the ability for someone to pass a callback to GetDiagnostics meant we could handle all 3 cases:
- get the default rules
- get the default rules, augmented with some other set.
- use an entirely custom set of rules
But yeah, i agree it's not necessarily stunning here...
| // For most of analyzers, the number of diagnostic descriptors is small, so this should be cheap. | ||
| var descriptors = infoCache.GetDiagnosticDescriptors(analyzer); | ||
| var analyzerConfigOptions = project.GetAnalyzerConfigOptions(); | ||
| return descriptors.Any(static (d, arg) => |
There was a problem hiding this comment.
This bit is actually readable now, thanks.
There was a problem hiding this comment.
Oh good. I'm glad!
4 participants
This entrypoint existed so that the "run code analysis" command could work. Specifically, because that is an explicit comamnd kicked off by the user for the exact purpose of analyzing everything, we needed a way to run analyzer and not have our default bail-out logic come into play. =
For example, if the user has set 'run analyzers on the active file' (the default) we clearly don't want that stopping us from getting analyzer diags when teh user explicitly kicks off 'run code analysis'.
This was always very clunky, especially since 'run code analysis' is a separate component that then drives the low level diagnostic analyzer service.
This PR changes things so that the core entrypoints into the diag analyzer service (in this case GetDiagnosticsForIds) is now flexible enough to just handle what 'run code analysis' needs. First, #80084 made it so that the caller can specify the documents they want diags for no matter what. Second, this PR makes it so that the caller has complete control if needed over which analyzers to run, avoiding the default impl which uses heuristics to filter out certain analyzers it doesn't think are necessary.
This is all with the goal of making IDiagAnalyzerService very simple and clear in terms of the semantics it provides and when and how to use its entrypoints.