Skip to content

Commit

Permalink
Merge pull request #3105 from mavasani/DuplicateAnalyzerCallbacks
Browse files Browse the repository at this point in the history 
Prevent duplicate invocations of per-compilation analyzer actions for a given symbol/syntax node in IDE.

User scenario: An analyzer author writes a compilation end analyzer, which registers symbol and/or syntax node actions to maintain some state about them, and a compilation end action to report diagnostics based on the final sate. When running in the IDE, the analyzer will get duplicate callbacks to the symbol/syntax node actions for the same symbol/node in the compilation: once while computing active document diagnostics and once while computing project diagnostics. This can cause one of the following issues:

  1. Crash the analyzer if it is using some map keyed on the callback symbol/node, and doesn't handle duplicate callbacks.
  2. Corrupt the analyzer state by recording the state twice for a single symbol/node.
  3. Possibly cause the analyzer to enter a race or deadlock.

Fix Description: Fix is to ensure that when computing the compilation end diagnostics for such stateful analyzers, we use a new instance of the analyzer. All the compilation wide actions are then invoked on this new instance of the analyzer, avoiding duplicate callbacks into the same instance.

Fixes #248 

Testing: Added a regression test + existing tests.

Approved 5/29 by ML Shiproom.
  • Loading branch information
@mavasani
mavasani committed May 29, 2015
2 parents e30ebe5 + 7943a17 commit 0154c37
Show file tree
Hide file tree
Showing 4 changed files with 99 additions and 27 deletions.
34 changes: 33 additions & 1 deletion src/EditorFeatures/Test2/Diagnostics/DiagnosticServiceTests.vb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,38 @@ class AnonymousFunctions
End Using
End Sub

<Fact, WorkItem(248, "https://github.com/dotnet/roslyn/issues/248"), Trait(Traits.Feature, Traits.Features.Diagnostics)>
Public Sub TestStatefulCompilationAnalyzer_2()
Dim test = <Workspace>
<Project Language="C#" CommonReferences="true">
<Document FilePath="Test.cs">
class Foo { void M() {} }
</Document>
</Project>
</Workspace>

Using workspace = TestWorkspaceFactory.CreateWorkspace(test)
Dim project = workspace.CurrentSolution.Projects.Single()
Dim analyzer = New StatefulCompilationAnalyzer
Dim analyzerReference = New AnalyzerImageReference(ImmutableArray.Create(Of DiagnosticAnalyzer)(analyzer))
project = project.AddAnalyzerReference(analyzerReference)

' Make couple of dummy invocations to GetDocumentDiagnostics.
Dim document = project.Documents.Single()
Dim fullSpan = document.GetSyntaxRootAsync().WaitAndGetResult(CancellationToken.None).FullSpan
Dim documentDiagnostics = DiagnosticProviderTestUtilities.GetDocumentDiagnostics(workspaceAnalyzerOpt:=Nothing, document:=document, span:=fullSpan)
documentDiagnostics = DiagnosticProviderTestUtilities.GetDocumentDiagnostics(workspaceAnalyzerOpt:=Nothing, document:=document, span:=fullSpan)

' Verify that the eventual compilation end diagnostics (and hence the analyzer state) is not affected by prior document analysis.
Dim projectDiagnostics = DiagnosticProviderTestUtilities.GetProjectDiagnostics(workspaceAnalyzerOpt:=Nothing, project:=project)
Assert.Equal(1, projectDiagnostics.Count())
Dim diagnostic = projectDiagnostics.Single()
Assert.Equal(StatefulCompilationAnalyzer.Descriptor.Id, diagnostic.Id)
Dim expectedMessage = String.Format(StatefulCompilationAnalyzer.Descriptor.MessageFormat.ToString(), 1)
Assert.Equal(expectedMessage, diagnostic.GetMessage)
End Using
End Sub

<Fact, WorkItem(1042914), Trait(Traits.Feature, Traits.Features.Diagnostics)>
Public Sub TestPartialTypeInGeneratedCode()
Dim test = <Workspace>
Expand Down Expand Up @@ -1372,7 +1404,7 @@ public class B
End Sub

Private Class CompilationAnalyzer
Private ReadOnly _symbolNames As New ConcurrentSet(Of String)
Private ReadOnly _symbolNames As New List(Of String)

Public Sub AnalyzeSymbol(context As SymbolAnalysisContext)
_symbolNames.Add(context.Symbol.Name)
Expand Down
77 changes: 57 additions & 20 deletions src/Features/Core/Diagnostics/EngineV1/DiagnosticAnalyzerDriver.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,7 @@ public async Task<ImmutableArray<Diagnostic>> GetSyntaxDiagnosticsAsync(Diagnost
}
}

