SentinelMethod.Equals should return reference equality

[benaadams]

Seen in #34375 (comment)

Unhandled Exception of Type System.NullReferenceException
Message :
System.NullReferenceException : Object reference not set to an instance of an object.

   at System.Reflection.TypeLoading.Sentinels.SentinelMethod.Equals(Object obj) in D:\j\workspace\windows-TGrou---33cbf18b\src\System.Reflection.MetadataLoadContext\src\System\Reflection\TypeLoading\General\Sentinels.cs:line 36
   at System.Reflection.TypeLoading.RoEvent.GetRoAddMethod() in D:\j\workspace\windows-TGrou---33cbf18b\src\System.Reflection.MetadataLoadContext\src\System\Reflection\TypeLoading\Events\RoEvent.cs:line 58
   at System.Reflection.TypeLoading.RoEvent.GetAddMethod(Boolean nonPublic) in D:\j\workspace\windows-TGrou---33cbf18b\src\System.Reflection.MetadataLoadContext\src\System\Reflection\TypeLoading\Events\RoEvent.cs:line 62
   at System.Reflection.Tests.EventTests.EventTest1() in D:\j\workspace\windows-TGrou---33cbf18b\src\System.Reflection.MetadataLoadContext\tests\src\Tests\Event\EventTests.cs:line 27

Blocking update of coreclr update in corefx

With "Support faster null checks" dotnet/coreclr#21765 the .Equals override gets called so it should test since its overriden (or not be overriden); rather than just throw null

/cc @steveharter @jkotas @stephentoub

[stephentoub]

Why are all of these overrides needed at all?

[jkotas]

Why are all of these overrides needed at all?

Ati liked to write code this way. It is trying to ensure that somebody is not calling methods on the sentinel by accident, etc.

[jkotas]

I am wondering whether this is an early red flag that the CoreCLR operator == changes are breaking. What are the chances that random custom reflection object models are going to have Equals broken in various ways?

I think the operator== changes should be fixed to keep calling left.Equals(right) like before. Yes, the code may be a bit bigger or slower but that is ok. (This change would not be necessary with this fix.)

[stephentoub]

It is trying to ensure that somebody is not calling methods on the sentinel by accident, etc.

If the instance is exposed, though, which it would seem to be given the break (?), that seems like something that should be fixed so that the methods do something reasonable if they are used. Throwing a null ref isn't particularly friendly. At the very least they should throw not supported or something like that, no?

[jkotas]

I think the operator== changes should be fixed to keep calling left.Equals(right) like before.

Or maybe abandon the idea of trying to "fix" operator == for null altogether, and go with the original plan to just use is null where the operator == gets used by accident.

[jkotas]

Throwing a null ref isn't particularly friendly. At the very least they should throw not supported or something like that, no?

It depends on style you prefer. My style would be to have these safety overrides in debug build only with asserts, and not compile them into release at all.

[benaadams]

Can still fix it for == null; but would need to double null check before calling left.Equals(right) which then makes it chunkier for the non == null due to the force inline

[benaadams]

Also isn't having a broken .Equals override method and expecting operator== to work an odd assumption?

[stephentoub]

My style would be to have these safety overrides in debug build only with asserts, and not compile them into release at all.

That sounds fine to me.

[benaadams]

My style would be to have these safety overrides in debug build only with asserts, and not compile them into release at all.

They are abstract method implementations which makes that trickier :-/

[benaadams]

Not sure what next steps are here?

[benaadams]

e.g. could change the checks to

public static bool operator ==(MethodInfo left, MethodInfo right)
{
    // Test "right" first to allow branch elimination when inlined for null checks (== null)
    // so it can become a simple test
    if (right is null)
    {
        return (left is null) ? true : false;
    }

    if (ReferenceEquals(left, right))
    {
        return true;
    }

    return (left is null) ? false : left.Equals(right);
}

[jkotas]

Yes, I think that would work. Also, I would change ReferenceEquals to (object)left == (object)right. It is more friendly to the inliner - now that the method is marked with aggresive inlining.

[benaadams]

dotnet/coreclr#21829

[benaadams]

Going to close this one