views:

300

answers:

4

Why does the C# compiler not even complain with a warning on this code? :

if (this == null)
{
   // ...
}

Obviously the condition will never be satisfied..

+22  A: 

Because you could override operator == to return true for that case.

public class Foo
{
    public void Test()
    {
        Console.WriteLine(this == null);
    }

    public static bool operator ==(Foo a, Foo b)
    {
        return true;
    }

    public static bool operator !=(Foo a, Foo b)
    {
        return true;
    }
}

Running new Foo().Test() will print "True" to the console.

The other question here is: why doesn't the compiler issue a warning for ReferenceEquals(this, null)? From the bottom of the above link:

A common error in overloads of operator == is to use (a == b), (a == null), or (b == null) to check for reference equality. This instead results in a call to the overloaded operator ==, causing an infinite loop. Use ReferenceEquals or cast the type to Object, to avoid the loop.

That might be answered by @Aaronaught's response. And that's also why you should be doing (object)x == null or ReferenceEquals(x, null), not doing a simple x == null, when you're checking for null references. Unless, of course, you're sure that the == operator is not overloaded.

Mark Rushakoff
This is yet another reason why I wish C# didn't confuse reference and value equality.
Jonathan Allen
@Jonathan: see the note at the bottom of that link: "A common error in overloads of `operator ==` is to use `(a == b)`, `(a == null)`, or `(b == null)` to check for reference equality. This instead results in a call to the overloaded operator ==, causing an infinite loop. **Use ReferenceEquals or cast the type to Object, to avoid the loop.**" (emphasis mine)
Mark Rushakoff
@Jonathan Allen: Since when does C# "confuse" reference and value equality? `System.Object` even has the `ReferenceEquals` and `Equals` methods for when you want to be explicit. The idea behind allowing the `==` operator to be overloaded is that you allow the object itself to decide which is appropriate. The only real nuisance is that it's possible for the overloaded `==` operator and `Equals` method to behave differently (and as a class designer you should avoid such things).
Aaronaught
Incidentally, there actually is a class (well, struct) that **already** overloads the `==` operator and specifically handles the `null` comparison: `Nullable<T>`. So this isn't just some bizarre corner case, there are legitimate reasons to do this (including the Null-Object Pattern).
Aaronaught
In other languages, including VB, reference equality and value equality are different operators. This makes it clear what type of comparison you are doing and prevents many subtle errors.
Jonathan Allen
+2  A: 

This also falls in line with other warnings C# does (or not does for that matter) like:

if(true)
or 
if(1 == 1)

These will also always have the same result no matter what.

Kevin
Well, sure, but if such code does result in *unreachable* code then that will produce a warning.
Eric Lippert
+3  A: 

Actually, the condition really can be satisfied, at least in Visual Studio 2008. They've fixed this behaviour in VS 2010, but it is not totally inconceivable that there might be another way to create such a condition.

(tl;dr version - in C# 3.5, it's legal to reference this from an anonymous function passed as a constructor argument, but if you actually try to use it, you'll find that this is null.)

Aaronaught
+7  A: 

Wow... I guess I was shamefully wrong

I disagree. I think you still make a good point.

The compiler knows whether the comparison is going to go to a user-defined comparison operator or not, and the compiler knows that if it does not, then 'this' is never null.

And in fact, the compiler does track whether a given expression can legally be null or not, in order to implement a minor optimization on non-virtual method calls. If you have a non-virtual method M and you say foo.M(); then the compiler generates this as "do a virtual call to M with receiver foo". Why? Because we want to throw if foo is null, and a virtual call always does a null check on the receiver. A non-virtual call does not; we'd have to generate it as "check foo for null, and then do a non-virtual call to M", which is longer, slower, more irritating code.

Now, if we can get away without doing the null check, we do. If you say this.M() or (new Foo()).M() then we do NOT generate a virtual call. We generate the non-virtual call without the null check because we know that it cannot be null.

So the compiler has excellent data on whether a particular comparison to null will sometimes, always, or never succeed.

The question then is "if the compiler knows that a particular comparison will never succeed, why not generate a warning for it?"

And the answer is "sometimes we do, and sometimes we don't".

We do in this situation:

int x = 123;
if (x == null) ...

There is an equality operator defined on two nullable ints. x is convertible to nullable int. null is convertible to nullable int. So that equality operator is valid, and therefore is used, and is of course always false. The compiler gives a warning that the expression is always false.

However, due to a bug we accidentally introduced in C# 3, this code does NOT produce that warning:

Guid x = whatever;
if (x == null) ...

Same deal. The nullable guid equality operator is valid, but the warning is suppressed. I'm not sure if we fixed this bug for C# 4 or not. If not, hopefully we'll get it into a service pack.

As for "if (this == null)" I don't know why we don't warn for that. It certainly seems like a good candidate for a warning. The most likely explanation is to follow this logical syllogism:

  • warnings are compiler features
  • compiler features must be (1) thought of, (2) designed, (3) implemented, (4) tested, (5) documented and (6) shipped to you before you can take advantage of the feature.
  • No one has done any of those six necessary things for this feature; we can't ship features that we never thought of in the first place.
  • therefore, no such feature.

There are infinitely many compiler features that we haven't thought of yet; we've implemented none of them.

Another reason to not give a warning here is that we try to give warnings on code which is both likely to be typed by accident and almost certainly wrong for a non-obvious reason. "this == null" is not likely to be typed in by accident, and though it is almost certainly wrong, as you note in the statement of your question, it is obviously wrong.

Compare that to our "guid == null" -- that is likely to happen by accident, because a developer might accidentally think that Guid is a reference type. Guids are usually passed around by reference in C++, so this is an easy mistake to make. The code is almost certainly wrong, but it is wrong in a non-obvious way. So this is a good candidate for a warning. (Which is why it is so unfortunate that this is a warning in C# 2 but not C# 3, due to a bug we introduced.)

Eric Lippert
wow... just wow. Never thought of these.
Andrei Rinea
@Andrei - Eric has access to (in fact, has probably written some of) the C# source code due to his job.
Heath Hunnicutt