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.

Answer 1

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.