C# 8, Noda Time

First steps with nullable reference types

44 Comments

This blog post is effectively a log of my experience with the preview of the C# 8 nullable reference types feature.

There are lots of caveats here: it’s mostly “as I go along” so there may well be backtracking. I’m not advising the right thing to do, as I’m still investigating that myself. And of course the feature is still changing. Oh, and this blog post is inconsistent about its tense. Sometimes I write in the present tense as I go along, sometimes I wrote in the past tense afterwards without worrying about it. I hope this isn’t/wasn’t/won’t be too annoying.

I decided that the best way of exploring the feature would be to try to use it with Noda Time. In particular:

  • Does it find any existing bugs?
  • Do my existing attributes match what Roslyn expects?
  • Does the feature get in the way, or improve my productivity?

Installation

I started at the preview page on GitHub. There are two really important points to note:

  • Do this on a non-production machine. I used an old laptop, but presumably you can use a VM instead.
  • Uninstall all versions of Visual Studio other than VS2017 first

I ended up getting my installation into a bad state, and had to reinstall VS2017 (and then the preview) before it would work again. Fortunately that takes a lot less time than it used to.

Check it works

The preview does not work with .NET Core projects or the command-line csc

It’s only for old-style MSBuild projects targeting the .NET framework, and only from Visual Studio.

So to test your installation:

  • Create a new .NET Framework desktop console app
  • Edit Program.cs to include: string? x = null;
  • Build

If you get an error CS0453 (“The type ‘string’ must be a non-nullable value type…”) then it’s not working. If it builds with maybe a warning about the variable not being used, you’re good to go.

First steps with Noda Time

The first thing I needed to do was convert Noda Time to a desktop project. This didn’t require the preview to be installed, so I was able to do it on my main machine.

I created a new solution with three desktop projects (NodaTime, NodaTime.Test and NodaTime.Testing), and added the dependencies between the projects and external ones. I then copied these project files over the regular Noda Time ones.

Handy tip: if you add <Compile Include="**\*.cs" /> in an MSBuild file and open it in Visual Studio, VS will replace it with all the C# files it finds. No need for tedious “Add existing” all over the place.

A small amount of fiddling was required for signing and resources, and then I had a working copy of Noda Time targeting just .NET 4.5. All tests passed :)

For anyone wanting to follow my progress, the code is in a branch of my fork of Noda Time although I don’t know how long I’ll keep it for.

Building with the preview

After fetching that branch onto my older laptop, it built first time – with 228 warnings, most of which were “CS8600: Cannot convert null to non-nullable reference.” Hooray – this is exactly what we want. Bear in mind that this is before I’ve made any changes to the code.

The warnings were split between the three projects like this:

  • NodaTime: 94
  • NodaTime.Testing: 0
  • NodaTime.Test: 134

Follow the annotations

Noda Time already uses [CanBeNull] and [NotNull] attributes for both parameters and return types to indicate expectations. The first obvious step is to visit each application of [CanBeNull] and use a nullable reference type there.

To make it easier to see what’s going on, I first unloaded the NodaTime.Test project. This was so that I could concentrate on making NodaTime self-consistent to start with.

Just doing that actually raised the number of warnings from 94 to 110. Clearly I’m not as consistent as I’d like to be. I suspect I’ve got plenty of parameters which can actually be null but which I didn’t apply the annotation to. It’s time to start actually looking at the warnings.

Actually fix things

I did this in a completely haphazard fashion: fix one warning, go onto another.

I’ve noticed a pattern that was already feasible before, but has extra benefits in the nullable reference type world. Instead of this:

// Old code
string id = SomeMethodThatCanReturnNull();
if (id == null)
{
    throw new SomeException();
}
// Use id knowing it's not null

… I can use the ?? operator with the C# 7 feature of throw expressions:

// Old code
string id = SomeMethodThatCanReturnNull() ??
    throw new SomeException();
// Use id knowing it's not null

