CODEWITHSUNDEEP
c#.netoperator-overloadingstack-overflow
Ropstah
Ropstah

Reputation: 17794

Why is a stackoverflowexception thrown in this operator definition?

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;
    }
}

Upvotes: 8

Views: 2394

Answers (2)

Jean
Jean

Reputation: 5101

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;
    }
}

Upvotes: 0

Jon Skeet
Jon Skeet

Reputation: 1500245

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.

Upvotes: 27

Related Questions