`SimpleDiagnostic` class uses reference equality when comparing message arguments

[eiriktsarpalis]

Encountered the following issue when trying to debug an incremental source generator false positive:

Minimal reproduction

[Fact]
public void ReturnsEqualValuesForEqualArguments()
{
    var descriptor = new DiagnosticDescriptor(
        id: "0001",
        title: "title",
        messageFormat: "Prameterized format {0}.",
        category: "category",
        defaultSeverity: DiagnosticSeverity.Warning,
        isEnabledByDefault: true);

    string arg1 = new string('a', 5);
    string arg2 = new string('a', 5);
    Assert.Equal(arg1, arg2);
    Assert.NotSame(arg1, arg2);

    Diagnostic diag1 = Diagnostic.Create(descriptor, location: null, arg1);
    Diagnostic diag2 = Diagnostic.Create(descriptor, location: null, arg2);

    Assert.Equal(diag1, diag2); // Assertion failing
}

The issue appears to be caused by the following line:

roslyn/src/Compilers/Core/Portable/Diagnostic/Diagnostic_SimpleDiagnostic.cs

Line 165 in 687921f

&& _messageArgs.SequenceEqual(other._messageArgs, (a, b) => a == b)

[CyrusNajmabadi]

Diagnostics do not have value-semantics (and arbritrary parts of our stack produce different instances of them which themselves are not comparable). You should store the data you collect to create the diagnostic in some value-record and store that in your model.

[agocke]

@CyrusNajmabadi This was not true in the compiler, as far as I know, which is why the abstract Diagnostic class implements IEquatable. If this isn't the case, what are the expected semantics for Diagnostic.Equals?

[agocke]

In particular, the observed behavior is caused by this line:

roslyn/src/Compilers/Core/Portable/Diagnostic/Diagnostic_SimpleDiagnostic.cs

Line 165 in 59727ee

&& _messageArgs.SequenceEqual(other._messageArgs, (a, b) => a == b)

I suspect that the use of == there, which always produces a reference-equality comparison of the message arguments, is a bug. The right thing to do was probably a.Equals(b), which would have done a virtual call.

This interpretation as a bug also lines up with the hash code implementation, which does a virtual call to GetHashCode. If only reference equality mattered, a hash code implementation specific to the reference (RuntimeHelpers.GetHashCode) could be used instead, but it wasn't.

[agocke]

OK, went back and looked at the pre-github source code. I'm confident this is a bug.

At first, we had arguments in SimpleDiagnostic. Then, we briefly decide to move them elsewhere in the hierarchy. Later, we decided to bring them back, in much the same form they are in right now. However, the contact added (a, b) => a == b where it wasn't there before, despite the semantics being equivalent to the previous design. I think this was a typo, and the behavior should have matched the original version, which effectively called object.Equals for each argument.