When to null-check arguments with nullable reference types enabled

Question

Given a function in a program using C# 8.0's nullable reference types feature, should I still be performing null checks on the arguments?

void Foo(string s, object o)
{
    if (s == null) throw new ArgumentNullException(nameof(s)); // Do I need these?
    if (o == null) throw new ArgumentNullException(nameof(o));
    ...
}

None of the code is part of a public API so I suspect that these checks may be redundant. The two parameters are not marked as nullable, so the compiler should warn if any calling code may be passing in null.

If you have control over the entire codebase that will be calling this method, and all of it is compiled with C# 8 with this feature enabled, and you're diligent and fix all warnings/errors the compiler produce, then you could remove those if-statements. If you're unsure about any of those requirements, then you should keep the if-statements. — Lasse V. Karlsen
It is clear to me through the comments in the now-deleted answer that you're not looking to understand the compiler checks, but you're asking about other peoples viewpoints. I have thus voted to close this question as primarily based on opinion. As have been stated in the answer and those comments, the compiler checks are not a substitute for the if-statements or contract system and is quite easy to bypass. Whether this risk is enough for you or not is entirely up to you. However, it seems you already know what the new compiler checks does. — Lasse V. Karlsen
I have been using JetBrains Annotations along with ReSharper/Rider for quite some time and they help you get confidence that you're code is doing the right thing, but they're not a substitute for the actual checks so I have both. I will continue to have both after nullable reference types comes along because the code I write may be consumed by code compiled without those checks, or the writer of that code could even just outright lie to the compiler. (I have been known to employ the odd trick here and there when I feel it is needed). — Lasse V. Karlsen
The compiler cannot literally warn for any calling code -- there are escape hatches and overrides (like the null-forgiving operator) that will stymie its analysis. Whether or not you'd add those checks should be largely independent of whether you're using nullable references or not -- if you already felt confident not having them, you can continue to not have them; if you didn't, then you should probably leave them in. The new checks make it less probable that errors are introduced. Whether they make it improbable enough that you don't need them anymore will depend. — Jeroen Mostert
@LasseVågsætherKarlsen that's the kind of opinion-based question that's explicitly encouraged. This is the Many good questions generate some degree of opinion based on expert experience. At this point in time, many people know the feature's description but very few know about the actual compiler limitations. Even fewer have experience on migrating large projects to use nullable references. I can name 3, and one already answered. The other two work at SO — Panagiotis Kanavos

Jon Skeet Jon Skeet · Accepted Answer · 2019-02-05T15:39:53

Given a function in a program using C# 8.0's nullable reference types feature, should I still be performing null checks on the arguments?

That depends on how certain you are of all the paths through your API. Consider this code:

public void Foo(string x)
{
    FooImpl(x);
}

private void FooImpl(string x)
{
    ...
}

Here FooImpl isn't part of the public API, but can still receive a null reference if Foo doesn't validate its parameter. (Indeed, it may be relying on Foo to perform argument validation.)

Checking in FooImpl is certainly not redundant in that it's performing checks at execution time that the compiler cannot be absolutely certain about at compile-time. Nullable reference types improve the general safety and more importantly the expressiveness of code, but they're not the same kind of type safety that the CLR provides (to stop you treating a string reference as a Type reference, for example). There are various ways the compiler can be "wrong" about its view of whether or not a particular expression might be null at execution time, and the compiler can be overridden with the ! anyway.

More broadly: if your checks weren't redundant before C# 8, they're not redundant after C# 8, because the nullable reference type feature doesn't change the IL generated for the code other than in terms of attributes.

So if your public API was performing all the appropriate parameter checking (Foo in the example above) then the checking in the code was already redundant. How confident are you of that? If you're absolutely confident and the impact of being wrong is small, then sure - get rid of the validation. The C# 8 feature may help you gain confidence in that, but you still need to be careful you don't get too confident - after all - the code above would give no warnings.

Personally I'm not removing any parameter validation when updating Noda Time for C# 8.

When to null-check arguments with nullable reference types enabled

1 Answers