EqualityComparer does not respect value-equality for null

[Meg Gotshall]

I am using a third-party library that includes Null Objects with value comparisons that return true for null. However, the NUnit equality comparer uses reference equality (via the == operator with System.Object references) to check for null and returns false if only one input is the null pointer. This means that the value-equality comparison for the custom type is never reached.

The following is a quick & dirty implementation of a Null Object with a few tests that demonstrate the problem:

public class MyObject
{
    public static MyObject NullObject = new MyObject(-1);

    public int Value { get; set; }

    public MyObject(int value)
    {
        this.Value = value;
    }

    public static bool operator ==(MyObject left, MyObject right)
    {
        if (ReferenceEquals(left, NullObject) || ReferenceEquals(left, null))
            return (ReferenceEquals(right, NullObject) || ReferenceEquals(right, null));

        else if (ReferenceEquals(right, NullObject) || ReferenceEquals(right, null))
            return false;

        return Equals(left, right);
    }

    public static bool operator !=(MyObject left, MyObject right)
    {
        return !(left == right);
    }
    public override bool Equals(object obj)
    {
        if (ReferenceEquals(obj, NullObject) || ReferenceEquals(obj, null))
            return (ReferenceEquals(this, NullObject));

        else if (ReferenceEquals(this, NullObject))
            return false;

        MyObject myobj = obj as MyObject;
        if (myobj == null)
            return false;

        return this.Value == myobj.Value;
    }

    public override int GetHashCode()
    {
        return base.GetHashCode();
    }

}

[TestFixture]
public class TestNullObject
{
    // TEST PASSES
    [Test]
    public void TestIsTrue()
    {
        Assert.IsTrue(MyObject.NullObject == null);
        Assert.IsTrue(null == MyObject.NullObject);
    }

    // TESTS FAIL
    [Test]
    public void TestIsNull()
    {
        Assert.IsNull(MyObject.NullObject);
    }

    [Test]
    public void TestAreEqual1()
    {
        Assert.AreEqual(MyObject.NullObject, null);
    }

    [Test]
    public void TestAreEqual2()
    {
        Assert.AreEqual(null, MyObject.NullObject);
    }
}

[Simone Busoli (simone.busoli)]

Looks like a valid scenario in my opinion. It's not straightforward to get right as we're doing all sorts of equality in NUnit but I put together a spike which passes all tests plus the ones Meg posted here except the Is.Null one. I think that one should not be supported as Is.Null should check for reference equality, which you cannot override.

I would like to hear what Charlie thinks too.

lp:~simone.busoli/nunitv2/null-equality

[Charlie Poole (charlie.poole)]

The NUnit equality model has handled null reference the same way for 13
years, so I hesitate to change it in any way that will break existing
applications.

I agree with Simone that IsNull should not pass with anything except the
null reference.

If there is a way we can make the two AreEqual tests pass without breaking
other tests, then I might consider that. However, I'm dubious of the value
of such an addition since use of AreEqual is not the way to test that the
NullObject in question works. The right way is to call the object's Equals
overload directly and find out what it returns.

Testing by using Assert.AreEqual verifies that the object's implementation
of equality matches NUnit's. I don't think that's what you want to test.

I'm inclined to reject this on the grounds of utility, but I'd like to hear
the other side first.

Charlie

On Thu, Mar 21, 2013 at 3:26 PM, Simone Busoli
<email address hidden>wrote:

> Looks like a valid scenario in my opinion. It's not straightforward to
> get right as we're doing all sorts of equality in NUnit but I put
> together a spike which passes all tests plus the ones Meg posted here
> except the Is.Null one. I think that one should not be supported as
> Is.Null should check for reference equality, which you cannot override.
>
> I would like to hear what Charlie thinks too.
>
> lp:~simone.busoli/nunitv2/null-equality
>
> --
> You received this bug notification because you are subscribed to NUnit
> Extended Testing Platform.
> https://bugs.launchpad.net/bugs/1157828
>
> Title:
> EqualityComparer does not respect value-equality for null
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/nunitv2/+bug/1157828/+subscriptions
>

[Simone Busoli (simone.busoli)]

On Fri, Mar 22, 2013 at 6:39 AM, Charlie Poole <email address hidden> wrote:

> The NUnit equality model has handled null reference the same way for 13
> years, so I hesitate to change it in any way that will break existing
> applications.
>

I don't think that the changes this would require would necessarily break
existing applications. Did you assume that because you know they would? As
I said, the changes I did in the branch I linked earlier don't break any
NUnit tests, although I did not spend much time trying to figure out other
side-effects.

> I agree with Simone that IsNull should not pass with anything except the
> null reference.
>
> If there is a way we can make the two AreEqual tests pass without breaking
> other tests, then I might consider that.