That avoids having a separate local variable of type string?, which can be very handy.

I did find a few places where the compiler could do better at working out nullity. For example:

// This is fine
string? x = SomeExpressionThatCanReturnNull();
if (x == null)
{
    return;
}
string y = x;

// This creates a warning: the compiler doesn't know that x
// can't be null on the last line
string? x = SomeExpressionThatCanReturnNull();
if (ReferenceEquals(x, null))
{
    return;
}
string y = x;

The preview doc talks about this in the context of string.IsNullOrEmpty; the ReferenceEquals version is a little more subtle as we can’t determine nullity just from the output – it’s only relevant if the other argument is a constant null. On the other hand, that’s such a fundamental method that I’m hopeful it’ll get fixed.

Fixing these warnings didn’t take very long, but it was definitely like playing Whackamole. You fix one warning, and that causes another. For example, you might make a return type nullable to make
a return null; statement work – and that affects all the callers.

I found that rebuilding would sometimes find more warnings, too. At one point I thought I was done (for the time being) – after rebuilding, I had 26 warnings.

I ran into one very common problem: implementing IEquatable<T> (for a concrete reference type T). In every case, I ended up making it implement IEquatable<T?>. I think that’s the right thing to do… (I didn’t do it consistently though, as I found out later on. And IEqualityComparer<T> is trickier, as I’ll talk about later.)

Reload the test project

So, after about an hour of fixing warnings in the main project, what would happen when I reload the test project? We previously had 134 warnings in the test project. After reloading… I was down to 123.

Fixing the test project involved fixing a lot more of the production code, interestingly enough. And that led me to find a limitation not mentioned on the preview page:

public static NodaFormatInfo GetInstance(IFormatProvider? provider)
{
    switch (provider)
    {
        case null:
            return ...;
        case CultureInfo cultureInfo:
            return ...;
        case DateTimeFormatInfo dateTimeFormatInfo;
            return ...;
        default:
            throw new ArgumentException($"Cannot use provider of type {provider.GetType()}");
    }
}

This causes a warning of a possible dereference in the default case – despite the fact that provider clearly can’t be null, as otherwise it would match the null case. Will try to provide a full example in a bug report.

The more repetitive part is fixing all the tests that ensure a method throws an ArgumentNullException if called with a null argument. As there’s no compile-time checking as well, the argument
needs to be null!, meaning “I know it’s really null, but pretend it isn’t.” It makes me chuckle in terms of syntax, but it’s tedious to fix every occurrence.

IEqualityComparer<T>

I have discovered an interesting problem. It’s hard to implement IEqualityComparer<T> properly. The signatures on the interface are pretty trivial:

public interface IEqualityComparer<in T>
{
    bool Equals(T x, T y);
    int GetHashCode(T obj);
}

But problems lurk beneath the surface. The documentation for the Equals() method doesn’t state what should happen if x or y is null. I’ve typically treated this as valid, and just used the normal equality rules (two null references are equal to each other, but nothing else is equal to a null reference.) Compare that with GetHashCode(), where it’s explicitly documented that the method should throw ArgumentNullException if obj is null.

Now think about a type I’d like to implement an equality comparer for – Period for example. Should I write:

public class PeriodComparer : IEqualityComparer<Period?>

This allows x and y to be null – but also allows obj to be null, causing an ArgumentNullException, which this language feature is trying to eradicate as far as possible.

I could implement the non-nullable version instead:

public class PeriodComparer : IEqualityComparer<Period>

Now the compiler will check that you’re not passing a possibly-null value to GetHashCode(), but will also check that for Equals, despite it being okay.

This feels like it’s a natural but somewhat unwelcome result of the feature arriving so much later than the rest of the type system. I’ve chosen to implement the nullable form, but still throw the
exception in GetHashCode(). I’m not sure that’s the right solution, but I’d be interested to hear what others think.

Found bugs in Noda Time!

