Resharper Isn't Always Smart

I was writing some code today, essentially like this:

public class MyClass

{

    private int value;

    public MyClass(int value)

    {

        this.value = value;

    }

    public static bool operator==(MyClass left, MyClass right)

    {

        return left.value == right.value;

    }

 

    public static bool operator !=(MyClass left, MyClass right)

    {

        return !(left == right);

    }

}

    //...

    MyClass myClass1 = new MyClass(1);

    MyClass myClass2 = new MyClass(1);

    if((Object)myClass1 != (Object)myClass2) // "Type cast is reundant"

    {

        Console.WriteLine("not equal");

    }

    else

    {

        Console.WriteLine("equal");

    }

...where Resharper warned that both the casts to Object were redundant and offered a "Quick-fix" (red light bulb) to "Remove cast".  Well, doing that to one of the casts results in a compile error so you have to manually change the other; but what it's suggesting is this:

    if(myClass1 != myClass2)

This completely changes the output of my application from "not equal" to "equal".  What Resharper doesn't know (or doesn't care to check) is that removing those casts switches from a reference comparison to a value comparison and may have different results.  What I wrote with the original code was to test if the two references referenced the same object.  The default behaviour of a class is to do a reference check; but I've overloaded operator== (and operator !=) to perform a value comparison (I've left out the lovely bits that truly gives MyClass value semantics for clarity).

So, when Resharper offers to change your code, be sure you know side-effects of that change before you let it do it.  You could introduce a nasty bug.

kick it on DotNetKicks.com

Published Mon, Mar 31 2008 12:47 by PeterRitchie
Filed under: , ,

Comments

# A case where Resharper will break your code with a

You've been kicked (a good thing) - Trackback from DotNetKicks.com

Monday, March 31, 2008 1:45 PM by DotNetKicks.com

# re: Resharper Isn't Always Smart

Yup, had the same problem a few times, this one still compiles but some "fixes" even generate compile errors, but I always report problems like this.

Anyways, ReferenceEquals is always usefull for this ;)

Monday, March 31, 2008 4:17 PM by Steve

# re: Resharper Isn't Always Smart

This is exactly why I never use a cast to Object to perform a reference comparison: it is confusing to the reader, whether the reader is a human or a refactoring program. Instead I use ReferenceEquals; it immediately and clearly signifies what kind of comparison I am trying to accomplish.

Monday, March 31, 2008 4:43 PM by David Nelson

# re: Resharper Isn't Always Smart

Isn't this just the kind of thing that Object.ReferenceEquals() was invented for?

(Also, Code Analysis does suggest that overriding == is a bad idea for most reference types)

Monday, March 31, 2008 7:40 PM by Wayne Bradney

# re: Resharper Isn't Always Smart

@Peter,

Is this with 4 EAP?

Well, to be fair.  #R does not know that you changed the default meaning of the != operator.

But your message is true, and it is not just when using #R.  Anytime ANYTHING changes your code you need to know the intent of the change before you allow it.

Monday, March 31, 2008 8:07 PM by Derik Whittaker

# re: Resharper Isn't Always Smart

@Wayne: no complaints from Code Analysis about overloading operator==.

@David & Wayne:  All ReferenceEquals does is cast (implicitly) and perform operator==.

Tuesday, April 01, 2008 6:26 AM by PeterRitchie

# re: Resharper Isn't Always Smart

@Derik: Yes, 4 EAP.  It could know.  And if they don't want to check, then don't suggest the "quick-fix".

It's the same with Code Analysis, don't change the code simply to get rid of a warning, unless you know what you're doing...

Tuesday, April 01, 2008 6:29 AM by PeterRitchie

# re: Resharper Isn't Always Smart

I am well aware of how ReferenceEquals operates. I was not saying that it does something different; I was merely pointing out that it is more explicit about your intent and tends to be less confusing.

Tuesday, April 01, 2008 8:59 AM by David Nelson

# re: Resharper Isn't Always Smart

@David:  Depends on point of view, I guess.  To me and all the developers I work with (Object)x==(Object)y is obvious to me.

It is apparent that ReferenceEquals is more R#-friendly.

Tuesday, April 01, 2008 9:12 AM by PeterRitchie

# re: Resharper Isn't Always Smart

Really? I get CA1046 right away when compiling your sample:

Warning 5 CA1046 : Microsoft.Design : 'MyClass' should not overload the equality operator. C:\Users\wayne_bradney\Documents\Visual Studio 2008\Projects\ConsoleApplication7\ConsoleApplication7\Program.cs 7 ConsoleApplication7

Tuesday, April 01, 2008 1:37 PM by Wayne Bradney

# re: Resharper Isn't Always Smart

@Wayne: I ran VS 2008 Code Analysis.

But, there are some valid reasons for implementing the equality operator; it's not without consequences/work; but valid.

Tuesday, April 01, 2008 2:30 PM by PeterRitchie

Leave a Comment

(required) 
(required) 
(optional)
(required)