As I said, all NUnit tests except the one provided by Meg which uses the
NullConstraint pass.

> However, I'm dubious of the value
> of such an addition since use of AreEqual is not the way to test that the
> NullObject in question works. The right way is to call the object's Equals
> overload directly and find out what it returns.
>

I am also doubtful about the usefulness, I never myself had such a
requirement for a null object to compare to null, but I think it's
generally just an extension to the normal equality comparison. In our
EqualConstraint we are returning false as soon as either actual or expected
== null, but the operator can be overridden, so it would probably be more
correct to use ReferenceEquals(actual|expeced, null) in that case and try
to use object.Equals if !(actual|expected).ReferenceEquals(null) (although
(actual|expected) == null might be true).

> Testing by using Assert.AreEqual verifies that the object's implementation
> of equality matches NUnit's. I don't think that's what you want to test.
>

I think that NUnit should fallback to the implementation of object.Equals
that actual and expected eventually override, which it is already doing,
except that it is not doing it if either actual or expected == null.

> I'm inclined to reject this on the grounds of utility, but I'd like to hear
> the other side first.
>

Sure, let's see if Meg has any other input.

>
> Charlie
>
>
> On Thu, Mar 21, 2013 at 3:26 PM, Simone Busoli
> <email address hidden>wrote:
>
> > Looks like a valid scenario in my opinion. It's not straightforward to
> > get right as we're doing all sorts of equality in NUnit but I put
> > together a spike which passes all tests plus the ones Meg posted here
> > except the Is.Null one. I think that one should not be supported as
> > Is.Null should check for reference equality, which you cannot override.
> >
> > I would like to hear what Charlie thinks too.
> >
> > lp:~simone.busoli/nunitv2/null-equality
> >
> > --
> > You received this bug notification because you are subscribed to NUnit
> > Extended Testing Platform.
> > https://bugs.launchpad.net/bugs/1157828
> >
> > Title:
> > EqualityComparer does not respect value-equality for null
> >
> > To manage notifications about this bug go to:
> > https://bugs.launchpad.net/nunitv2/+bug/1157828/+subscriptions
> >
>
> --
> You received this bug notification because you are a member of NUnit
> De...

[Meg Gotshall (natraj)]

I think the biggest reason to fix it would be for consistency. Right now, if you override the equality operation, it will use the custom comparison EXCEPT for the case where one side is null. That means that you would essentially need to document the Assert.AreEqual method to say that it uses the custom comparison for types that override Object::Equals but performs a reference comparison when 'actual' or 'expected' is null. As an API developer, this sounds like a documentation nightmare. And since most programmers aren't inclined to read the documentation anyway, once they see that it uses their custom implementation once, they will assume that it always uses the custom implementation.

I'm not thrilled with the NullObject API I'm using, but its documentation recommends using 'resultObj != null' as the null check even in cases where the NullObject might be returned. Having written 'if (resultObje != null)' all over production code, I instinctively wrote 'Assert.IsNotNull(resultObj)' in my test code. Only to have them fail unexpectedly later on when trying to perform an unsupported operation on the NullObject. I can completely back the idea that IsNull and IsNotNull should be reference comparisons to the null pointer, but with the same Equality logic underneath, it is closely coupled to the AreEqual ambiguity.

Chances are that a caller isn't going to test every particular circumstance. If I have a type that overrides .Equals, I would run a quick test to see if something like AreEqual is using reference comparison or calling my custom comparison. After that first check I would assume (never a good idea, but not uncommon) that the method will always use the custom comparison.

And evidence indicates that at least one otherwise talented developer has fallen into this trap already! ;)

[Charlie Poole (charlie.poole)]

Hi Simone,

On Fri, Mar 22, 2013 at 2:59 AM, Simone Busoli
<email address hidden>wrote:

> On Fri, Mar 22, 2013 at 6:39 AM, Charlie Poole <email address hidden>
> wrote:
>
> > The NUnit equality model has handled null reference the same way for 13
> > years, so I hesitate to change it in any way that will break existing
> > applications.
> >
>
> I don't think that the changes this would require would necessarily break
> existing applications. Did you assume that because you know they would? As
> I said, the changes I did in the branch I linked earlier don't break any
> NUnit tests, although I did not spend much time trying to figure out other
> side-effects.
>

No, I don't assume they would. I was only trying to set a criterion for any
changes.

> > I agree with Simone that IsNull should not pass with anything except the
> > null reference.
> >
> > If there is a way we can make the two AreEqual tests pass without
> breaking
> > other tests, then I might consider that.
>
>
> As I said, all NUnit tests except the one provided by Meg which uses the
> NullConstraint pass.
>

How did you do this without bypassing the null test for other objects?

