Definition and substituted symbols should be equal when nullability is ignored

[jcouv]

        [Fact]
        public void IgnoredNullability_MethodSymbol()
        {
            var src = @"
#nullable enable

public class C
{
    public void M<T>(out T x)
    {
        M<T>(out x);
#nullable disable
        M<T>(out x);
    }
}
";
            var comp = CreateCompilation(src);
            comp.VerifyDiagnostics();

            var tree = comp.SyntaxTrees.Single();
            var model = comp.GetSemanticModel(tree, ignoreAccessibility: false);
            var method1 = model.GetDeclaredSymbol(tree.GetRoot().DescendantNodes().OfType<MethodDeclarationSyntax>().Single());
            Assert.True(method1.IsDefinition);
            var invocations = tree.GetRoot().DescendantNodes().OfType<InvocationExpressionSyntax>().ToArray();
            Assert.Equal("M<T>(out x)", invocations[0].ToString());
            var method2 = model.GetSymbolInfo(invocations[0]).Symbol;
            Assert.False(method2.IsDefinition);
            Assert.Equal("M<T>(out x)", invocations[1].ToString());
            var method3 = model.GetSymbolInfo(invocations[1]).Symbol;
            Assert.True(method3.IsDefinition);

            // Note: definitions and subsituted symbols are not equal even when ignoring nullability
            Assert.False(method2.Equals(method3, SymbolEqualityComparer.Default));
            Assert.False(method2.Equals(method3, SymbolEqualityComparer.IncludeNullability));
            Assert.False(method3.Equals(method2, SymbolEqualityComparer.Default));
            Assert.False(method3.Equals(method2, SymbolEqualityComparer.IncludeNullability));

            Assert.False(method1.Equals(method2, SymbolEqualityComparer.Default));
            Assert.False(method1.Equals(method2, SymbolEqualityComparer.IncludeNullability));
            Assert.False(method2.Equals(method1, SymbolEqualityComparer.Default));
            Assert.False(method2.Equals(method1, SymbolEqualityComparer.IncludeNullability));

            Assert.True(method1.Equals(method3, SymbolEqualityComparer.Default));
            Assert.True(method1.Equals(method3, SymbolEqualityComparer.IncludeNullability));
            Assert.True(method3.Equals(method1, SymbolEqualityComparer.Default));
            Assert.True(method3.Equals(method1, SymbolEqualityComparer.IncludeNullability));
        }

[AlekseyTs]

If I remember correctly, when we discussed equality behavior in relation to nullable annotations back several months ago, we decided that, if symbols were equal before enabling nullable, they should remain equal after enabling nullable. Otherwise, IDE, analyzers and other clients suffer.

[jaredpar]

@AlekseyTs are you referring to equals by default (aka going through object.Equals) or when passing a specific set of comparison flags?

[AlekseyTs]

are you referring to equals by default (aka going through object.Equals) or when passing a specific set of comparison flags?

The equals by default, which is equivalent to using SymbolEqualityComparer.Default.

[jaredpar]

Gotcha. Yes agree that should be equivalent. The reasoning though actually goes back almost a year. We first hit this problem when we added support for nullability in the compiler with xUnit. Once the equality for symbols differed via object.Equals it broke a lot of the analysis. Hence we started moving to the model of "make object.Eqauls use C# 7 logic and force nullable aware code to opt in with nullable specific equality".

[jaredpar]

@jcouv can you help us understand what you think the issue is here that we're missing?

[jcouv]

@jaredpar Some implementations of Symbol.Equals(Symbol, TypeCompareKind) take a shortcut when one of the symbols is a definition (then we just check if the other is also a definition). But when the comparison ignores nullability it should be possible for a definition to be equal to a non-definition symbol, so that shortcut isn't valid.