var analyzerExecutor = GetAnalyzerExecutor(analyzer, compilation, diagnostics.Add);
var analyzerExecutor = GetAnalyzerExecutor(compilation, diagnostics.Add);
var analyzerActions = await this.GetAnalyzerActionsAsync(analyzer, analyzerExecutor).ConfigureAwait(false);
if (analyzerActions != null)
{
Expand Down Expand Up @@ -298,15 +298,24 @@ internal void OnAnalyzerException(Exception ex, DiagnosticAnalyzer analyzer, Com
_onAnalyzerException(ex, analyzer, exceptionDiagnostic);
}

private AnalyzerExecutor GetAnalyzerExecutor(DiagnosticAnalyzer analyzer, Compilation compilation, Action<Diagnostic> addDiagnostic)
private AnalyzerExecutor GetAnalyzerExecutor(Compilation compilation, Action<Diagnostic> addDiagnostic)
{
return AnalyzerExecutor.Create(compilation, _analyzerOptions, addDiagnostic, _onAnalyzerException, AnalyzerHelper.IsCompilerAnalyzer, AnalyzerManager.Instance, s_analyzerGateMap.GetOrCreateValue, cancellationToken: _cancellationToken);
}

private AnalyzerExecutor GetAnalyzerExecutorForClone(Compilation compilation, Action<Diagnostic> addDiagnostic, DiagnosticAnalyzer analyzerForExceptionDiagnostics)
{
// Report analyzer exception diagnostics on original analyzer, not the temporary cloned one.
Action<Exception, DiagnosticAnalyzer, Diagnostic> onAnalyzerException = (ex, a, diag) =>
_onAnalyzerException(ex, analyzerForExceptionDiagnostics, diag);

return AnalyzerExecutor.Create(compilation, _analyzerOptions, addDiagnostic, onAnalyzerException, AnalyzerHelper.IsCompilerAnalyzer, AnalyzerManager.Instance, s_analyzerGateMap.GetOrCreateValue, cancellationToken: _cancellationToken);
}

public async Task<AnalyzerActions> GetAnalyzerActionsAsync(DiagnosticAnalyzer analyzer)
{
var compilation = _project.SupportsCompilation ? await _project.GetCompilationAsync(_cancellationToken).ConfigureAwait(false) : null;
var analyzerExecutor = GetAnalyzerExecutor(analyzer, compilation, addDiagnostic: null);
var analyzerExecutor = GetAnalyzerExecutor(compilation, addDiagnostic: null);
return await GetAnalyzerActionsAsync(analyzer, analyzerExecutor).ConfigureAwait(false);
}

Expand Down Expand Up @@ -362,7 +371,7 @@ public async Task<ImmutableArray<Diagnostic>> GetSemanticDiagnosticsAsync(Diagno
}
else
{
var analyzerExecutor = GetAnalyzerExecutor(analyzer, compilation, diagnostics.Add);
var analyzerExecutor = GetAnalyzerExecutor(compilation, diagnostics.Add);
var analyzerActions = await GetAnalyzerActionsAsync(analyzer, analyzerExecutor).ConfigureAwait(false);
if (analyzerActions != null)
{
Expand Down Expand Up @@ -445,27 +454,55 @@ private async Task GetCompilationDiagnosticsAsync(DiagnosticAnalyzer analyzer, L
{
var localDiagnostics = pooledObject.Object;
var compilation = await _project.GetCompilationAsync(_cancellationToken).ConfigureAwait(false);
DiagnosticAnalyzer clonedAnalyzer = null;

// Get all the analyzer actions, including the per-compilation actions.
var analyzerExecutor = GetAnalyzerExecutor(analyzer, compilation, localDiagnostics.Add);
var analyzerActions = await this.GetAnalyzerActionsAsync(analyzer, analyzerExecutor).ConfigureAwait(false);
var hasDependentCompilationEndAction = await AnalyzerManager.Instance.GetAnalyzerHasDependentCompilationEndAsync(analyzer, analyzerExecutor).ConfigureAwait(false);

if (hasDependentCompilationEndAction && forceAnalyzeAllDocuments != null)
try
{
// Analyzer registered a compilation end action and at least one other analyzer action during its compilation start action.
// We need to ensure that we have force analyzed all documents in this project for this analyzer before executing the end actions.
forceAnalyzeAllDocuments(_project, analyzer, _cancellationToken);
}
// Get all the analyzer actions, including the per-compilation actions.
var analyzerExecutor = GetAnalyzerExecutor(compilation, localDiagnostics.Add);
var analyzerActions = await this.GetAnalyzerActionsAsync(analyzer, analyzerExecutor).ConfigureAwait(false);
var hasDependentCompilationEndAction = await AnalyzerManager.Instance.GetAnalyzerHasDependentCompilationEndAsync(analyzer, analyzerExecutor).ConfigureAwait(false);

if (hasDependentCompilationEndAction && forceAnalyzeAllDocuments != null)
{
// Analyzer registered a compilation end action and at least one other analyzer action during its compilation start action.
// We need to ensure that we have force analyzed all documents in this project for this analyzer before executing the end actions.
// Doing so on the original analyzer instance might cause duplicate callbacks into the analyzer.
// So we create a new instance of this analyzer and compute compilation diagnostics on that instance.

try
{
clonedAnalyzer = Activator.CreateInstance(analyzer.GetType()) as DiagnosticAnalyzer;
}
catch
{
// Unable to created a new analyzer instance, bail out on reporting diagnostics.
return;
}

// Report analyzer exception diagnostics on original analyzer, not the temporary cloned one.
analyzerExecutor = GetAnalyzerExecutorForClone(compilation, localDiagnostics.Add, analyzerForExceptionDiagnostics: analyzer);

analyzerActions = await this.GetAnalyzerActionsAsync(clonedAnalyzer, analyzerExecutor).ConfigureAwait(false);
forceAnalyzeAllDocuments(_project, clonedAnalyzer, _cancellationToken);
}

// Compilation actions.
analyzerExecutor.ExecuteCompilationActions(analyzerActions.CompilationActions);
// Compilation actions.
analyzerExecutor.ExecuteCompilationActions(analyzerActions.CompilationActions);

// CompilationEnd actions.
analyzerExecutor.ExecuteCompilationActions(analyzerActions.CompilationEndActions);
// CompilationEnd actions.
analyzerExecutor.ExecuteCompilationActions(analyzerActions.CompilationEndActions);

var filteredDiagnostics = CompilationWithAnalyzers.GetEffectiveDiagnostics(localDiagnostics, compilation);
diagnostics.AddRange(filteredDiagnostics);
var filteredDiagnostics = CompilationWithAnalyzers.GetEffectiveDiagnostics(localDiagnostics, compilation);
diagnostics.AddRange(filteredDiagnostics);
}
finally
{
if (clonedAnalyzer != null)
{
AnalyzerManager.Instance.ClearAnalyzerState(ImmutableArray.Create(clonedAnalyzer));
}
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/Features/Core/Diagnostics/EngineV1/DiagnosticIncrementalAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -515,7 +515,7 @@ private static async Task<bool> ShouldRunAnalyzerForStateTypeAsync(DiagnosticAna
internal void ForceAnalyzeAllDocuments(Project project, DiagnosticAnalyzer analyzer, CancellationToken cancellationToken)
{
var diagnosticIds = Owner.GetDiagnosticDescriptors(analyzer).Select(d => d.Id).ToImmutableHashSet();
ReanalyzeAllDocumentsAsync(project, diagnosticIds, cancellationToken).Wait(cancellationToken);
ReanalyzeAllDocumentsAsync(project, analyzer, diagnosticIds, cancellationToken).Wait(cancellationToken);
}

public override void LogAnalyzerCountSummary()
Expand Down
13 changes: 8 additions & 5 deletions src/Features/Core/Diagnostics/EngineV1/DiagnosticIncrementalAnalyzer_GetDiagnostics.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@ public override Task<ImmutableArray<DiagnosticData>> GetProjectDiagnosticsForIds
return new IDELatestDiagnosticGetter(this, diagnosticIds, concurrent: projectId == null).GetProjectDiagnosticsAsync(solution, projectId, cancellationToken);
}

private Task ReanalyzeAllDocumentsAsync(Project project, ImmutableHashSet<string> diagnosticIds, CancellationToken cancellationToken)
private Task ReanalyzeAllDocumentsAsync(Project project, DiagnosticAnalyzer analyzer, ImmutableHashSet<string> diagnosticIds, CancellationToken cancellationToken)
{
return new ReanalysisDiagnosticGetter(this, diagnosticIds).ReanalyzeAllDocumentsAsync(project, cancellationToken);
return new ReanalysisDiagnosticGetter(this, analyzer, diagnosticIds).ReanalyzeAllDocumentsAsync(project, cancellationToken);
}

private abstract class DiagnosticsGetter
Expand Down Expand Up @@ -473,8 +473,11 @@ private DiagnosticLogAggregator DiagnosticLogAggregator

private class ReanalysisDiagnosticGetter : LatestDiagnosticsGetter
{
public ReanalysisDiagnosticGetter(DiagnosticIncrementalAnalyzer owner, ImmutableHashSet<string> diagnosticIds) : base(owner, diagnosticIds)
private readonly DiagnosticAnalyzer _analyzer;

public ReanalysisDiagnosticGetter(DiagnosticIncrementalAnalyzer owner, DiagnosticAnalyzer analyzer, ImmutableHashSet<string> diagnosticIds) : base(owner, diagnosticIds)
{
_analyzer = analyzer;
}

public async Task ReanalyzeAllDocumentsAsync(Project project, CancellationToken cancellationToken)
Expand All @@ -500,10 +503,10 @@ protected override async Task<AnalysisData> GetDiagnosticAnalysisDataAsync(
switch (stateType)
{
case StateType.Syntax:
await GetSyntaxDiagnosticsAsync(analyzerDriver, stateSet.Analyzer).ConfigureAwait(false);
await GetSyntaxDiagnosticsAsync(analyzerDriver, _analyzer).ConfigureAwait(false);
break;
case StateType.Document:
await GetSemanticDiagnosticsAsync(analyzerDriver, stateSet.Analyzer).ConfigureAwait(false);
await GetSemanticDiagnosticsAsync(analyzerDriver, _analyzer).ConfigureAwait(false);
break;
case StateType.Project:
default:
Expand Down

0 comments on commit 0154c37

Please sign in to comment.