Why is a stackoverflowexception thrown in this operator definition?

Question

Please see my comment in the code below. How should I check for the parameter to be null? It looks like null is being casted to Foo which essentially makes a recursive call the the == operator. Why does this happen?

public class Foo
{
    public static bool operator ==(Foo f1, Foo f2)
    {
        if (f1 == null) //This throw a StackOverflowException
            return f2 == null;
        if (f2 == null)
            return f1 == null;
        else
            return f1.Equals((object)f2);
    }

    public static bool operator !=(Foo f1, Foo f2)
    {
        return !(f1 == f2);
    }

    public override bool Equals(object obj)
    {
        Foo f = obj as Foo;
        if (f == (Foo)null)
            return false;

        return false;
    }

    public override int GetHashCode()
    {
        return 0;
    }
}

You are *using* the == operator in the implementation of the == operator - and it's on the same type. What you need here is Object.ReferenceEquals to test for null. — harold, Mar 11 '12 at 15:04
What type do you expect `null` to be in that `==` operation if not `Foo`? — BoltClock, Mar 11 '12 at 15:07
related: http://stackoverflow.com/questions/73713/how-do-i-check-for-nulls-in-an-operator-overload-without-infinite-recursion (a thread on the workkaround) — nawfal, Apr 16 '13 at 08:34

Jon Skeet · Accepted Answer · 2012-03-11T15:26:38.797

Why does this happen?

Because the language rules say to.

You've provided an operator with this signature:

public static bool operator ==(Foo f1, Foo f2)

and then - wherever this happens to be in code - you've got this expression:

f1 == null

where f1 has a compile-time type of Foo. Now null is implicitly convertible to Foo as well, so why wouldn't it use your operator? And if you've got the first line of your operator unconditionally calling itself, you should expect a stack overflow...

In order for this not to happen, you'd need one of the two changes to the language:

The language would have to special-case what == meant when it's used within a declaration for ==. Ick.
The language would have to decide that any == expression with one operand being null always meant the reference comparison.

Neither is particularly nice, IMO. Avoiding it is simple though, avoids redundancy, and adds an optimization:

public static bool operator ==(Foo f1, Foo f2)
{
    if (object.ReferenceEquals(f1, f2))
    {
        return true;
    }
    if (object.ReferenceEquals(f1, null) ||
        object.ReferenceEquals(f2, null))
    {
        return false;
    }
    return f1.Equals(f2);
}

However, you then need to fix your Equals method, because that then ends up calling back to your ==, leading to another stack overflow. You've never actually ended up saying how you want equality to be determined...

I would normally have something like this:

// Where possible, define equality on sealed types.
// It gets messier otherwise...
public sealed class Foo : IEquatable<Foo>
{
    public static bool operator ==(Foo f1, Foo f2)
    {
        if (object.ReferenceEquals(f1, f2))
        {
            return true;
        }
        if (object.ReferenceEquals(f1, null) ||
            object.ReferenceEquals(f2, null))
        {
            return false;
        }

        // Perform actual equality check here
    }

    public override bool Equals(object other)
    {
        return this == (other as Foo);
    }

    public bool Equals(Foo other)
    {
        return this == other;
    }

    public static bool operator !=(Foo f1, Foo f2)
    {
        return !(f1 == f2);
    }

    public override int GetHashCode()
    {
        // Compute hash code here
    }
}

Note that this allows you to only bother with nullity checks in one place. In order to avoid redundantly comparing f1 for null when it's been called via an instance method of Equals to start with, you could delegate from == to Equals after checking for nullity of f1, but I'd probably stick to this instead.

@Jon Skeet, thank you for not simply writing a short response, and showing a nice full example, greatly appreciated. — David Venegoni, Apr 19 '13 at 05:32
Well-explained! A good way to get caught out with this mistake is by following [this MSDN example](https://learn.microsoft.com/en-us/visualstudio/code-quality/ca2224-override-equals-on-overloading-operator-equals#class-example), unfortunately, which contains the same misbehaviour... :( — Dan J, Dec 13 '17 at 22:44
What if `f1` and `f2` are both null? Your code would return `false`. Shouldn't you have checked `if( object.ReferenceEquals(f1,null) && !object.ReferenceEquals(f2,null) )` and vice-versa? — Drew Chapin, Apr 26 '18 at 20:30
@DrewChapin: No it won't - it will return true due to `if (object.ReferenceEquals(f1, f2)) { return true; }` — Jon Skeet, Apr 27 '18 at 05:34

score 0 · Answer 2 · answered Sep 12 '18 at 12:31

A bit old subject, but I reached this page, so it might help some people. To keep the exact same behavior, but without stack overflow, I would now rewrite it as follow:

public class Foo
{
    public static bool operator ==(Foo f1, Foo f2)
    {
        if (f1 is null) 
            return f2 is null;
        if (f2 is null)
            return false;
        else
            return f1.Equals((object)f2);
    }

    public static bool operator !=(Foo f1, Foo f2)
    {
        return !(f1 == f2);
    }

    public override bool Equals(object obj)
    {
        Foo f = obj as Foo;
        if(f is null) return false;
        return f == this;
    }

    public override int GetHashCode()
    {
        return 0;
    }
}

Why is a stackoverflowexception thrown in this operator definition?

2 Answers2