One of the things I was interested in finding out with all of this was how consistent Noda Time is in terms of its nullity handling. Until you have a tool like this, it’s hard to tell. I’m very pleased to say that most of it hangs together nicely – although so far that’s only the result of getting down to no warnings, rather than a file-by-file check through the code, which I suspect I’ll want to do eventually.

I did find two bugs, however. Noda Time tries to handle the case where TimeZoneInfo.Local returns a null reference, because we’ve seen that happen in the wild. (Hopefully that’s been fixed now in Mono, but even so it’s nice to know we can cope.) It turns out that we have code to cope with it in one place, but there are two places where we don’t… and the C# 8 tooling found that. Yay!

Found a bug in the preview!

To be clear, I didn’t expect the preview code to be perfect. As noted earlier, there are a few places I think it can be smarter. But I found a nasty bug that would hang Visual Studio and cause csc.exe to fail when building. It turns out that if you have a type parameter T with a constraint of T : class, IEquatable<T?>, that causes a stack overflow. I’ve reported the bug (now filed on GitHub thanks to diligent Microsoft folks) so hopefully it’ll be fixed long before the final version.

Admittedly the constraint is interesting in itself – it’s not necessarily clear what it means, if T is already a nullable reference type. I’ll let smarter people than myself work that out.

Conclusion

Well, that was a jolly exercise. My first impressions are:

  • We really need class library authors to embrace this as soon as C# 8 comes out, in order to make it as useful as possible early. Noda Time has no further dependencies, fortunately.
  • It didn’t take as long as I feared it might to do a first pass at annotating Noda Time, although I’m sure I missed some things while doing it.
  • A few bugs aside, the tooling is generally in very good shape; the warnings it produced were relevant and easy to understand.
  • It’s going to take me a while to get used to things like IList<string?>? for a nullable list of nullable strings.

Overall I’m very excited by all of this – I’m really looking forward to the final release. I suspect more blog posts will come over time…