> > However, I'm dubious of the value
> > of such an addition since use of AreEqual is not the way to test that the
> > NullObject in question works. The right way is to call the object's
> Equals
> > overload directly and find out what it returns.
> >
>
> I am also doubtful about the usefulness, I never myself had such a
> requirement for a null object to compare to null, but I think it's
> generally just an extension to the normal equality comparison. In our
> EqualConstraint we are returning false as soon as either actual or expected
> == null, but the operator can be overridden, so it would probably be more
> correct to use ReferenceEquals(actual|expeced, null) in that case and try
> to use object.Equals if !(actual|expected).ReferenceEquals(null) (although
> (actual|expected) == null might be true).
>

Well, we use == for convenience, but the intent is to call ReferenceEquals.
The general rule is that any two null references are equal and a null
reference
can't be equal to a non-null reference.

> Testing by using Assert.AreEqual verifies that the object's implementation
> > of equality matches NUnit's. I don't think that's what you want to test.
> >
>
> I think that NUnit should fallback to the implementation of object.Equals
> that actual and expected eventually override, which it is already doing,
> except that it is not doing it if either actual or expected == null.
>

Well, of course you can't call object.Equals on a null reference, which is
why the check is necessary in the first place. Since we default to the
Equals implementation of the expected value, we could theoretically
only apply the null logic to expected, but applying it symetrically seems
to make the most sense.

>
> > I'm inclined to reject this on the grounds of utility, but I'd like to
> hear
> > the other side first.
> >
>
> Sure, let's see if Meg has any other input.
>
>
> >
> > Charlie
> >
> >
> > On Thu, Mar 21, 2013 at 3:26 PM, Simone Busoli
> > <email address hidden>wrote:
> >
> > > Looks like a valid scenario i...

[Simone Busoli (simone.busoli)]

Hi Charlie, I agree with you except when you mention that we use == for
convenience. I am wondering if rather than convenience it is instead an
unintentionally incorrect way to check for nullness. I am not sure if
switching from using equality operators in favor of ReferenceEquals would
break existing supported scenarios in addition to opening up the one
proposed by Meg, I'll try to look a bit more into it and see if I can
somehow come up with a solution which gives us some confidence that it will
not break existing code. As it stands now, allowing equality between
objects which look like they're null is a valid scenario and sounds like a
natural extension to allowing objects to override their Equals method,
unless it turns out to be technically unfeasible.

Simone

On Sat, Mar 23, 2013 at 6:25 AM, Charlie Poole <email address hidden> wrote:

> Hi Simone,
>
>
> On Fri, Mar 22, 2013 at 2:59 AM, Simone Busoli
> <email address hidden>wrote:
>
> > On Fri, Mar 22, 2013 at 6:39 AM, Charlie Poole <email address hidden>
> > wrote:
> >
> > > The NUnit equality model has handled null reference the same way for 13
> > > years, so I hesitate to change it in any way that will break existing
> > > applications.
> > >
> >
> > I don't think that the changes this would require would necessarily break
> > existing applications. Did you assume that because you know they would?
> As
> > I said, the changes I did in the branch I linked earlier don't break any
> > NUnit tests, although I did not spend much time trying to figure out
> other
> > side-effects.
> >
>
> No, I don't assume they would. I was only trying to set a criterion for any
> changes.
>
>
> > > I agree with Simone that IsNull should not pass with anything except
> the
> > > null reference.
> > >
> > > If there is a way we can make the two AreEqual tests pass without
> > breaking
> > > other tests, then I might consider that.
> >
> >
> > As I said, all NUnit tests except the one provided by Meg which uses the
> > NullConstraint pass.
> >
>
> How did you do this without bypassing the null test for other objects?
>
>
> > > However, I'm dubious of the value
> > > of such an addition since use of AreEqual is not the way to test that
> the
> > > NullObject in question works. The right way is to call the object's
> > Equals
> > > overload directly and find out what it returns.
> > >
> >
> > I am also doubtful about the usefulness, I never myself had such a
> > requirement for a null object to compare to null, but I think it's
> > generally just an extension to the normal equality comparison. In our
> > EqualConstraint we are returning false as soon as either actual or
> expected
> > == null, but the operator can be overridden, so it would probably be more
> > correct to use ReferenceEquals(actual|expeced, null) in that case and try
> > to use object.Equals if !(actual|expected).ReferenceEquals(null)
> (although
> > (actual|expected) == null might be true).
> >
>
> Well, we use == for convenience, but the intent is to call ReferenceEquals.
> The general rule is that any two null references are equal and a null
> reference
> can't be equal to a non-null reference.
>
> > Testing by using Assert.AreEqual ver...

[Charlie Poole (charlie.poole)]

Are you thinking that the existing code may actually be using any operator
override? I would not have thought so, since the arguments are objects at
the point where we make the comparison. But it could be and testing is the
way to find out. In the process, I think we should replace any code where
we actually want to use reference equality with reference equals to avoid
the possible confusion.

