Why would I ever want to do object.ReferenceEquals(null, this) in Equals override?

Question

Consider the following code that I was reviewing:

public override bool Equals(object other)
{
    return !object.ReferenceEquals(null, this)
           && (object.ReferenceEquals(this, other)
           || ((other is MyType) && this.InternalEquals((MyType)other)));
}

The first line in this code triggered my curiosity. Whenever this is null, the method should return false. Now I am pretty sure the programmer meant to write !object.ReferenceEquals(other, null), to shortcut situations with null, but he's insistent that this can be null. I'm insistent that it cannot (unless someone uses direct memory manipulation). Should we leave it in?

At the same time, the order of the parameters to `ReferenceEquals` doesn't matter. Your proposed change is no different. — Servy, Jan 30 '13 at 21:05
@AndrewBarber: Yes it can. You have to work a bit, but it's *possible* to call an instance method where `this` is null. — Jon Skeet, Jan 30 '13 at 21:06
@Andrew: Well, the argument is that during finalization, the object can become garbage collected, but some methods may still get executed, esp. in `using` blocks. — Abel, Jan 30 '13 at 21:06
@Servy: a typo in my post, there was another line at first. Now it makes more sense, I think. — Abel, Jan 30 '13 at 21:07
@Abel: Oh it's got nothing to do with finalization. An object *can* be finalized and even GC'd while a method is running, but not if you're going to use `this` anywhere. — Jon Skeet, Jan 30 '13 at 21:08
@Abel In such cases you can generally assert that unexpected behavior is appropriate. It's unreasonable for anyone to thing that a method on an already GCed object would work properly. — Servy, Jan 30 '13 at 21:08
@JonSkeet: are you saying that I should program defensively and test for `this` being null? That can become rather awkward in about every object invocation, can it not? — Abel, Jan 30 '13 at 21:09
@Abel No, I think he's just providing an interesting tidbit of knowledge. What needs to be done to make `this` `null` is so convoluted and in any case I've seen, clearly intentional, such that you can effectively ignore such cases and blame the caller for breaking their own method. So while `this` can be `null`, it is appropriate (in *almost* all cases) to pretend it can't be. — Servy, Jan 30 '13 at 21:10
Related: http://stackoverflow.com/questions/10625326/this-null-inside-net-instance-method-why-is-that-possible — Brian Rasmussen, Jan 30 '13 at 21:11
`!object.ReferenceEquals(null, this)` is equivalent to `true`. `true && anything` is equivalent to `anything`. It's safe to remove this condition. — JosephHirn, Jan 30 '13 at 21:13
@Ginosaji As the OP correctly analized, `other` needs to be checked for `null`, so the appropriate fix is to change `this` to `other` in that snippet. — Servy, Jan 30 '13 at 21:16
@Ginosaji: the discussion is (see other comments, esp. the one by Jon Skeet whether your statement is really true. It appears not to be so always ;) — Abel, Jan 30 '13 at 21:20
@Abel: What reason did the original programmer give for putting it in? How did they think that `this` could become `null`? — TarkaDaal, Jan 31 '13 at 09:37
@TarkaDaal: that during finalizing/disposing, like in the destructor, it _could_ be null. Maybe this is true, I'm not certain. — Abel, Jan 31 '13 at 16:18

Jon Skeet · Accepted Answer · 2013-01-30T21:23:40.843

While I certainly wouldn't normally check this for nullity, it's possible, without any actual memory nastiness - just a bit of reflection:

using System;

public class Test
{
    public void CheckThisForNullity()
    {
        Console.WriteLine("Is this null? {0}", this == null);
    }

    static void Main(string[] args)
    {
        var method = typeof(Test).GetMethod("CheckThisForNullity");
        var openDelegate = (Action<Test>) Delegate.CreateDelegate(
               typeof(Action<Test>), method);
        openDelegate(null);
    }
}

Alternatively, generate IL which uses call instead of callvirt to call an instance method on a null target. Entirely legit, just not something the C# compiler would normally do.

This has nothing to do with finalization, which is hairy in its own right but in different ways. It's possible for a finalizer to run while an instance method is executing if the CLR can prove that you're not going to use any fields in the instance (which I would strongly expect to include the this reference).

As for the code presented - nope, that looks like it's just a mistake. I would rewrite it as:

public override bool Equals(object other)
{
    return Equals(other as MyType);
}

public bool Equals(MyType other)
{
    if (ReferenceEquals(other, null))
    {
        return false;
    }
    // Now perform the equality check
}

... assuming that MyType is a class, not a struct. Note how I'm using another public method with the right parameter type - I'd implement IEquatable<MyType> at the same time.

figured I'd point out that the actual question asked was `Should we leave it in?`, and you haven't really addressed that. — Servy, Jan 30 '13 at 21:12
@Servy, I think Jon says that in his opening statement: he wouldn't do so. Relatively simple (nasty?) trick though, should've come up with that myself. If I'd have to guard against that, I get the same kind of code in every instance method as in every extension method: checking for nullity. — Abel, Jan 30 '13 at 21:15
Nice to add the typical equality pattern as well, great update! — Abel, Jan 30 '13 at 21:18
One comment, correct me if I'm wrong: I don't think the GC analyzes the code to prove that a reference isn't used anymore (in a running method). I believe that's something the *JIT* does (to help the GC's run, of course). — Theodoros Chatzigiannakis, Jan 30 '13 at 21:19
@TheodorosChatzigiannakis: Have edited to "CLR" to be on the safe side :) — Jon Skeet, Jan 30 '13 at 21:23

score 1 · Answer 2 · answered Jan 30 '13 at 21:14

C# doesn't normally allow methods to be called on null. I think that the programmer who wrote that is either coming from a C++ background (where I think methods can be called on null, as long as they don't access a data member of this) or writing defensively for special scenarios (such as invocation by reflection, as has already been said).

Why would I ever want to do object.ReferenceEquals(null, this) in Equals override?

2 Answers2