C#, C# 8, Evil code

Lying to the compiler

25 Comments

This morning I tweeted this:

Just found a C# 8 nullable reference types warning in Noda Time. Fixing it by changing Foo(x, x?.Bar) to Foo(x, x?.Bar!) which looks really dodgy… anyone want to guess why it’s okay?

This attracted more interest than I expected, so I thought I’d blog about it.

First let’s unpack what x?.Bar! means. x?.Bar means “if x is non-null, the value of x.Bar; otherwise, the corresponding null value”. The ! operator at the end is introduced in C# 8, and it’s the damn-it operator (more properly the “null-forgiving operator”, but I expect to keep calling it the damn-it operator forever). It tells the compiler to treat the preceding expression as “definitely not null” even if the compiler isn’t sure for whatever reason. Importantly, this does not emit a null check in the IL – it’s a compile-time only change.

When talking about the damn-it operator, I’ve normally given two scenarios where it makes sense:

  • When testing argument validation
  • When you have invariants in your code which allow you to know more than the compiler does about nullability. This is a little bit like a cast: you’re saying you know more than the compiler. Remember that it’s not like a cast in terms of behaviour though; it’s not checked at execution time.

My tweet this morning wasn’t about either of these cases. It’s in production code, and I absolutely believe that it’s possible for x?.Bar to be null. I’m lying to the compiler to get it to stop it emitting a warning. The reason is that in the case where the value is null, it won’t matter that it’s null.

The actual code is in this Noda Time commit, but the code below provides a simplified version. We have three classes:

  • Person, with a name and home address
  • Address, with some properties I haven’t bothered showing here
  • Delivery, with a recipient and an address to deliver to
using System;

public sealed class Address
{
    // Properties here
}

public sealed class Person
{
    public string Name { get; }
    public Address HomeAddress { get; }

    public Person(string name, Address homeAddress)
    {
        Name = name ??
            throw new ArgumentNullException(nameof(name));
        HomeAddress = homeAddress ??
            throw new ArgumentNullException(nameof(homeAddress));
    }
}

public sealed class Delivery
{
    public Person Recipient { get; }
    public Address Address { get; }

    public Delivery(Person recipient)
        : this(recipient, recipient?.HomeAddress!)
    {
    }

    public Delivery(Person recipient, Address address)
    {
        Recipient = recipient ??
            throw new ArgumentNullException(nameof(recipient));
        Address = address ??
            throw new ArgumentNullException(nameof(address));
    }
}

The interesting part is the Delivery(Person) constructor, that delegates to the Delivery(Person, Address) constructor.

Here’s a version that would compile with no warnings:

public Delivery(Person recipient)
    : this(recipient, recipient.HomeAddress)

However, now if recipient is null, that will throw NullReferenceException instead of the (preferred) ArgumentNullException. Remember that nullable reference checking in C# 8 is really just advisory – the compiler does nothing to stop a non-nullable reference variable from actually having a value of null. This means we need to keep all the argument validation we’ve already got.

We could validate recipient before we pass it on to the other constructor:

public Delivery(Person recipient)
    : this(recipient ?? throw new ArgumentNullException(...),
           recipient.HomeAddress)

That will throw the right exception, but it’s ugly and more code than we need. We know that the constructor we’re delegating to already validates recipient – we just need to get that far. That’s where the null-conditional operator comes in. So we can write:

public Delivery(Person recipient)
    : this(recipient, recipient?.HomeAddress)

That will behave as we want it to – if recipient is null, we’ll pass null values as both arguments to the other constructor, and it will validate them. But now the compiler warns that the second argument could be null, and the parameter is meant to be non-null. The solution is to use the damn-it operator:

public Delivery(Person recipient)
    : this(recipient, recipient?.HomeAddress!)

Now we get the behaviour we want, with no redundant code, and no warnings. We’re lying to the compiler and satisfied that we’re doing so sensibly, because recipient?.HomeAddress is only null if recipient is null, and we know that that will be validated first anyway.

I’ve added a comment, as it’s pretty obscure otherwise – but part of me just enjoys the oddity of it all :)