On Sat, Mar 23, 2013 at 5:15 PM, Simone Busoli
<email address hidden>wrote:

> Hi Charlie, I agree with you except when you mention that we use == for
> convenience. I am wondering if rather than convenience it is instead an
> unintentionally incorrect way to check for nullness. I am not sure if
> switching from using equality operators in favor of ReferenceEquals would
> break existing supported scenarios in addition to opening up the one
> proposed by Meg, I'll try to look a bit more into it and see if I can
> somehow come up with a solution which gives us some confidence that it will
> not break existing code. As it stands now, allowing equality between
> objects which look like they're null is a valid scenario and sounds like a
> natural extension to allowing objects to override their Equals method,
> unless it turns out to be technically unfeasible.
>
> Simone
>
>
> On Sat, Mar 23, 2013 at 6:25 AM, Charlie Poole <email address hidden> wrote:
>
> > Hi Simone,
> >
> >
> > On Fri, Mar 22, 2013 at 2:59 AM, Simone Busoli
> > <email address hidden>wrote:
> >
> > > On Fri, Mar 22, 2013 at 6:39 AM, Charlie Poole <email address hidden>
> > > wrote:
> > >
> > > > The NUnit equality model has handled null reference the same way for
> 13
> > > > years, so I hesitate to change it in any way that will break existing
> > > > applications.
> > > >
> > >
> > > I don't think that the changes this would require would necessarily
> break
> > > existing applications. Did you assume that because you know they would?
> > As
> > > I said, the changes I did in the branch I linked earlier don't break
> any
> > > NUnit tests, although I did not spend much time trying to figure out
> > other
> > > side-effects.
> > >
> >
> > No, I don't assume they would. I was only trying to set a criterion for
> any
> > changes.
> >
> >
> > > > I agree with Simone that IsNull should not pass with anything except
> > the
> > > > null reference.
> > > >
> > > > If there is a way we can make the two AreEqual tests pass without
> > > breaking
> > > > other tests, then I might consider that.
> > >
> > >
> > > As I said, all NUnit tests except the one provided by Meg which uses
> the
> > > NullConstraint pass.
> > >
> >
> > How did you do this without bypassing the null test for other objects?
> >
> >
> > > > However, I'm dubious of the value
> > > > of such an addition since use of AreEqual is not the way to test that
> > the
> > > > NullObject in question works. The right way is to call the object's
> > > Equals
> > > > overload directly and find out what it returns.
> > > >
> > >
> > > I am also doubtful about the usefulness, I never myself had such a
> > > requirement for a null object to compare to null, but I think it's
> > > generally just an extension to the normal equality compar...

[Meg Gotshall (natraj)]

From what I recall, the logic for the null checks at the beginning of the method is something like:

if (a == null && b == null)
  return true;

if (a == null || b == null)
  return false;

Could that be safely replaced with:

if (a == null && b == null) // or ReferenceEquals if you switch for clarity
  return true;

if (a == null)
  return b.Equals(null);
else if (b == null)
  return a.Equals(null);

?

I don't have the code in front of me, but off the top of my head, I can't think of any cases where you would still need to do any of the type-specific comparisons that come before the call to a.Equals(b). It seems like a relatively safe way to fix the issue without affecting any existing calls.

[Charlie Poole (charlie.poole)]

Nice! Passing the responsibility to the non-null object seems like a good
strategy.

It would need to be extended to cover cases where the non-null object
implements IEquatable as well. And we should look at every special case in
NUnitEqualityComparer and make sure that they will handle null correctly.
Most likely, this will lead to some refactoring too.

Charlie

On Tue, Mar 26, 2013 at 5:09 AM, Meg Gotshall <email address hidden> wrote:

> >From what I recall, the logic for the null checks at the beginning of
> the method is something like:
>
> if (a == null && b == null)
> return true;
>
> if (a == null || b == null)
> return false;
>
>
> Could that be safely replaced with:
>
> if (a == null && b == null) // or ReferenceEquals if you switch for
> clarity
> return true;
>
> if (a == null)
> return b.Equals(null);
> else if (b == null)
> return a.Equals(null);
>
> ?
>
> I don't have the code in front of me, but off the top of my head, I
> can't think of any cases where you would still need to do any of the
> type-specific comparisons that come before the call to a.Equals(b). It
> seems like a relatively safe way to fix the issue without affecting any
> existing calls.
>
> --
> You received this bug notification because you are subscribed to NUnit
> Extended Testing Platform.
> https://bugs.launchpad.net/bugs/1157828
>
> Title:
> EqualityComparer does not respect value-equality for null
>
> To manage notifications about this bug go to:
> https://bugs.launchpad.net/nunitv2/+bug/1157828/+subscriptions
>