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.
10 comments

as workaround would this help?

return object.ReferenceEquals(Value, null) ? string.Empty : Value.ToString();
0

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.

0

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.

0

As a workaround that certainly has the desired result. Thanks.

0

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

0

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...

0

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:

public static class GenericMath
{
    private interface INullOp<T>
    {
        bool HasValue(T value);
    }
    
    private struct StructNullOp<T> : INullOp<T>, INullOp<T?> where T : struct
    {
        public bool HasValue(T value)
        {
            return true;
        }
        
        public bool HasValue(T? value)
        {
            return value.HasValue;
        }
    }
    
    private struct ClassNullOp<T> : INullOp<T> where T : class
    {
        public bool HasValue(T value)
        {
            return null != value;
        }
    }
    
    private static class GenericOperator<T>
    {
        public static readonly INullOp<T> NullOp = CreateNullOp<T>();
    }
    
    private static INullOp<T> CreateNullOp<T>()
    {
        Type typeT = typeof(T);
        if (typeT.IsValueType)
        {
            Type nullType = Nullable.GetUnderlyingType(typeT);
            return (INullOp<T>)Activator
                .CreateInstance(typeof(StructNullOp<>)
                .MakeGenericType(nullType ?? typeT));
        }
        
        return (INullOp<T>)Activator
            .CreateInstance(typeof(ClassNullOp<>)
            .MakeGenericType(typeT));
    }
    
    public static bool HasValue<T>(T value)
    {
        return GenericOperator<T>.NullOp.HasValue(value);
    }
}


Now you can replace "if (value == null)" with "if (!GenericMath.HasValue(value))", and everything will work as expected.

0

Yes the value type will be boxed, however your code is a classic case of premature optimization. Performance wise it is quite bad.

Benchmark:

using System;
using System.Collections.Generic;
using System.Linq;
using System.Runtime.InteropServices;
using System.Text;

namespace TestProject
{
    public class HighResolutionTimer
    {
        [DllImport("Kernel32.dll")]
        private static extern bool QueryPerformanceCounter(out long lpPerformanceCount);

        [DllImport("Kernel32.dll")]
        private static extern bool QueryPerformanceFrequency(out long lpFrequency);

        private long StartTime, StopTime;
        private long freq;

        public HighResolutionTimer()
        {
            StartTime = 0;
            StopTime = 0;

            if (QueryPerformanceFrequency(out freq) == false)
            {
                throw new NotSupportedException("High resolution timer is not supported!");
            }
            Start();
        }

        public HighResolutionTimer Start()
        {
            QueryPerformanceCounter(out StartTime);
            return this;
        }

        public HighResolutionTimer Stop()
        {
            QueryPerformanceCounter(out StopTime);
            return this;
        }

        public TimeSpan Duration
        {
            get { return GetDurationAsTimeSpan(StartTime, StopTime); }
        }

        private decimal GetDuration(long start, long stop)
        {
            return (decimal)(stop - start) / freq;
        }

        private TimeSpan GetDurationAsTimeSpan(long start, long stop)
        {
            return TimeSpan.FromTicks((long)Math.Round(GetDuration(start, stop) * TimeSpan.TicksPerSecond));
        }
    }

    public static class GenericMath
    {
        private interface INullOp<T>
        {
            bool HasValue(T value);
        }

        private struct StructNullOp<T> : INullOp<T>, INullOp<T?> where T : struct
        {
            public bool HasValue(T value)
            {
                return true;
            }

            public bool HasValue(T? value)
            {
                return value.HasValue;
            }
        }

        private struct ClassNullOp<T> : INullOp<T> where T : class
        {
            public bool HasValue(T value)
            {
                return null != value;
            }
        }

        private static class GenericOperator<T>
        {
            public static readonly INullOp<T> NullOp = CreateNullOp<T>();
        }

        private static INullOp<T> CreateNullOp<T>()
        {
            Type typeT = typeof(T);
            if (typeT.IsValueType)
            {
                Type nullType = Nullable.GetUnderlyingType(typeT);
                return (INullOp<T>)Activator
                    .CreateInstance(typeof(StructNullOp<>)
                    .MakeGenericType(nullType ?? typeT));
            }

            return (INullOp<T>)Activator
                .CreateInstance(typeof(ClassNullOp<>)
                .MakeGenericType(typeT));
        }

