How to prevent warning "possible compare of value type with 'null'"?
I'm trying to override the ToString() method in a generic class. A trimmed down version of the class is
using System;
public class GenericClass<T> {
public T Value { get; set; }
public override string ToString() {
return (Value != null) ? Value.ToString() : string.Empty;
}
}
ReSharper gives me a warning: "possible compare of value type with 'null'" on the (Value != null) expression. Since my class is meant to be able to both take reference types and value types, it is true that there is a possible compare of a value type with null. I don't know why it would be a problem though (the code runs just fine).
I'd like to know if and how I can rewrite my code so the warning disappears. I've tried
public class GenericClass<T> {
public T Value { get; set; }
public override string ToString() {
if (Value is ValueType) return Value.ToString();
return (Value != null) ? Value.ToString() : string.Empty;
}
}
in which case the (Value != null) will only be evaluated if Value is not a ValueType. But the warnings remains. This seems like a bug in ReSharper.
I found this post http://devnet.jetbrains.net/message/5199280#5199280 from 2007 that describes the same problem. Unfortunately it doesn't have any replies.
I'm not sure if I'm misunderstanding something, this is a bug, or this is a feature request.
Please sign in to leave a comment.
as workaround would this help?
Actually, this is one of the analysis with lots of false positives. The main idea of this analysis is that comparison with 'null' for reference types is always false, and thus behavior of code could be wrong if there is no 'class' constraint.
I suppose you mean the result of comparing a ValueType with null is always false?
That's why I already tried to check first if it's not a ValueType. I would've hoped the code analyzer is smart enough to see that in my (Value!=null) check, the argument can't be a ValueType anymore.
As a workaround that certainly has the desired result. Thanks.
I would dare to say that in more than 99% of the cases where something like "if (x != null)" appears it is used to check whether or not it is safe to call a method on it (e.g. x.ToString()) without causing a null reference exception and therefor the statement is exactly what the author had in mind. I usually disable this warning as it is more annoying than helpful. A warning should catch common pitfalls or bad style but somehow I have trouble to imagine pitfalls for this case and I can't see it as bad style.
I also think there is a bug in the suggestion which comes along with the warning: It offers you to convert the expression to "if (x != default(T))" which is wrong. It won't compile - at least not on C# 3.0 (.Net 3.5). It's also wrong here: http://confluence.jetbrains.net/display/ReSharper/Possible+compare+of+value+type+with+null
Even if would be legal the suggestion would very likely introduce unexpected behaviour.
Cheers
Christian
Thank you for the feedback.
For now, I'm still deciding what to do with this analysis, because it actually has a lot of false-positives, but could though catch potentional bug...
I'm not sure whether the JIT compiler is clever enough to remove the null check when the type parameter is a value type. You might end up with a call to "object.Equals(object, object)", which would cause the value to be boxed.
Here's a workaround inspired by the MiscUtil project:
Now you can replace "if (value == null)" with "if (!GenericMath.HasValue(value))", and everything will work as expected.
Yes the value type will be boxed, however your code is a classic case of premature optimization. Performance wise it is quite bad.
Benchmark:
Result:
So your code actually takes about 5 times longer than just leaving it to the compiler and boxing a value type seems to incur not much of an overhead (at least in this scenario)
Cheers
Christian
It looks like the JIT compiler is smart enough to remove the null comparison check for non-nullable value types, at least in a release build. In a debug build, the difference is nowhere near as noticable. There's also some variation between x86 and x64 executables, and between .NET 3.5 and .NET 4.0 builds, but nothing significant.
I can get the code down to around 2x slower by building a delegate, but it's probably not worth it. I'll just stick with "if (value == null)" and ignore the R# warning.
BTW, you don't really need your HighResolutionTimer class - the Stopwatch class introduced in .NET 2.0 does the same thing.
Didn't try it in debug - it just shows that doing something in your own code instead of using the standard language construct takes optimization potential away from the compiler. I have found so many different "workarounds" on google for this trying to solve a problem which actually isn't one. Really strange.
Missed the StopWatch class. The HPT class comes from before 2.0 and has stuck around in our lib since then. Thanks for the reminder.
Cheers
Christian