25 thoughts on “Lying to the compiler”

  1. The example would make more sense if it was Delivery(Address address, Person recipient).

    As it is, this(recipient ?? throw new ArgumentNullException(…), recipient.HomeAddress) would throw ArgumentNullException and beauty/ugliness is on the eye of the beholder.

    On the other hand, this(recipient.HomeAddress, recipient ?? throw new ArgumentNullException(nameof(…))) would throw NullReferenceException.

    Like

    Reply

    1. Except I’d try to avoid writing overloads like that in the first place, with the position of the recipient varying :)

      I still prefer to avoid the duplication here, even though it’s an option.

      Like

      Reply

  2. I’m pretty sure the comment is longer than the extra length of:

    public Delivery(Person recipient)
    : this(recipient ?? throw new ArgumentNullException(…),
    recipient.HomeAddress)

    Which in my opinion is an indication that the option which doesn’t require a comment is the clearest and easiest to understand.

    I prefer being explicit about what’s happening, even if that leads to code duplication.

    Liked by 1 person

    Reply

    1. Each to their own. I can see your point of view, but I think I still prefer this approach. If I use it more often, I may well not need the comment in a bit.

      Like

      Reply

      2. I think you will find, Jon, that you’ll still want to comment OR put that in a code guide of some kind.

        Like

        Reply

  3. I am confused tho.

    Coming from languages that have null awareness for much longer, the following would never allow null values:
    public Delivery(Person recipient, Address address)

    Yet, in your code, you write code to handle the cases where the values would be null.
    Shouldnt the declaration then be this?
    public Delivery(Person? recipient, Address? address)

    At which point, it makes very little sense to allow null values since they would only throw an ArgumentNullException (or NRE).

    I thought that the whole point of C# 8 null awareness was that Address is definitely not null, but Address? is nullable.

    Like

    Reply

    1. No, the declaration is correct – it’s intended that passing in a null reference should cause an exception, hence the validation. But that doesn’t mean the parameters definitely won’t be null – otherwise we wouldn’t need to validate them at all. That’s really about the overall design of C# 8’s nullable references, which is well beyond the scope of this article, but importantly:

      Turning on nullable reference handling does not change the generated IL (preserves backward compatibility)
      Code that’s not nullable-aware could easily be calling these methods (or they could be using reflection)
      The CLR hasn’t changed to support any of this

      All of that adds up to “you need to be prepared for your non-nullable parameters to have null values”.

      Like

      Reply

      1. i think this design is source of problem: we say it’s not null but maybe null
        it might be ok for time being while c# evolves but one day they should introduce full non null design with clr support
        in this lang or another

        Like

        Reply

  4. I believe that storing UTC time always is the right way to go. Why? Because there has to be a right way to go. If you remove this rather common pattern for using datetimes, then you introduce inconsistency in the way people handle time. The conversion from local to UTC at any time has to be correct at any time. If you want conversion from UTC to local, then you have to take into account the times (a bit recursive, but OK) at which the rules have changed to get a correct value across rule change boundaries.

    This is one of the reasons I am so against daylight saving time, because you skip times and double them later. You break the bijectivity of the timeline correlation between UTC and local time. Go even further: you want to create software that works on the Earth, but also Mars. There has to be something akin to universal time, something that doesn’t really depend on the rotation of any planet relative to any star.

    Like

    Reply

    1. I think you meant to post this on another post – but I disagree. Your logic seems to be “one size has to fit all” but it really doesn’t. It’s not so hard to take different approaches for different situations. I doubt that you try to use a single data type for all situations – why limit yourself when it comes to thinking about date and time?

      Like

      Reply

    1. No, that would give the same warning – the ?? null would have no effect, as you’re basically saying “If the result is null, use null instead”

      Like

      Reply
  6. Pingback: The Morning Brew - Chris Alcock » The Morning Brew #2753

  7. I like the brevity and apparent self-contradiction of the foo?.bar! expression. This could become a common idiom to avoid pesky warnings in these scenarios. I just hope it doesn’t get overused to the extent that, having understood the idiom, it is not easy to see that it is being used correctly. Perhaps C# will continue to evolve to avoid warnings in this situation. But then I suppose you might become puzzled as to why a warning wasn’t being issued :-/

    Like

    Reply

  8. I wonder whether the ! operator should be used to say “let me do what I want regarding nullability” or to communicate “I know this isn’t null”. If the latter, I find your code to be confusing to the reader (so it _needs_that comment, which seems like a smell to me). In that case, how about disabling the warning using a #pragma instead? Or creating an extension method for exactly this scenario?

    Like

    Reply

  9. Hi. Just a quick thought about this. I found the headline of this article quite thrilling and learnt something new about C# 8. However I would have written the constructor of Delivery like this:

    public Delivery(Person recipient, Address address = null)
    {
    Recipient = recipient
    ?? throw new ArgumentNullException(nameof(recipient));

    Address = address
    ?? recipient.HomeAddress 
    ?? throw new ArgumentException("Homeless recipient detected!", nameof(recipient));

    }

    I think this is exactly what default parameters are made for. No need for multiple constructors and lying to the compiler at all. Additionally, by the design of Person, Person.HomeAddress cannot be null anyways, so the last line of the constructor could even be dropped.

    Like

    Reply

    1. Maybe… except then if someone calls the constructor with an explicit address that they didn’t expect to be null, they end up with the recipient’s home address instead of an exception… that could be unexpected. In other words, this has quite different behaviour to the code I posted, in the case where the address is passed by the caller. Sometimes that’s fine – sometimes it wouldn’t be.

      Like

      Reply

      1. True, but this is like the caller calling Delivery(Person) instead of Delivery(Person, Address) by mistake. We cannot handle every mistake a caller COULD actually make. I mean by reflection he could even set a Person’s HomeAddress to null if he wanted to and the like…

        Like

        Reply

  10. True, but this is like the caller calling Delivery(Person) instead of Delivery(Person, Address) by mistake.

    I don’t think it is – it’s an unexpected situation at execution time, rather than a clear bug at compile-time.

    It’s not a matter of trying to handle every possible mistake a caller could make – it’s a matter of defining the semantics for the Delivery(Person, Address) constructor. If we want the semantics of “use the person’s home address if the specified address is null” then your solution is fine. But those aren’t the semantics of the original code, and I think the original semantics are equally reasonable. Define the semantics, and then write the code appropriately – I wouldn’t write the shortest code and then just assume that the resulting semantics are appropriate.

    Like

    Reply

    1. I see your point and it might be a delicate difference, but in my opinion your original semantics of “use the person’s home address if there’s no address specified” and my semantics of “use the person’s home address if the specified address is null” are the same.

      If the caller really wants to make a delivery to a certain address he will make sure this address isn’t null.

      Anyway. Thanks for the article and let’s everyone form an own opinion on this, because I think there’s some truth in both of our perspectives. And I don’t want to spam this comments section any further ;)

      Like

      Reply

  11. Personally I think your complaints about the alternatives – they are “too much code” or “ugly” are a sign that you don’t expect someone else to take your code and understand it. You are obviously a genius but in a normal scenario you need to code for the lowest common denominator, and duplication or ugliness are not a problem compared to clarity. I bloody love NodaTime though.

    Like

    Reply

  12. It’s a neat trick, but the more I write code the more I edge away from neat tricks in favour of the more boring, straightforward, self-explanatory alternatives. Another little advantage to the more boring, verbose alternative of throwing the exception in the single arg constructor is that the stack trace of the ArgumentNullException will be that one line shorter. In general, I think it’s good practice to make failures as shallow as possible so that somebody reading the stack trace can more quickly identify where the bug was truly introduced. In this case, it’s just one extra line, but it all adds up :)

    Like

    Reply

  13. You want authorize null in non null reference because you know more than compiler. I think it’s one of scenario where ‘!’ make sense.

    Like

    Reply
  14. Pingback: How to process multiple background concurrent Tasks in c#

Leave a comment