What should I do about "Possible compare of value type with 'null'"?

Question

While writing this method for a custom NUnit Constraint.

    private void AddMatchFailure<TExpected, TActual>(string failureName, TExpected expected, TActual actual)
    {
        _matchFailures.Add(
            String.Format(MatchFailureFormat, failureName,
            (expected == null) ? "null" : expected.ToString(),
            (actual == null) ? "null" : actual.ToString()));
    }

Resharper warns that expected and actual might be ValueType objects.

e.g. TExpected is DateTime
   expected == null;//  but DateTime is a struct.

What are the rules when comparing a ValueType to null and how should I write the method to account for that without limiting the generic parameters by adding a class constraint?

score 82 · Accepted Answer · answered Mar 17 '11 at 15:00

82

Don't change the code - just ignore the warning. If the type parameter is a non-nullable value type, the comparison will always fail and it'll always call ToString() instead. I don't know whether it's actually JITted away, but I wouldn't be surprised... and this doesn't sound like it's performance-critical code anyway :)

I'd personally leave the warning "on", but ignore it in this particular case - possibly with a comment.

I think I came across the same warning a few times when reimplementing LINQ to Objects.

answered Mar 17 '11 at 15:00

Jon Skeet

1,421,763
867
9,128
9,194

Since there's no way to compare a value type with null, there's no way to that the comparison won't be "JITted away". What else could the JITter do? – Gabe Mar 17 '11 at 15:05
@Gabe: It could possibly call into some other code which will always return null. I'd hope not though. – Jon Skeet Mar 17 '11 at 15:06
65

I've added "// I have Mr. Skeets express permission to ignore this warning." seems to do the trick :) – Grokodile Mar 17 '11 at 15:07
19

@panamack: Could I request an apostrophe in "Skeet's"? Ta. – Jon Skeet Mar 17 '11 at 15:09
66

certainly, "// I have Mr. Skeet(TODO add apostrophe here)s express permission to ignore this warning." – Grokodile Mar 17 '11 at 15:29
15

It's worth noting that if someone is truly bothered by the error message, they could always do `Object.ReferenceEquals(objA, null)` which is really just calling `objA == objB` in the .NET source code. And, I'm pretty sure that this will be JITted or inlined somehow. The nice thing is that the error message disappears though, and the call is still valid :) – myermian Jul 31 '13 at 21:10
4

For completeness, Eric Lippert [said](http://stackoverflow.com/a/8824259/1083771) that the JIT does in fact replace a "val == null" check with "false" when compiling the method for a type parameter that's a non-nullable value type. – Joe Amenta Jan 18 '15 at 14:36
It seems like Resharper has dropped this warning. Can anyone confirm with official documentation? – julealgon Jun 16 '17 at 12:54
@julealgon I'm still seeing this warning with ReSharper 2017.3.3 – Zero3 Mar 22 '18 at 14:58
That's weird @Zero3. Now on our tools, only Sonarqube seems to complain about this. Are you sure your instance is the same as the one presented here and not another scenario? – julealgon Mar 22 '18 at 20:03
1

@julealgon Good point. I just doublechecked by copy-pasting the code in the question, and ReSharper does indeed give me the warnings mentioned in the question. It is worth noting that this ReSharper inspection actually is disabled by default, which might be why you are not seeing it in your project. – Zero3 Apr 04 '18 at 15:52
Oh, this could definitely be why @Zero3. It's possible that it was once enabled by default, and was changed to disabled in newer versions. That would totally explain the behavior I saw across time on our projects here. – julealgon Apr 05 '18 at 18:10
I just found that Dictionary implementation does comparisons generic-type variables with null. So we could call it MS standard :) and ignore these warnings. https://referencesource.microsoft.com/#mscorlib/system/collections/generic/dictionary.cs,fd1acf96113fbda9,references – Pavlo K Oct 11 '18 at 17:57

jason · Answer 2 · 2011-03-17T15:23:02.233

6

What are the rules when comparing a ValueType to null and how should I write the method to account for that without limiting the generic parameters by adding a class constraint?

If you do not know that they will be reference types, then you can say

