'is' versus try cast with null check

Question

I noticed that Resharper suggests that I turn this:

if (myObj.myProp is MyType)
{
   ...
}

into this:

var myObjRef = myObj.myProp as MyType;
if (myObjRef != null)
{
   ...
}

Why would it suggest this change? I'm used to Resharper suggesting optimization changes and code reduction changes, but this feels like it wants to take my single statement and turn it into a two-liner.

According to MSDN:

An is expression evaluates to true if both of the following conditions are met:

expression is not null. expression can be cast to type. That is, a cast expression of the form (type)(expression) will complete without throwing an exception.

Am I misreading that, or doesn't is do the exact same checks, just in a single line without the need to explicitly create another local variable for the null check?

are you using myObjRef later in the code? if you are, you wouldn't be needing the `MyProp` getter after this change. — default, Nov 15 '12 at 20:35

Jeff · Accepted Answer · 2017-12-12T18:15:05.187

178

Because there's only one cast. Compare this:

if (myObj.myProp is MyType) // cast #1
{
    var myObjRef = (MyType)myObj.myProp; // needs to be cast a second time
                                         // before using it as a MyType
    ...
}

to this:

var myObjRef = myObj.myProp as MyType; // only one cast
if (myObjRef != null)
{
    // myObjRef is already MyType and doesn't need to be cast again
    ...
}

C# 7.0 supports a more compact syntax using pattern matching:

if (myObj.myProp is MyType myObjRef)
{
    ...
}

edited Dec 12 '17 at 18:15

answered Nov 15 '12 at 20:40

Jeff

7,504
3
25
34

4

exactly. using 'is' is basically doing something like return ((myProp as MyType) == null) – Bambu Nov 15 '12 at 20:41
Aha, you nailed it. Further proof, if I remove the contents of the if(), the Resharper suggestion goes away. Good catch! – HotN Nov 15 '12 at 20:45
2

As far as changes go though, this is pretty minute. The null check is going to be pretty comparable to the second type check. `as` may be a couple of nanoseconds quicker, but I consider this a premature microoptimization. – Servy Nov 15 '12 at 20:55
4

Also note that the original version is not thread-safe. The value of `myObj` or `myProp` could get changed (by another thread) between the `is` and the cast, causing undesirable behaviour. – Jeff Nov 15 '12 at 21:16
1

I might also add that using `as` + `!= null` will also execute the overridden `!=` operator of `MyType` if defined (even if `myObjRef` is null). While in _most_ cases this is a non-issue (especially if you properly implement it), in some extreme cases (bad code, performance) it might not be desired. (would have to be _pretty extreme_ though) – Chris Sinclair Nov 15 '12 at 21:16
1

@Chris: Right, the correct translation of the code would use `object.ReferenceEquals(null, myObjRef)`. – Ben Voigt Nov 16 '12 at 04:35
That makes a lot more sense, didn't realize it had to cast it twice. Thanks. – Brady Liles Dec 16 '15 at 19:39
@JeffE Just curious, I tried this on a project which is built on .Net Framework 4 in Visual Studio 2019. It compiled successfully. Does that mean this pattern matching is supported in earlier versions of C# as well? – FMFF Dec 02 '19 at 23:40

Francesco Cattoni · Answer 2 · 2017-12-01T22:35:44.857

14

The best option is use pattern matching like that:

if (value is MyType casted){
    //Code with casted as MyType
    //value is still the same
}
//Note: casted can be used outside (after) the 'if' scope, too

edited Dec 01 '17 at 22:35

answered Sep 15 '17 at 09:04

Francesco Cattoni

358
6
13

How exactly this one is better than the second fragment from the question? – Victor Yarema Jan 24 '20 at 20:19
The second fragment of the question is referring to the basic usage of is (without the variable declaration) and in that case you will check the type twice (one in the is statement and another before the cast) – Francesco Cattoni Jan 25 '20 at 10:43

score 9 · Answer 3 · edited May 23 '17 at 10:31

There's no information yet about what actually happens below the belt. Take a look at this example:

object o = "test";
if (o is string)
{
    var x = (string) o;
}

This translates to the following IL:

IL_0000:  nop         
IL_0001:  ldstr       "test"
IL_0006:  stloc.0     // o
IL_0007:  ldloc.0     // o
IL_0008:  isinst      System.String
IL_000D:  ldnull      
IL_000E:  cgt.un      
IL_0010:  stloc.1     
IL_0011:  ldloc.1     
IL_0012:  brfalse.s   IL_001D
IL_0014:  nop         
IL_0015:  ldloc.0     // o
IL_0016:  castclass   System.String
IL_001B:  stloc.2     // x
IL_001C:  nop         
IL_001D:  ret

What matters here are the isinst and castclass calls -- both relatively expensive. If you compare that to the alternative you can see it only does an isinst check:

object o = "test";
var oAsString = o as string;
if (oAsString != null)
{

}

IL_0000:  nop         
IL_0001:  ldstr       "test"
IL_0006:  stloc.0     // o
IL_0007:  ldloc.0     // o
IL_0008:  isinst      System.String
IL_000D:  stloc.1     // oAsString
IL_000E:  ldloc.1     // oAsString
IL_000F:  ldnull      
IL_0010:  cgt.un      
IL_0012:  stloc.2     
IL_0013:  ldloc.2     
IL_0014:  brfalse.s   IL_0018
IL_0016:  nop         
IL_0017:  nop         
IL_0018:  ret

Also worth mentioning is that a value type will use unbox.any rather than castclass:

object o = 5;
if (o is int)
{
    var x = (int)o;
}

IL_0000:  nop         
IL_0001:  ldc.i4.5    
IL_0002:  box         System.Int32
IL_0007:  stloc.0     // o
IL_0008:  ldloc.0     // o
IL_0009:  isinst      System.Int32
IL_000E:  ldnull      
IL_000F:  cgt.un      
IL_0011:  stloc.1     
IL_0012:  ldloc.1     
IL_0013:  brfalse.s   IL_001E
IL_0015:  nop         
IL_0016:  ldloc.0     // o
IL_0017:  unbox.any   System.Int32
IL_001C:  stloc.2     // x
IL_001D:  nop         
IL_001E:  ret

Note however that this not necessarily translates to a faster result as we can see here. There seem to have been improvements since that question was asked though: casts seem to be performed as fast as they used to be but as and linq are now approximately 3 times faster.

Tom · Answer 4 · 2014-01-13T22:47:27.463

Resharper warning:

"Type check and direct cast can be replaced with try cast and check for null"

Both will work, it depends how your code suits you more. In my case I just ignore that warning:

//1st way is n+1 times of casting
if (x is A) ((A)x).Run();
else if (x is B) ((B)x).Run();
else if (x is C) ((C)x).Run();
else if (x is D) ((D)x).Run();
//...
else if (x is N) ((N)x).Run();    
//...
else if (x is Z) ((Z)x).Run();

//2nd way is z times of casting
var a = x as Type A;
var b = x as Type B;
var c = x as Type C;
//..
var n = x as Type N;
//..
var z = x as Type Z;
if (a != null) a.Run();
elseif (b != null) b.Run();
elseif (c != null) c.Run();
...
elseif (n != null) n.Run();
...
elseif (x != null) x.Run();

In my code 2nd way is longer and worse performance.

In your real-world example, there is simply a design problem. If you control the types, just use an interface such as `IRunable`. If you haven't got the control, perhaps you could use `dynamic`? — M. Mimpen, Aug 29 '16 at 11:15

score 3 · Answer 5 · answered Nov 15 '12 at 20:46

To me this seems dependent on what the odds are that it's going to be of that type or not. It would certainly be more efficient to do the cast up front if the object is of that type most of the time. If it's only occasionally of that type then it may be more optimal to check first with is.

The cost of creating a local variable is very negligible compared to the cost of the type check.

Readability and scope are the more important factors for me typically. I would disagree with ReSharper, and use the "is" operator for that reason alone; optimize later if this is a true bottleneck.

(I'm assuming that you are only using myObj.myProp is MyType once in this function)

score 1 · Answer 6 · answered Nov 15 '12 at 20:38

I would say this is to make a strongly-typed version of myObj.myProp, which is myObjRef. This should then be used when you are referencing this value in the block, vs. having to do a cast.

For example, this:

myObjRef.SomeProperty

is better than this:

((MyType)myObj.myProp).SomeProperty

score 0 · Answer 7 · answered Nov 15 '12 at 20:35

0

It should be suggesting a second change as well:

(MyType)myObj.myProp

into

myObjRef

This saves a property access and a cast, compared to the original code. But it's only possible after changing is to as.

answered Nov 15 '12 at 20:35

Ben Voigt

277,958
43
419
720

@Default: No it's not. That doesn't mean it isn't in the code. – Ben Voigt Nov 15 '12 at 20:37
1

sorry.. misunderstood. however, `(MyType)` will throw exception if the cast fails. `as` only returns `null`. – default Nov 15 '12 at 20:37
@Default: The cast won't fail, because the type has already been checked with `is` (that code is in the question). – Ben Voigt Nov 15 '12 at 20:38
1

however, re# wants to replace that code - meaning it wouldn't be there after the suggested change. – default Nov 15 '12 at 20:38
I *think* I'm following your thought here (just took me some time). You mean that the first line *is* somewhere in the code and that line would be simplified after the Re# suggestion to the second line? – default Nov 15 '12 at 20:42

'is' versus try cast with null check

7 Answers7

Linked

Related