Fix ordering issue in analyzer callbacks for nested actions in symbol start analyzers

[mavasani]

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 all 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.

[mavasani]

@dotnet/roslyn-compiler for reviews.

[mavasani]

@dotnet/roslyn-compiler Ping for reviews.

[AlekseyTs]

@mavasani Unfortunately, I am not familiar with the code being modified. Could you explain the following?

• what effect the change has
• why it is appropriate to do what the modified code does
• how does the change address the problem

[mavasani]

@AlekseyTs definitely, let me try to summarize.

SymbolStart/End pair of actions allow analyzers to register a callback at a start of a symbol declaration (most often a named type symbol), then register nested callbacks within the scope of this symbol's declaration nodes for any syntax nodes or operations within these declaration nodes to accumulate analysis state and finally register a symbol end action callback that gets called after all the callbacks have been made for everything defined within this outer symbol, so it can use the accumulated state to report diagnostics.

These analyzers rely on the ordering guarantee that symbol end callback is made after everything defined inside the outer symbol has received all callbacks. To ensure this ordering, whenever non-zero SymbolEndActionsCount are registered within a symbol start callback, we compute the inner dependent symbols for the outer symbol for which symbol start callback was made. This core logic is defined in the below code:

roslyn/src/Compilers/Core/Portable/DiagnosticAnalyzer/AnalyzerManager.AnalyzerExecutionContext.cs

Lines 138 to 221 in 06534d5

	public Task<HostSymbolStartAnalysisScope> GetSymbolAnalysisScopeAsync(
	ISymbol symbol,
	bool isGeneratedCodeSymbol,
	SyntaxTree? filterTree,
	TextSpan? filterSpan,
	ImmutableArray<SymbolStartAnalyzerAction> symbolStartActions,
	AnalyzerExecutor analyzerExecutor,
	CancellationToken cancellationToken)
	{
	lock (_gate)
	{
	_lazySymbolScopeTasks ??= new Dictionary<ISymbol, Task<HostSymbolStartAnalysisScope>>();
	if (!_lazySymbolScopeTasks.TryGetValue(symbol, out var symbolScopeTask))
	{
	symbolScopeTask = Task.Run(() => getSymbolAnalysisScopeCore(), cancellationToken);
	_lazySymbolScopeTasks.Add(symbol, symbolScopeTask);
	}
	return symbolScopeTask;
	HostSymbolStartAnalysisScope getSymbolAnalysisScopeCore()
	{
	var symbolAnalysisScope = new HostSymbolStartAnalysisScope();
	analyzerExecutor.ExecuteSymbolStartActions(symbol, _analyzer, symbolStartActions, symbolAnalysisScope, isGeneratedCodeSymbol, filterTree, filterSpan, cancellationToken);
	var symbolEndActions = symbolAnalysisScope.GetAnalyzerActions(_analyzer);
	if (symbolEndActions.SymbolEndActionsCount > 0)
	{
	var dependentSymbols = getDependentSymbols();
	lock (_gate)
	{
	_lazyPendingMemberSymbolsMap ??= new Dictionary<ISymbol, HashSet<ISymbol>?>();
	// Guard against entry added from another thread.
	VerifyNewEntryForPendingMemberSymbolsMap(symbol, dependentSymbols);
	_lazyPendingMemberSymbolsMap[symbol] = dependentSymbols;
	}
	}
	return symbolAnalysisScope;
	}
	}
	HashSet<ISymbol>? getDependentSymbols()
	{
	HashSet<ISymbol>? memberSet = null;
	switch (symbol.Kind)
	{
	case SymbolKind.NamedType:
	processMembers(((INamedTypeSymbol)symbol).GetMembers());
	break;
	case SymbolKind.Namespace:
	processMembers(((INamespaceSymbol)symbol).GetMembers());
	break;
	}
	return memberSet;
	void processMembers(IEnumerable<ISymbol> members)
	{
	foreach (var member in members)
	{
	if (!member.IsImplicitlyDeclared && member.IsInSource())
	{
	memberSet ??= new HashSet<ISymbol>();
	memberSet.Add(member);
	// Ensure that we include symbols for both parts of partial methods.
	if (member is IMethodSymbol method &&
	!(method.PartialImplementationPart is null))
	{
	memberSet.Add(method.PartialImplementationPart);
	}
	}
	if (member is INamedTypeSymbol typeMember)
	{
	processMembers(typeMember.GetMembers());
	}
	}
	}
	}
	}

This logic was only adding the most immediate containing member symbols as dependent symbols, but not member symbols of nested types. This is sufficient for most real world analysis cases (and hence wasn't identified until now), but the analyzer that Cyrus recently added relied on gathering analysis state information even for nodes/operations within the member symbols of nested types to report the correct diagnostic in the symbol end callback for the outer type.

This change updates this dependent symbol computation logic to even consider members of nested types as dependent symbols. Additionally, the change in AnalyzerDriver.cs in this PR, which is executed after a symbol has been completely analyzed, now walks up the ContainingType chain to see if this was the last dependent symbol to be analyzed for this named type, and if so the corresponding named type's symbol end action gets called.

[AlekseyTs]

@mavasani Thanks for the explanation. Is it possible to add a test?

[AlekseyTs]

LGTM (commit 3). From offline discussion, there are tests that fail in the IDE for the IDE analyzer changed in the PR if only the IDE changes are made. However, consider adding a targeted test in the analyzer layer.

[AlekseyTs]

@dotnet/roslyn-compiler For the second review

[mavasani]

LGTM (commit 3). From offline discussion, there are tests that fail in the IDE for the IDE analyzer changed in the PR if only the IDE changes are made. However, consider adding a targeted test in the analyzer layer.

I have tried adding an analyzer unit test with main...mavasani:roslyn:TestForCyrus, but am unable to get it to fail, with or without the compiler layer product changes. However, I have verified that the IDE unit tests for CSharpUsePrimaryConstructorDiagnosticAnalyzer.cs fail more than half the times if the compiler layer product changes in this PR are reverted.

[mavasani]

@dotnet/roslyn-compiler for second review

[mavasani]

Thank you @cston