Race condition in adding DefaultSyntaxValue diagnostics in SourceComplexParameterSymbol

[agocke]

Adding diagnostics after an Interlocked.CompareExchange is a race condition if the thread which succeeds in setting the default syntax value is paused before it adds the diagnostics and another thread binds without seeing the diagnostics.

[gafter]

This refers to the following code:

                if (_lazyDefaultSyntaxValue == ConstantValue.Unset)
                {
                    var diagnostics = DiagnosticBag.GetInstance();
                    if (Interlocked.CompareExchange(ref _lazyDefaultSyntaxValue, MakeDefaultExpression(diagnostics, ParameterBinder), ConstantValue.Unset) == ConstantValue.Unset)
                    {
                        AddDeclarationDiagnostics(diagnostics);
                    }

                    diagnostics.Free();
                }

[RikkiGibson]

Trying to understand what the failure mode is here. If you access the ExplicitDefaultConstantValue on a parameter and then try to get declaration diagnostics from the compilation, some diagnostics may be missing?

However it still seems like if you force complete the assembly, which happens when you try to get diagnostics via public API, the diagnostics will be present when ForceComplete returns.

It's just not obvious to me what expectation is violated here. Hypothetically can't symbols get away with deferring computing diagnostics until later on in ForceComplete? For example, we could delay deciding whether to include CS8017: The parameter has multiple distinct default values until ForceComplete is called.

[RikkiGibson]

Talked with @jaredpar a bit about this. It feels like whether this is a bug depends on what guarantees symbols make about whether diagnostics will be present in the compilation after reading specific members on symbols.

Tagging @AlekseyTs @cston as well to see if you have any thoughts on this.

[jaredpar]

The comment on that bug says the following:

// This is a race condition where the thread that loses
// the compare exchange tries to access the diagnostics
// before the thread which won the race finishes adding
// them. https://github.com/dotnet/roslyn/issues/17243

To me a clearer way of stating the problem is the following:

The only thread that is guaranteed to see the declaration diagnostics of a parameter constant value is the one that creates them.

By extension that means that we cannot be guaranteed that all declaration diagnostics are available until all of the threads being used to create declaration diagnostics have completed. I'm unsure if that is or is not the guarantee we wanted to provide.

Looking deeper at the code I do believe there is a bug here. Take a look at the content of SourceComplexParameterSymbol.ForceComplete

base.ForceComplete(locationOpt, cancellationToken);

// Force binding of default value.
var unused = this.ExplicitDefaultConstantValue;

Generally in ForceComplete the goal is that the method should not return until the results are available to all the threads that could be potentially calling ForceComplete. The current implementation of ExplicitConstantDefaultValue itself does not live up to that guarantee (see above analysis)

I think we need to fix this by changing the code to use a proper SymbolCompletionState object instead of the current CompareExchange on constants. Then we should be doing a proper SpinWait in ForceComplete as we do for similar uses of SymbolCompletionState