44 thoughts on “First steps with nullable reference types”

  2. Pingback: First steps with nullable reference types - How to Code .NET
  3. Pingback: Newsy .NET 2018-04-22 – DevNation
  4. Pingback: The Morning Brew - Chris Alcock » The Morning Brew #2569

  5. On the IEqualityComparer issue: I think they’ll have to change the signature of the methods to the following to make the interface more explicit.

    public interface IEqualityComparer
    {
    bool Equals(T? x, T? y);
    int GetHashCode(T obj);
    }

    This way it indicates that the GetHashCode method expects a value, while the Equals method knows that the values can be null.

    Like

    Reply

        1. I think that when it comes to Generics, we will ultimately need an alternate syntax for indicating that T is nullable if T is a reference type, but not if it is a value type. I’d probably vote for a [CanBeNull] or [NullableReference] attribute. We’d have Equals([NullableReference] T x, T y) and GetHashCode(T obj).

          I suppose an alternative would be that if T was not restricted with a “class” or “struct” constraint, that the meaning of T? would only mean “nullable if T is a reference.” If T were constrained to “struct” it would explicitly mean Nullable. I’m not sure if this would follow or violate the “principal of least surprise.”

          Like

          Reply

            1. Kotlin interprets nullable primitives as the boxed versions. This works well on the JVM, but I suspect would be more complicated with proper collections of primitives.

              Like

              Reply

  6. Will this ripple throughout everything that touches IEqualityComparer<>, for instance Dictionary<,> and LINQ operators?
    Seems like just the tip of the iceberg. What is the impact like with IEnumerable<>, considering how pervasively it is used? I’ve been looking forward to this for a while and now, but now I’ve got more questions and I’m hesitant to dive in.

    Like

    Reply

    1. I don’t think there’s any problem with IEnumerable<T>. We’ll see with other types. It’s entirely possible that I’ve misunderstood the aims a little, or that there’s a better solution in the works.

      Like

      Reply

      1. Perhaps I’m missing something. Here’s why I asked: If IEnumerable means the something different than IEnumerable<T?> does — then by existing collection types implementing IEnumerable rather than IEnumerable<T?>, it seems like it could be problematic for them. In essence, I would expect that IEnumerable indicates that enumerators’ “Current”property would be T rather than T? (i.e., IEnumerator<T?> vs IEnumerator) — so they wouldn’t/couldn’t return null.

        Like

        Reply

        2. That’s fine in itself. I like the idea of “this is a sequence of string references, none of which will ever be null”. The reason IEqualityComparer is difficult is that it’s got a single type parameter that is handled differently in two different places.

          Like

          Reply

          1. “it’s got a single type parameter that is handled differently in two different places”

            Frankly, that’s a problem in design, regardless of the introduction of nullable reference types. I think that’s the concern here. We have HUGE amounts of code written and designed in a world without this concept, and that code won’t always make sense in a world with this concept. I want this so bad it hurts, but I’m not sure how trying to add it this late in the game will play out. Eventually we might be living in a great world, but I suspect we’re at least going to go through a rough period of time. And if it’s too rough, people may refuse to use it, which may be a self fulfilling problem. How many average .NET developers will read an article like this and realize how much work you had to do to “shut the compiler up”, and in the process you only found 2 bugs?

            Like

            Reply

            1. I view the fact that I only found two bugs as evidence that most of my hard work on Noda Time paid off – we already have a lot of annotations saying what could be null and what couldn’t.

              It’s not just about finding bugs though – it’s about communicating intent to developers. Previously they had to read my comments saying “The return value may be null” or “The return value will never be null” etc… whereas now they’ll be able to use tooling to help them.

              I agree that the equality part is at least somewhat a problem of design, but I don’t think that means the feature will have little value. I agree we may well have a rough period – just like we did in early days of .NET Core tooling – but for a better future. Yes, there would be less pain if this had been included right from the start, but I think it’s still going to be a net win. I also think it’s very hard to predict with any certainty before it gets rolled out more widely.

              I encourage other library suppliers to do the same kind of test that I did – the more pre-release feedback the team gets, the better.

              Like

              Reply

              1. Has the runtime changed in such a way to support this cannot-be-null behario that changes the reference-type/value-type distinction? In other words, is “string?” an alias in the same way that “int?” is an alias for Nullable?

                Liked by 1 person

            2. I was impressed it found 2 issues in code that is so well tested and reviewed. It would find a lot more issues in any code I was in the process of writing…..

              Like

              Reply

            3. I was very impressed it found two issues in such well tested and reviewed code. It clearly would find a lot more bugs in code that I am in the process of writing.

              Like

              Reply

  7. No, there’s no representation at execution time. There’s no distinction between “string” and “string?” beyond attributes added to the member, as far as I’m aware. In that sense it’s like tuple element names.

    Liked by 1 person

    Reply

  8. For the IEqualityComparer problem, have you tried implementing the interface both ways?
    (I don’t have the preview installed, so I can’t say if this works)

    struct PeriodComparer : IEqualityComparer, IEqualityComparer
    {
    public bool Equals(PeriodComparer x, PeriodComparer y)
    {
    return true;
    }

    public bool Equals(PeriodComparer ? x, PeriodComparer ? y)
{
    return true;
}

public int GetHashCode(PeriodComparer  obj)
{
    return 0;
}

[Obsolete("No nulls", true)]
public int GetHashCode(PeriodComparer ? obj)
{
    throw new NotImplementedException();
}

    }

    Like

    Reply
  11. Pingback: Szumma #107 – 2018 17. hét – ./d/fuel

  12. Jon, I’m intrigued by this:
    “The more repetitive part is fixing all the tests that ensure a method throws an ArgumentNullException if called with a null argument”
    Shouldn’t those methods take not-nullable arguments, and the tests be removed entirely, now that the compiler takes care of the warning?

    Liked by 1 person

    Reply

    1. The methods do take non-nullable parameters – but the method still needs to check that the argument isn’t null (to do the right thing if someone isn’t using C# 8 or ignores the warning). And if the precondition needs to be there, I need to test it..

      Like

      Reply

      2. A static checking tool may help, e.g. all public methods that can’t take NULL, but check and throw if NULL is passed it. It seems pointless having to write lots of test by hand for error checking that can be defined by rules…..

        Like

        Reply

      3. For a closed source project, developed by a small team, where everyone will be able to upgrade to C# 8 in the same day, and where the compiler is set to treat warnings as errors. Would you agree that nullchecks can be removed from the code for any non-nullable reference?

        Like

        Reply

        1. I’m not sure, to be honest. I suspect there will still be some cases that the compiler won’t be able to spot, and null checks can make it massively simpler to work out what’s going on when there’s a problem. I think I’d probably keep them – at least to start with.

          Like

          Reply

  13. Hey Jon, It is a really interesting article, I enjoyed it. It is always great to see you playing with new features.
    Anyway, I have to point out, there is a typo at the end of this sentence: ‘I can use the ?? operator with the C# 7 feature of throw expressions:’

    Like

    Reply

  14. Hi Jon. I recently created a prototype for an analyzer with codefixer that transforms Resharpers’ (Item)NotNull/CanBeNull annotations into C# 8 nullable reference format. May help on future experiments!

    To try it out, install the VSIX at https://ci.appveyor.com/project/bkoelman/resharpercodecontractnullability/build/1.0.9-pre-32-nullablerefs/artifacts. Then in the lightbulb-menu, select “Convert to C# syntax” on a squiggled span. Works document/project/solution-wide too.

    The source code can be found at https://github.com/bkoelman/ResharperCodeContractNullability/tree/nullablerefs (note alternative branch).

    Like

    Reply

  15. “Compare that with GetHashCode(), where it’s explicitly documented that the method should throw ArgumentNullException if obj is null.”

    I did a small double-take here, but I looked it up at the API doc page for IEqualityComparer[T].GetHashCode(T). It does indeed have an exceptions element implying this.

    For the EqualityComparer type family and for Object.GetHashCode, I’ve always used the recommendations at the doc page for Object.GetHashCode, under “Notes to Inheritors”, which states that overrides of that method “should not throw exceptions”. I took that advice to apply to any hash code-computing method one defines anywhere, including implementations among and for EqualityComparer etc. And I took it to mean, essentially, “if you can return a reasonable hash code for null, e.g. zero, if it makes sense for your type, you should go ahead and do so”.

    So I also looked up (in the reference source) a framework implementation of IEqualityComparer.GetHashCode(object), specifically StringComparer.GetHashCode(object), and it throws on null, but then I saw the explicit implementation of IEqualityComparer.GetHashCode(object) on EqualityComparer[T], and this one returns zero on null. Then my head exploded. Not really (phew), but the framework and its docs have certainly taken me for a spin these past few minutes.

    Like

    Reply

  16. For open source project, it would be very useful to have a tool that converts the C# 8 code with nullable types into C# 7 code, so that only one version of a liberty needs to be maintained. (Or a way to use attributes that are valid in C# 7 to drive a tool that generates the C# 8 code.)

    Likewise for closed source software where the C# 8 compiler is being tied out before all team members have switched to it.
    (Reminds me of the old days, wherein C code, macros were often used to allow type checking with newer compilers but still allow the code to complete on systems with K&R compilers.

    Like

    Reply
  17. Pingback: The best of C# and .NET in 2018 - Productive C# | Learn C# with Practical Video Tutorials

  18. I haven’t been able to find any info on what the default value of a non-nullable reference types is (what happens in array creation? what happens to default(T)?

    Are those now prohibited for non-nullables?

    Like

    Reply
  19. Pingback: Null No More | OzCode

Leave a comment