        public static bool HasValue<T>(T value)
        {
            return GenericOperator<T>.NullOp.HasValue(value);
        }
    }

    public class Foo { }

    public static class Extensions
    {
        public static bool IsNull<T>(this T o)
        {
            return o == null;
        }

        public static bool IsNullRef<T>(this T o) where T : class
        {
            return o == null;
        }

        public static bool IsNullEx<T>(this T o)
        {
            return !GenericMath.HasValue(o);
        }
    }

    public class Program
    {
        static void Main(string[] args)
        {
            var timer = new HighResolutionTimer();
            var timeBox = TimeSpan.Zero;
            var timeExplicit = TimeSpan.Zero;
            var timeReference = TimeSpan.Zero;

            const int runs = 10;
            const int iterations = 100000000;

            for (int r = 0; r < runs; r++)
            {
                bool res = false;
                timer.Start();
                for (int i = 0; i < iterations; ++i)
                {
                    res = 1.IsNull();
                }
                timer.Stop();
                Console.WriteLine("1 is null boxing: {0}, iterations {1}, time {2}ms", res, iterations, timer.Duration.TotalMilliseconds);
                timeBox += timer.Duration;

                res = false;
                timer.Start();
                for (int i = 0; i < iterations; ++i)
                {
                    res = 1.IsNullEx();
                }
                timer.Stop();
                Console.WriteLine("1 is null explicit: {0}, iterations {1}, time {2}ms", res, iterations, timer.Duration.TotalMilliseconds);
                timeExplicit += timer.Duration;

                res = false;
                var o = new Foo();
                timer.Start();
                for (int i = 0; i < iterations; ++i)
                {
                    res = o.IsNullRef();
                }
                timer.Stop();
                Console.WriteLine("new Foo is null ref: {0}, iterations {1}, time {2}ms", res, iterations, timer.Duration.TotalMilliseconds);
                timeReference += timer.Duration;
            }

            Console.WriteLine("Time boxing: {0} over {1} runs, avg. {2}", timeBox.TotalMilliseconds, runs, timeBox.TotalMilliseconds / runs);
            Console.WriteLine("Time explicit: {0} over {1} runs, avg. {2}", timeExplicit.TotalMilliseconds, runs, timeExplicit.TotalMilliseconds / runs);
            Console.WriteLine("Time reference: {0} over {1} runs, avg. {2}", timeReference.TotalMilliseconds, runs, timeReference.TotalMilliseconds / runs);
        }
    }
}


Result:

1 is null boxing: False, iterations 100000000, time 140.3865ms
1 is null explicit: False, iterations 100000000, time 707.3473ms
new Foo is null ref: False, iterations 100000000, time 140.3639ms
1 is null boxing: False, iterations 100000000, time 141.6479ms
1 is null explicit: False, iterations 100000000, time 704.0656ms
new Foo is null ref: False, iterations 100000000, time 144.5006ms
1 is null boxing: False, iterations 100000000, time 139.1869ms
1 is null explicit: False, iterations 100000000, time 707.5423ms
new Foo is null ref: False, iterations 100000000, time 140.1634ms
1 is null boxing: False, iterations 100000000, time 139.6888ms
1 is null explicit: False, iterations 100000000, time 707.5673ms
new Foo is null ref: False, iterations 100000000, time 140.6707ms
1 is null boxing: False, iterations 100000000, time 140.1672ms
1 is null explicit: False, iterations 100000000, time 706.7747ms
new Foo is null ref: False, iterations 100000000, time 140.2675ms
1 is null boxing: False, iterations 100000000, time 141.0017ms
1 is null explicit: False, iterations 100000000, time 706.2924ms
new Foo is null ref: False, iterations 100000000, time 139.3428ms
1 is null boxing: False, iterations 100000000, time 141.585ms
1 is null explicit: False, iterations 100000000, time 706.5493ms
new Foo is null ref: False, iterations 100000000, time 136.7124ms
Time boxing: 1403.9657 over 10 runs, avg. 140.39657
Time explicit: 7067.7046 over 10 runs, avg. 706.77046
Time reference: 1403.0553 over 10 runs, avg. 140.30553


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

0

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.

0

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

0

Please sign in to leave a comment.