OOP #1 - new analyzer engine that uses CompilerAnalyzer model #10509
Conversation
| @@ -117,22 +117,20 @@ Namespace Microsoft.CodeAnalysis.Editor.Implementation.Diagnostics.UnitTests | |||
| Assert.Equal(workspaceDiagnosticAnalyzer.DiagDescriptor.Id, descriptors(0).Id) | |||
|
|
|||
| ' Add an existing workspace analyzer to the project, ensure no duplicate diagnostics. | |||
| Dim duplicateProjectAnalyzers = ImmutableArray.Create(Of DiagnosticAnalyzer)(workspaceDiagnosticAnalyzer) | |||
| Dim duplicateProjectAnalyzersReference = New AnalyzerImageReference(duplicateProjectAnalyzers) | |||
| Dim duplicateProjectAnalyzersReference = diagnosticService.HostAnalyzerReferences.FirstOrDefault() | |||
There was a problem hiding this comment.
our dedup logic is based on reference not analyzer. not sure how it worked before. anyway, made the test to actually test what we dedup.
|
|
||
| // REVIEW: more unnecessary allocations just to get diagnostics per analyzer | ||
| var oneAnalyzers = ImmutableArray.Create(analyzer); | ||
|
|
There was a problem hiding this comment.
need new API from compilerWithAnalyzer to not do this. once @mavasani provides me one, I will replace this with new API.
changed test to use GetDiagnosticsAsync rather than GetProjectDiagnosticsAsync
|
|
||
| namespace Microsoft.CodeAnalysis.Editor.UnitTests.Diagnostics | ||
| { | ||
| public class DiagnosticDataSerializerTests : TestBase |
There was a problem hiding this comment.
Are these new tests being added? Are they specific to v2 engine?
|
|
||
| foreach (var documentId in lastResult.DocumentIds) | ||
| { | ||
| var document = project.GetDocument(documentId); |
There was a problem hiding this comment.
Check for cancellation in the loop?
added assert for GetEffectiveDiagnostics
| @@ -12,10 +11,14 @@ namespace Microsoft.CodeAnalysis.Diagnostics | |||
| /// </summary> | |||
| internal abstract class DocumentDiagnosticAnalyzer : DiagnosticAnalyzer | |||
| { | |||
| // REVIEW: why DocumentDiagnosticAnalyzer doesn't have span based analysis? | |||
There was a problem hiding this comment.
Because the point is to analyze the entire document :)
This is the extension point for other languages (like TS) to plugin and do their analysis of the document. How would we pick the spans with which ot analyze?
There was a problem hiding this comment.
span based analyzer means caller ask span to analyze not analyzer itself determine span to analyze. so if an analyzer has ability to return diagnostics based on given span, it can claim it is span based (or my wording might be wrong that confused you)
| continue; | ||
| } | ||
|
|
||
| // REVIEW: is build diagnostic contains suppressed diagnostics? |
There was a problem hiding this comment.
Build will never report suppressed diagnostics - at least it is not supported right now. If /errorlog is specified to build, the error log will have suppressed diagnostics, but command line output will not.
There was a problem hiding this comment.
hmmm that means with build/live dedup logic, as soon as user build, user will lose all suppressed errors... I might need to save suppressed diagnostics when we do de-dup.
Will we at least be able to prioritize analysis? That way we get diagnostics for the files that the user is editing quickly? Thanks! |
| } | ||
| } | ||
|
|
||
| private bool ShouldIncludeSuppressedDiagnostic(DiagnosticData diagnostic) |
There was a problem hiding this comment.
Rename to ShouldIncludeDiagnostic?
There was a problem hiding this comment.
that one already exist, so I added Suppressed :)
|
@CyrusNajmabadi that is still the way it works. active file/open file will get highest priority and analyzed per file as before. |
|
@dotnet/roslyn-analysis @mattwar can you take a look? |
| return _diagnosticIds == null || _diagnosticIds.Contains(diagnostic.Id); | ||
| } | ||
|
|
||
| protected override async Task AppendDiagnosticsAsync(Project project, DocumentId targetDocumentId, IEnumerable<DocumentId> documentIds, CancellationToken cancellationToken) |
There was a problem hiding this comment.
This API seems very weird - it takes project, targetDocument and then also an enumerable of documents. It doesn't seem clear for someone invoking this API what exact set of diagnostics will be appended and which arguments may or may not be null.
There was a problem hiding this comment.
changed it to use flag instead.
|
@heejaechang Great! One quesiton though. Say the user is in File A in Compilation A. So we're prioritizing that. Then they jump to File B in compilation B, we can jump to prioritizing that file right? We don't need to wait for Compilation A to finish? |
| var stateSets = _stateManager.GetOrUpdateStateSets(document.Project); | ||
| var analyzerDriverOpt = await _compilationManager.GetAnalyzerDriverAsync(document.Project, stateSets, cancellationToken).ConfigureAwait(false); | ||
|
|
||
| foreach (var stateSet in stateSets) |
There was a problem hiding this comment.
Should we bail out early for unsuppressed analyzers, just like you do in line 74 below for AnalyzeProjectAsync?
There was a problem hiding this comment.
I didn't do that to make sure stateSets is immutable. (since I cache CompilationWithAnalyzer based only on Project snapshot). can IsAnalyzerSuppressed be changed with same project snapshot?
There was a problem hiding this comment.
let's see whether not filtering those out will cause any perf issue. we can add it later with a bit of precaution later if it affects perf.
|
@CyrusNajmabadi depends on how fast compilation A analysis respond to cancellation. when the situation you mentioned happen, we will raise cancellation and as soon as that get cancelled, we will process the active file that is switched. (assuming it has change otherwise, it should already have right diagnostics) |
|
Ah, ok. As long as we don't have to unconditionally wait on Compilation A finishing, them i'm fine. I think Cancellation is a reasonable way to accomplish this. |
…tually no action for the analyzer
added flag for including project non local result.
|
I am checking this in before branch switch. feature itself is off by default so it shouldn't affect anything. and I got ok from @mavasani offline. |
this is a rewrite of our analyzer engine (V2)
unlike the v1, we no longer tries to do incremental analysis per document (except active/open files) rather we follow compiler model where we do every work in one shot.
we realized implementing this incremental analysis on top of compiler model (which doesn't support this incremental analysis naturally) cost more than just doing everything at once.
this should let us to remove any need to maintain state in compilationWithAnalyzer to track in progress state.
also, this make implementation a bit less complicated than before.