Skip to content

Commit

Permalink
Fixes dotnet#68484
Browse files Browse the repository at this point in the history
Race condition is detailed in the description of dotnet#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.
  • Loading branch information
mavasani committed Jun 8, 2023
1 parent 63d4074 commit d352b44
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand All @@ -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;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,8 +208,7 @@ void processMembers(IEnumerable<ISymbol> members)
}
}

if (member.Kind != symbol.Kind &&
member is INamedTypeSymbol typeMember)
if (member is INamedTypeSymbol typeMember)
{
processMembers(typeMember.GetMembers());
}
Expand Down

0 comments on commit d352b44

Please sign in to comment.