Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Check reference equality for strongly-typed equals in records #51149

Merged
merged 8 commits into from
Feb 19, 2021
Merged

Check reference equality for strongly-typed equals in records #51149

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
abbda7e
Check reference equality for strongly-typed equals in records
Youssef1313 Feb 11, 2021
177fa99
Fix record tests for reference equality change
Youssef1313 Feb 11, 2021
bc934c5
Improve codegen for C# anonymous types
Youssef1313 Feb 12, 2021
64d341f
Enhance VB anonymous types codegen
Youssef1313 Feb 12, 2021
da031eb
Fix test
Youssef1313 Feb 12, 2021
731ab65
Update comments
Youssef1313 Feb 18, 2021
a5836bd
Update AnonymousTypeMethodBodySynthesizer.cs
Youssef1313 Feb 18, 2021
5970cc4
Update SynthesizedRecordEquals.cs
Youssef1313 Feb 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/Compilers/CSharp/Portable/Compiler/AnonymousTypeMethodBodySynthesizer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,10 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
//
// {
// $anonymous$ local = value as $anonymous$;
// return local != null
// return (object)local == this || (local != null
// && System.Collections.Generic.EqualityComparer<T_1>.Default.Equals(this.backingFld_1, local.backingFld_1)
// ...
// && System.Collections.Generic.EqualityComparer<T_N>.Default.Equals(this.backingFld_N, local.backingFld_N);
// && System.Collections.Generic.EqualityComparer<T_N>.Default.Equals(this.backingFld_N, local.backingFld_N));
// }

// Type and type expression
Expand Down Expand Up @@ -137,6 +137,9 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
fields.Free();
}

// Compare references
retExpression = F.LogicalOr(F.ObjectEqual(F.This(), boundLocal), retExpression);

// Final return statement
BoundStatement retStatement = F.Return(retExpression);

Expand Down
6 changes: 3 additions & 3 deletions src/Compilers/CSharp/Portable/Symbols/Synthesized/Records/SynthesizedRecordEquals.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,8 +115,8 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
// delegate to:
//
// virtual bool Equals(Derived other) =>
// base.Equals((Base)other) &&
// field1 == other.field1 && ... && fieldN == other.fieldN;
// (object)other == this || (base.Equals((Base)other) &&
// field1 == other.field1 && ... && fieldN == other.fieldN);
retExpr = F.Call(
F.Base(baseEquals.ContainingType),
baseEquals,
Expand Down Expand Up @@ -155,7 +155,7 @@ internal override void GenerateMethodBody(TypeCompilationState compilationState,
}

fields.Free();

retExpr = F.LogicalOr(F.ObjectEqual(F.This(), other), retExpr);
Copy link
Contributor

@AlekseyTs AlekseyTs Feb 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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();
        }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dotnet/roslyn-compiler Thoughts?

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@AlekseyTs AlekseyTs Feb 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@Youssef1313 Youssef1313 Feb 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

F.CloseMethod(F.Block(F.Return(retExpr)));
}
catch (SyntheticBoundNodeFactory.MissingPredefinedMember ex)
Expand Down
Loading