SourcePropertySymbol constructor sometimes sets the property twice

[jcouv]

In some error scenarios (bad overrides) we set the type of the property a second time.
The problem is that the first type matters (it causes side-effects) which are not undone when the type is set the second time.

See the constructor for SourcePropertySymbol:

                    // We do an extra check before copying the type to handle the case where the overriding
                    // property (incorrectly) has a different type than the overridden property.  In such cases,
                    // we want to retain the original (incorrect) type to avoid hiding the type given in source.
                    if (type.Type.Equals(overriddenPropertyType.Type, TypeCompareKind.IgnoreCustomModifiersAndArraySizesAndLowerBounds | TypeCompareKind.IgnoreNullableModifiersForReferenceTypes | TypeCompareKind.IgnoreDynamic))
                    {
                        type = type.WithTypeAndModifiers(
                            CustomModifierUtils.CopyTypeCustomModifiers(overriddenPropertyType.Type, type.Type, this.ContainingAssembly),
                            overriddenPropertyType.CustomModifiers);

                        // Although we only do this in error scenarios, it is undesirable to mutate the symbol by setting its type twice.
                        // Tracked by https://github.com/dotnet/roslyn/issues/35381
                        Interlocked.Exchange(ref _lazyType, null);
                        Interlocked.CompareExchange(ref _lazyType, new StrongBox<TypeWithAnnotations>(type), null);
                    }

[AlekseyTs]

The code looks safe to me. We are inside a constructor and the previously assigned value is used only for the purpose of finding overridden/implemented methods. Also, the only part of the type that we are adjusting are custom modifiers, those in an overriding/implementing property have no impact what so ever on language semantics or on the way overridden/implemented methods are found.

[gafter]

Could #35226 be a symptom of this issue?

[gafter]

@jcouv @agocke Because we are seeing various crashes that can most easily be explained by this issue (specifically, the temporary assignment of null to the type), I think this should be addressed sooner rather than later.

[agocke]

@gafter That issue looks like it might have been caused by the TypeSYmbolWithAnnotations race that we fixed, since it was reported back in April. I don't think we've seen a lot of crashes since then.

[gafter]

The stack track for both of these issues point to a null type coming from SourcePropertySymbol, which is what the offending code quoted above does.

[agocke]

But the TypeSymbolWithAnnotations problem was also in SourcePropertySymbol. What makes you think this particular piece of code is the problem, as opposed to the general race we fixed earlier? And why haven't we seen any crash reports since we fixed the TypeSymbolWithAnnotations race?