From d352b444918a87f6ef3959f4e665fd1dcb6bab55 Mon Sep 17 00:00:00 2001 From: Manish Vasani Date: Thu, 8 Jun 2023 19:59:24 +0530 Subject: [PATCH] Fixes #68484 Race condition is detailed in the description of #68484. Fix is to ensure that we also mark the members of nested types as dependent symbols so that the SymbolEnd actions for the outer type are executed only after callbacks for nested type members have been done. Testing: This bug was identified by the newly added CSharpUsePrimaryConstructorDiagnosticAnalyzer. There is an existing unit test that failed intermittently due to this race condition: https://github.com/CyrusNajmabadi/roslyn/blob/f3e4a27818ea2495606f12fcabd986238b66efe4/src/Analyzers/CSharp/Tests/UsePrimaryConstructor/UsePrimaryConstructorTests.cs#LL741C27-L741C62 A workaround was added to the analyzer to register a dummy SymbolEnd action for nested types to work around this bug. This PR reverts that workaround and I verified that the test passes successfully for hundreds of iterations. I also attempted to create a standalone compiler unit test, but was unable to get this race condition to repro for the test even without the product change. --- .../CSharpUsePrimaryConstructorDiagnosticAnalyzer.cs | 9 --------- .../Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs | 8 +++++++- .../AnalyzerManager.AnalyzerExecutionContext.cs | 3 +-- 3 files changed, 8 insertions(+), 12 deletions(-) diff --git a/src/Analyzers/CSharp/Analyzers/UsePrimaryConstructor/CSharpUsePrimaryConstructorDiagnosticAnalyzer.cs b/src/Analyzers/CSharp/Analyzers/UsePrimaryConstructor/CSharpUsePrimaryConstructorDiagnosticAnalyzer.cs index 978a33d6b950a..ee5f7cb0067bd 100644 --- a/src/Analyzers/CSharp/Analyzers/UsePrimaryConstructor/CSharpUsePrimaryConstructorDiagnosticAnalyzer.cs +++ b/src/Analyzers/CSharp/Analyzers/UsePrimaryConstructor/CSharpUsePrimaryConstructorDiagnosticAnalyzer.cs @@ -217,12 +217,10 @@ public static void AnalyzeNamedTypeStart( // Ensure that any analyzers for containing types are created and they hear about any reference to their // fields in this nested type. - var hadContainingTypeAnalyzer = false; for (var containingType = startSymbol.ContainingType; containingType != null; containingType = containingType.ContainingType) { var containgTypeAnalyzer = TryGetOrCreateAnalyzer(containingType); RegisterFieldOrPropertyAnalysisIfNecessary(containgTypeAnalyzer); - hadContainingTypeAnalyzer = hadContainingTypeAnalyzer || containgTypeAnalyzer != null; } // Now try to make the analyzer for this type. @@ -232,13 +230,6 @@ public static void AnalyzeNamedTypeStart( RegisterFieldOrPropertyAnalysisIfNecessary(analyzer); context.RegisterSymbolEndAction(analyzer.OnSymbolEnd); } - else if (hadContainingTypeAnalyzer) - { - // We didn't create an analyze for this type. But we have containing types with analyzers. Ensure - // we register a symbol-end analyzer for us. The analysis subsystem needs this to ensure that outer - // analyzers don't complete until its nested types are done. - context.RegisterSymbolEndAction(static _ => { }); - } return; diff --git a/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs b/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs index 3e70f952c9a66..e744e4ddd4dc0 100644 --- a/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs +++ b/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerDriver.cs @@ -1620,7 +1620,13 @@ async Task onSymbolAndMembersProcessedAsync(ISymbol symbol, DiagnosticAnalyzer a } await processContainerOnMemberCompletedAsync(symbol.ContainingNamespace, symbol, analyzer).ConfigureAwait(false); - await processContainerOnMemberCompletedAsync(symbol.ContainingType, symbol, analyzer).ConfigureAwait(false); + + var containingType = symbol.ContainingType; + while (containingType != null) + { + await processContainerOnMemberCompletedAsync(containingType, symbol, analyzer).ConfigureAwait(false); + containingType = containingType.ContainingType; + } } async Task processContainerOnMemberCompletedAsync(INamespaceOrTypeSymbol containerSymbol, ISymbol processedMemberSymbol, DiagnosticAnalyzer analyzer) diff --git a/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.AnalyzerExecutionContext.cs b/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.AnalyzerExecutionContext.cs index e589c463aca4c..0c7d6a92dd5e1 100644 --- a/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.AnalyzerExecutionContext.cs +++ b/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.AnalyzerExecutionContext.cs @@ -208,8 +208,7 @@ void processMembers(IEnumerable members) } } - if (member.Kind != symbol.Kind && - member is INamedTypeSymbol typeMember) + if (member is INamedTypeSymbol typeMember) { processMembers(typeMember.GetMembers()); }