Simplify caching api between CodeAnalysisService and DiagnosticAnalysisService#77094
Simplify caching api between CodeAnalysisService and DiagnosticAnalysisService#77094CyrusNajmabadi merged 12 commits intodotnet:mainfrom
Conversation
| using var workspace = TestWorkspace.CreateCSharp(code, composition: s_compositionWithMockDiagnosticUpdateSourceRegistrationService, openDocuments: true); | ||
|
|
||
| var diagnosticService = Assert.IsType<DiagnosticAnalyzerService>(workspace.GetService<IDiagnosticAnalyzerService>()); | ||
| var diagnosticService = workspace.GetService<IDiagnosticAnalyzerService>(); |
There was a problem hiding this comment.
removed all bizarre casting in the tests. tests should validate the actual interface to the rest of the code.
There was a problem hiding this comment.
also, we truly only have one impl of this. so all this type testing is really bizarre.
| var globalOptions = exportProvider.GetExportedValue<IGlobalOptionService>(); | ||
|
|
||
| var diagnostics = await analyzer.GetDiagnosticsForIdsAsync( | ||
| var diagnostics = await service.GetDiagnosticsForIdsAsync( |
There was a problem hiding this comment.
removed bizarre testing of internal implementation details. Tests should go through the service, not bypass it.
There was a problem hiding this comment.
in a followup, i will make DiagnosticIncrementalAnalyzer private and a nested type inside DIagnosticAnalyzerService.
| /// </summary> | ||
| private readonly ConcurrentSet<ProjectId> _analyzedProjectIds = []; | ||
| private readonly ConcurrentDictionary<ProjectId, ImmutableArray<DiagnosticData>> _analyzedProjectToDiagnostics = []; | ||
|
|
There was a problem hiding this comment.
much simpler model. instead of calling onto the service to compute diagnostics and then calling back in to fetch whatever it had cached when we asked it to compute, we just grab and cache the values ourselves whne we ask it to compute. THus not needing any sort of coordination.
| /// complete set of non-local document diagnostics. | ||
| /// </remarks> | ||
| Task<ImmutableArray<DiagnosticData>> GetCachedDiagnosticsAsync( | ||
| Workspace workspace, ProjectId projectId, DocumentId? documentId, CancellationToken cancellationToken); |
There was a problem hiding this comment.
this whole API can go away from IDiagAnalyzerService now, as it was only used in one place that now owns caching itself.
| /// Force analyzes the given project by running all applicable analyzers on the project and caching the reported analyzer diagnostics. | ||
| /// </summary> | ||
| Task ForceAnalyzeProjectAsync(Project project, CancellationToken cancellationToken); | ||
| Task<ImmutableArray<DiagnosticData>> ForceAnalyzeProjectAsync(Project project, CancellationToken cancellationToken); |
There was a problem hiding this comment.
note: the internal api for this already returned these values (which is what unit tests were validating to begin with). This removes any sort of impedence mismatch where tests were validating one thing, but the product code could potentially experience something else because it went through a different untested api.
| { | ||
| var analyzer = CreateIncrementalAnalyzer(workspace); | ||
| return analyzer.GetCachedDiagnosticsAsync(workspace.CurrentSolution, projectId, documentId, cancellationToken); | ||
| } |
| internal partial class DiagnosticIncrementalAnalyzer | ||
| { | ||
| public Task<ImmutableArray<DiagnosticData>> GetCachedDiagnosticsAsync(Solution solution, ProjectId projectId, DocumentId? documentId, CancellationToken cancellationToken) | ||
| => new IdeCachedDiagnosticGetter(this, solution, projectId, documentId).GetDiagnosticsAsync(cancellationToken); |
| var nonLocalResult = await state.GetProjectAnalysisDataAsync(project, cancellationToken: cancellationToken).ConfigureAwait(false); | ||
| return nonLocalResult.GetOtherDiagnostics(); | ||
| } | ||
| } |
There was a problem hiding this comment.
yaay, a massive amount of complexity just gone.
| /// and <see cref="GetLastComputedProjectDiagnostics"/>. | ||
| /// </summary> | ||
| private readonly ConcurrentSet<ProjectId> _analyzedProjectIds = []; | ||
| private readonly ConcurrentDictionary<ProjectId, ImmutableArray<DiagnosticData>> _analyzedProjectToDiagnostics = []; |
There was a problem hiding this comment.
couple of questions
- it seems a little weird to me that the diagnostic data is associated with a
ProjectIdinstead of aProject. If the snapshot is different, is the DiagnosticData valid to be re-used? I would assume spans could be all over the place. I can't quite tell, but the previous code did seem to be somewhat associated with aProject? - related to 1), how concerned are we that this cache could potentially go backwards? e.g. diagnostics for version1 of the project return slower, and overwrite diagnostics calculated for version2?
- related to 2), is it possible that different callers end up caching different sets of diagnostics? e.g. one caller asks for diagnostics without non-local diagnostics (or w.e)
There was a problem hiding this comment.
taking offline.
There was a problem hiding this comment.
Answer for 1:
So, remember this is 'Run Code Analysis'. Which a user kicks off manually,. The analysis runs on the 'current solution snapshot' at the time, and caches the results indefinitely (till the next time it is run), or the next time 'Build' is run by the user.
In this case, we want to stores the computed diagnostics around indefinitely, allowing the LSP pull diags system to see that 'old computed snapshot' of the project.
we do not want to store using Project as the key for two reasons:
- it will root an old project instance a long time.
- the LSP pull diags is asking for the old computed diags for the current project it has.
so ProjectId is the appropriate key for it.
--
the answers to the rest of the questions follow from this.
No description provided.