Field symbol from symbol tree and operation tree fail Equals check in presence of nullable enable

[mavasani]

Originally reported dotnet/roslyn-analyzers#2776, which seems to be a breaking change in analyzer behavior in presence of #nullable enable. I have created an isolated IOperation unit test:

        [CompilerTrait(CompilerFeature.IOperation, CompilerFeature.NullableReferenceTypes)]
        [Fact]
        public void NullableFieldReference()
        {
            var compWithoutNullable = CreateCompilation(@"
class C<T>
{
    private C<T> _field;
    public static void M(C<T> p)
    {
        _ = p._field;
    }
}");
            var compWithNullable = CreateCompilation(@"
#nullable enable
class C<T>
{
    private C<T>? _field;
    public static void M(C<T> p)
    {
        _ = p._field;
    }
}", options: WithNonNullTypesTrue());


            testCore(compWithoutNullable); // Passes
            testCore(compWithNullable); // Fails

            static void testCore(CSharpCompilation comp)
            {
                var syntaxTree = comp.SyntaxTrees[0];
                var model = comp.GetSemanticModel(syntaxTree);
                var root = syntaxTree.GetRoot();
                var classDecl = root.DescendantNodes().OfType<ClassDeclarationSyntax>().Single();
                var classSym = (INamedTypeSymbol)model.GetDeclaredSymbol(classDecl);
                var fieldSym = classSym.GetMembers("_field").Single();

                var methodDecl = root.DescendantNodes().OfType<MethodDeclarationSyntax>().Single();
                var methodBlockOperation = model.GetOperation(methodDecl);
                var fieldReferenceOperation = methodBlockOperation.Descendants().OfType<IFieldReferenceOperation>().Single();
                Assert.True(fieldSym.Equals(fieldReferenceOperation.Field));
            }
        }

[mavasani]

Assigned to @333fred based on our offline discussion.

[mavasani]

FYI: The above issue does not repro if C is a non-generic type.

[333fred]

This looks like a bug with our equality, we need to delegate to our internal comparison for more things than we do today.

[mavasani]

Isn't there a performance/allocation concern - we used the same symbol across both trees in non-nullable case, but have two different symbols (which are not reference equals) in the nullable case?

[333fred]

This isn't really an allocation we can get around.

[mavasani]

This isn't really an allocation we can get around.

Will this break any analyzers that performed reference equality checks between symbols from symbol and operation tree? I agree that analyzers should be fixed to use equality checks, but just wondering if we are fine with such a break.

[333fred]

Yes, we've previously said that we can't fix reference equality here. People should be using .Equals.