private void AddMatchFailure<TExpected, TActual>(
    string failureName,
    TExpected expected,
    TActual actual
) {
    _matchFailures.Add(
        String.Format(MatchFailureFormat, failureName,
        IsDefault<TExpected>(expected) ? DefaultStringForType<TExpected>() : expected.ToString(),
        IsDefault<TActual>(actual) ? DefaultStringForType<TActual>() : actual.ToString()
    );
}

private bool IsDefault<T>(T value) {
    if(typeof(T).IsValueType) {
        return default(T).Equals(value);
    }
    else {
        return Object.Equals(null, value);
    }
}

private string DefaultStringForType<T>() {
    if(typeof(T).IsValueType) {
        return default(T).ToString();
    }
    else {
        return "null";
    }
}

edited Mar 17 '11 at 15:23

answered Mar 17 '11 at 15:01

jason

236,483
35
423
525

But I think he wants to compare to `null`. – CodesInChaos Mar 17 '11 at 15:11
@CodeInChaos: `default` of a reference type is `null`. – jason Mar 17 '11 at 15:14
1

@Jason but it isn't `null` for a non nullable value type. It makes sense for the comparison to simply become a constant `false` for types without `null`. And that's probably the desired behavior for the OPs code. – CodesInChaos Mar 17 '11 at 15:17
@CodeInChaos: `default(Nullable)` where `T` is a non-nullable value type is the instance of `Nullable` where `Nullable.HasValue` is `false` (i.e., it is `null`). – jason Mar 17 '11 at 15:22
In my code, I simply want to never call null.ToString(). However, I also want my NUnit Constraint to be expressive when Matches() fails so thanks for reminding me how to use the default keyword with structs, I'd forgotten. – Grokodile Mar 17 '11 at 16:10
1

@Jason I know that, and that's unrelated to my objection to your code. Your `DefaultStringForType` doesn't handle a `Nullable` that's `null` correctly. And even apart from that the OP's code is much cleaner than yours. – CodesInChaos Mar 17 '11 at 18:20
While this behaviour may sometimes be desired, it clearly isn't what the OP wants. It's clear he only seeks to prevent `NullReferenceExceptions` and use a `"null"` string instead. This code however would print `0`, `false` or other default values, is more complicated, not intuitive and for what it's worth, also slower than a simple comparison with `null`. – enzi Mar 18 '15 at 08:52

score 2 · Answer 3 · edited May 23 '12 at 08:22

2

I'm using something like this to check for null on generic types:

if (Equals(result, Default(T)))

edited May 23 '12 at 08:22

Peter Lang

54,264
27
148
161

answered May 23 '12 at 08:13

Billy

107
4

9

Be careful! `default(int)` is 0, which can be a valid and explicitly set value. Checking for `default(T)` is quite different from checking for null. – ANeves Dec 10 '13 at 21:26
1

+1 to @ANeves, resharper suggest this in there help page and to me it code smells http://confluence.jetbrains.com/display/ReSharper/Possible+compare+of+value+type+with+null – Choco Smith Sep 17 '14 at 12:29
I also hate that example because it is clearly a case where most people would want `0.ToString()` to be called... – NobodysNightmare Sep 25 '14 at 07:15

score -3 · Answer 4 · answered Mar 17 '11 at 14:59

-3

private void AddMatchFailure<TExpected, TActual>(string failureName, TExpected expected, TActual actual)
{
    _matchFailures.Add(
        String.Format(MatchFailureFormat, failureName,
        (expected == default(TExpected)) ? "null" : expected.ToString(),
        (actual == default(TActual)) ? "null" : actual.ToString()));
}

Should do it.

default(T) gives the default value for that type, for reference types that's null - for others it depends. (Enums it's the equivalent of (enumType)0 for example).

answered Mar 17 '11 at 14:59

Massif

4,327
23
25

2

Then it will print "null" instead of 0 if you're comparing integers. Way to make a test failure hard to understand ;) – Jon Skeet Mar 17 '11 at 15:01
That gives me a compiler warning "Cannot apply operator == to operands of type 'TExpected' and 'TExpected'" – Grokodile Mar 17 '11 at 15:03
@Jon this is true, but it's the only way to get rid of the warning that fits the parameters. :p – Massif Mar 17 '11 at 15:03
2

The question doesn't ask how to get rid of the warning. It asks what to do about it :) – Jon Skeet Mar 17 '11 at 15:07
@Jon - fair enough... (now, where's the emoticon for "graciously concede") – Massif Mar 17 '11 at 15:19

What should I do about "Possible compare of value type with 'null'"?

4 Answers4

Linked