Check reference equality for strongly-typed equals in records

[Youssef1313]

Fixes #51136

Not sure if it's that simple.

[AlekseyTs]

F.ObjectEqual(F.This(), other) [](start = 38, length = 30)

Is this check duplicated in the equality operator now? Should it be removed from there? #Closed

[Youssef1313]

@AlekseyTs The issue is that users can re-define Equals.

[AlekseyTs]

The issue is that users can re-define Equals.

And? The redefined Equals should still do the right thing.

---

In reply to: 574776221 [](ancestors = 574776221)

[Youssef1313]

@AlekseyTs Just tried to remove the check from == implementation, and now null comparison fails unfortunately 😕

The current implementation looks like:

        if ((object)r1 != r2) // if this is removed, (MyRecord)null == (MyRecord)null be false.
        {
            if ((object)r1 != null)
            {
                return r1.Equals(r2);
            }
            return false;
        }
        return true;

This was caught by the following test:

        [Theory]
        [CombinatorialData]
        public void EqualityOperators_09(bool useImageReference)
        {
            var source1 =
@"
public record A(int X) 
{
}
";
            var comp1 = CreateCompilation(source1);

            var source2 =
@"
class Program
{
    static void Main()
    {
        Test(null, null);
        Test(null, new A(0));
        Test(new A(1), new A(1));
        Test(new A(2), new A(3));
    }

    static void Test(A a1, A a2)
    {
        System.Console.WriteLine(""{0} {1} {2} {3}"", a1 == a2, a2 == a1, a1 != a2, a2 != a1);
    }
}
";
            CompileAndVerify(source2, references: new[] { useImageReference ? comp1.EmitToImageReference() : comp1.ToMetadataReference() }, expectedOutput:
@"
True True False False
False False True True
True True False False
False False True True
").VerifyDiagnostics();
        }

[AlekseyTs]

Just tried to remove the check from == implementation, and now null comparison fails unfortunately

I guess we could change the code gen to the following:

         if ((object)r1 != null)
            {
                return r1.Equals(r2);
            }

        return (object)r2 == null;

However, I am not sure if this is going to give us any noticeable improvement.

---

In reply to: 575128469 [](ancestors = 575128469)

[AlekseyTs]

@dotnet/roslyn-compiler Thoughts?

---

In reply to: 575311021 [](ancestors = 575311021,575128469)

[333fred]

Is this check duplicated in the equality operator now? Should it be removed from there?

In the email thread, I believe we agreed to duplicate the equality check.

[AlekseyTs]

F.LogicalOr [](start = 26, length = 11)

I think we should do similar change for the other consumer of the GenerateFieldEquals helper, the code that generates Equals for Anonymous Types. I also think that we should check/adjust code gen for VB Anonymous Types as well. #Closed

[Youssef1313]

@AlekseyTs That sounds reasonable. Should I start doing this and update tests? or do you want to wait for another team member to confirm?

[AlekseyTs]

I think we should do similar change for the other consumer of the GenerateFieldEquals helper, the code that generates Equals for Anonymous Types. I also think that we should check/adjust code gen for VB Anonymous Types as well.

That sounds reasonable. Should I start doing this and update tests? or do you want to wait for another team member to confirm?

I don't think we need to wait for the confirmation from other team members. If I was working on this change, I would just do this (the improvement of code gen for Anonymous Types).

---

In reply to: 575327671 [](ancestors = 575327671)

[Youssef1313]

@AlekseyTs I made the change for anonymous types for C# and updated the tests.

Could you point me to where the parallel change for VB should go?

I looked at https://github.com/Youssef1313/roslyn/blob/records-ref-equality/src/Compilers/VisualBasic/Portable/Symbols/AnonymousTypes/SynthesizedSymbols/AnonymousType_EqualsMethodSymbol.vb but didn't find anything relevant. So I'm probably in the wrong place.

Found it:

roslyn/src/Compilers/VisualBasic/Portable/Binding/SyntheticBoundTrees/AnonymousTypeSyntheticMethods.vb

Line 56 in afd1030

Friend Overrides Function GetBoundMethodBody(compilationState As TypeCompilationState, diagnostics As DiagnosticBag, Optional ByRef methodBodyBinder As Binder = Nothing) As BoundBlock

[AlekseyTs]

Could you point me to where the parallel change for VB should go?

See AnonymousType_IEquatable_EqualsMethodSymbol.GetBoundMethodBody in src\Compilers\VisualBasic\Portable\Binding\SyntheticBoundTrees\AnonymousTypeSyntheticMethods.vb.

---

In reply to: 575376850 [](ancestors = 575376850)

[AlekseyTs]

Done with review pass (commit 2) #Closed

[Youssef1313]

I'm having seemingly unrelated test failures locally (haven't yet checked if these happen on master or only this branch), but these doesn't happen in CI:

Is that a bug?

[AlekseyTs]

Is that a bug?

This could be an issue with the tests, but more investigation is necessary to confirm. It appears that a sorted set is ordered differently for you locally. If sorting is indeed affected by some settings in your local environment, the tests should be adjusted to make sure this isn't happening and that the sorting is environment independent.

---

In reply to: 778409221 [](ancestors = 778409221)

[AlekseyTs]

LGTM (commit 5)

[AlekseyTs]

@dotnet/roslyn-compiler Please review, need a second sign-off.

[jcouv]

            //  Method body:

it'd be great to update the snippet to reflect the new codegen.

---

Refers to: src/Compilers/CSharp/Portable/Compiler/AnonymousTypeMethodBodySynthesizer.cs:101 in da031eb. [](commit_id = da031eb, deletion_comment = False)

[jcouv]

                // There are no base record types.

Same comment here and below (line 112)

---

Refers to: src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEquals.cs:81 in da031eb. [](commit_id = da031eb, deletion_comment = False)

[jcouv]

Possibly here too

[jcouv]

LGTM Thanks (iteration 5) with ask to keep comments in sync with code

[jcouv]

Merged/squashed. Thanks @Youssef1313!