NullableAttribute and C# 8

Background: Noda Time and C# 8

Note: this blog post was written based on experimentation with Visual Studio 2019 preview 2.2. It’s possible that some of the details here will change over time.

C# 8 is nearly here. At least, it’s close enough to being “here” that there are preview builds of Visual Studio 2019 available that support it. Unsurprisingly, I’ve been playing with it quite a bit.

In particular, I’ve been porting the Noda Time source code to use the new C# 8 features. The master branch of the repo is currently the code for Noda Time 3.0, which won’t be shipping (as a GA release) until after C# 8 and Visual Studio 2019 have fully shipped, so it’s a safe environment in which to experiment.

While it’s possible that I’ll use other C# 8 features in the future, the two C# 8 features that impact Noda Time most are nullable reference types and switch expressions. Both sets of changes are merged into master now, but the pull requests are still available so you can see just the changes:

The switch expressions PR is much simpler than the nullable reference types one. It’s entirely an implementation detail… although admittedly one that confused docfx, requiring a few of those switch expressions to be backed out or moved in a later PR.

Nullable reference types are a much, much bigger deal. They affect the public API, so they need to be treated much more carefully, and the changes end up being spread far wide throughout the codebase. That’s why the switch expression PR is a single commit, whereas nullable reference types is split into 14 commits – mostly broken up by project.

Reviewing the public API of a nullable reference type change

So I’m now in a situation where I’ve got nullable reference type support in Noda Time. Anyone consuming the 3.0 build (and there’s an alpha available for experimentation purposes) from C# 8 will benefit from the extra information that can now be expressed about parameters and return values. Great!

But how can I be confident in the changes to the API? My process for making the change in the first place was to enable nullable reference types and see what warnings were created. That’s a great starting point, but it doesn’t necessarily catch everything. In particular, although I started with the main project (the one that creates NodaTime.dll), I found that I needed to make more changes later on, as I modified other projects.

Just because your code compiles without any warnings with nullable reference types enabled doesn’t mean it’s “correct” in terms of the API you want to expose.

For example, consider this method:

public static string Identity(string input) => input;

That’s entirely valid C# 7 code, and doesn’t require any changes to compile, warning-free, in C# 8 with nullable reference types enabled. But it may not be what you actually want to expose. I’d argue that it should look like one of these:

// Allowing null input, and producing nullable output
public static string? Identity(string? input) => input;

// Preventing null input, and producing non-nullable output
public static string Identity(string input)
{
    // Convenience method for nullity checking.
    Preconditions.CheckNotNull(input, nameof(input));
    return input;
}

If you were completely diligent when writing tests for the code before C# 8, it should be obvious which is required – because you’d presumably have something like:

[Test]
public void Identity_AcceptsNull()
{
    Assert.IsNull(Identity(null));
}

That test would have produced a warning in C# 8, and would have suggested that the null-permissive API is the one you wanted. But maybe you forgot to write that test. Maybe the test you would have written was one that would have shown up a need to put that precondition in. It’s entirely possible that you write much more comprehensive tests than I do, but I suspect most of us have some code that isn’t explicitly tested in terms of its null handling.

The important part take-away here is that even code that hasn’t changed in appearance can change meaning in C# 8… so you really need to review any public APIs. How do you do that? Well, you could review the entire public API surface you’re exposing, of course. For many libraries that would be the simplest approach to take, as a “belt and braces” attitude to review. For Noda Time that’s less appropriate, as so much of the API only deals in value types. While a full API review would no doubt be useful in itself, I just don’t have the time to do it right now.

Instead, what I want to review is any API element which is impacted by the C# 8 change – even if the code itself hasn’t changed. Fortunately, that’s relatively easy to do.

Enter NullableAttribute

The C# 8 compiler applies a new attribute to every API element which is affected by nullability. As an example of what I mean by this, consider the following code which uses the #nullable directive to control the nullable context of the code.

public class Test
{
#nullable enable
    public void X(string input) {}

    public void Y(string? input) {}
#nullable restore

#nullable disable
    public void Z(string input) {}
#nullable restore
}

The C# 8 compiler creates an internal NullableAttribute class within the assembly (which I assume it wouldn’t if we were targeting a framework that already includes such an attribute) and applies the attribute anywhere it’s relevant. So the above code compiles to the same IL as this:

using System.Runtime.CompilerServices;

public class Test
{
    public void X([Nullable((byte) 1)] string input) {}    

    public void Y([Nullable((byte) 2)] string input) {}

    public void Z(string input) {}}
}

Note how the parameter for Z doesn’t have the attribute at all, because that code is still oblivious to nullable reference types. But both X and Y have the attribute applied to their parameters – just with different arguments to describe the nullability. 1 is used for not-null; 2 is used for nullable.

That makes it relatively easy to write a tool to display every part of a library’s API that relates to nullable reference types – just find all the members that refer to NullableAttribute, and filter down to public and protected members. It’s slightly annoying that NullableAttribute doesn’t have any properties; code to analyze an assembly needs to find the appropriate CustomAttributeData and examine the constructor arguments. It’s awkward, but not insurmountable.

I’ve started doing exactly that in the Noda Time repository, and got it to the state where it’s fine for Noda Time’s API review. It’s a bit quick and dirty at the moment. It doesn’t show protected members, or setter-only properties, or handle arrays, and there are probably other things I’ve forgotten about. I intend to improve the code over time and probably move it to my Demo Code repository at some point, but I didn’t want to wait until then to write about NullableAttribute.

But hey, I’m all done, right? I’ve just explained how NullableAttribute works, so what’s left? Well, it’s not quite as simple as I’ve shown so far.

NullableAttribute in more complex scenarios

It would be oh-so-simple if each parameter or return type could just be nullable or non-nullable. But life gets more complicated than that, with both generics and arrays. Consider a method called GetNames() returning a list of strings. All of these are valid:

// Return value is non-null, and elements aren't null
List<string> GetNames()

// Return value is non-null, but elements may be null
List<string?> GetNames()

// Return value may be null, but elements aren't null
List<string>? GetNames()

// Return value may be null, and elements may be null
List<string?>? GetNames()

So how are those represented in IL? Well, NullableAttribute has one constructor accepting a single byte for simple situations, but another one accepting byte[] for more complex ones like this. Of course, List<string> is still relatively simple – it’s just a single top-level generic type with a single type argument. For a more complex example, imagine Dictionary<List<string?>, string[]?> . (A non-nullable reference to a dictionary where each key is a not-null list of nullable strings, and each value is a possibly-null array of non-nullable elements. Ouch.)

The layout of NullableAttribute in these cases can be thought of in terms of a pre-order traversal of a tree representing the type, where generic type arguments and array element types are leaves in the tree. The above example could be thought of as this tree:

         Dictionary<,> (not null)
            /               \
           /                 \
 List<> (not null)      Array (nullable)
        |                     |
        |                     |
 string (nullable)      string (not null)

The pre-order traversal of that tree gives us these values:

  • Not null (dictionary)
  • Not null (list)
  • Nullable (string)
  • Nullable (array)
  • Not null (string)

So a parameter declared with that type would be decorated like this:

[Nullable(new byte[] { 1, 1, 2, 2, 1 })]

But wait, there’s more!

NullableAttribute in simultaneously-complex-and-simple scenarios

The compiler has one more trick up its sleeve. When all the elements in the tree are “not null” or all elements in the tree are “nullable”, it simply uses the constructor with the single-byte parameter instead. So Dictionary<List<string>, string[]> would be decorated with Nullable[(byte) 1] and Dictionary<List<string?>?, string?[]?>? would be decorated with Nullable[(byte) 2].

(Admittedly, Dictionary<,> doesn’t permit null keys anyway, but that’s an implementation detail.)

Conclusion

The C# 8 feature of nullable reference types is a really complicated one. I don’t think we’ve seen anything like this since async/await. This post has just touched on one interesting implementation detail. I’m sure there’ll be more posts on nullability over the next few months…

Farewell, Daisy Shipton

This is more of a quick, explanatory “heads-up” post than anything else.

On March 31st 2018, I started an experiment: I created a new Stack Overflow user called “Daisy Shipton” with no picture and a profile that just read “Love coding in C#” (or similar). I wanted to see how a new user presenting with a traditionally-female name would be treated, while posting the same content that I normally would. This experiment was only a small part of my thinking around the culture of Stack Overflow, and I expect to write more on that subject, touching on the experience of “Daisy”, at another time.

I let a few people in on the secret as I went along – people who I fully expected to recognize my writing style fairly quickly. A single person emailed me to ask whether Daisy and I were the same person – well done to them for spotting it. (Once someone had the idea, the evidence was pretty compelling – the “Jon Skeet” account went into a decline in posting answers at the same time that the “Daisy Shipton” account was created, and Daisy just happened to post about C#, Noda Time, Protocol Buffers, time zones and Google Cloud Platform client libraries for .NET. I really wasn’t trying to cover my tracks.)

As Daisy reached a rep of about 12,000 points, there became little point in continuing the experiment, so I asked for “her” account to be merged into my regular one. So if you see comments on my posts referring to @DaisyShipton, that’s why.

There’s one aspect of experimentation that never happened: Daisy never asked a question. Next time I want to ask a question on Stack Overflow, I’ll probably create another account to see how a question I think is good is received when posted from a 1-rep account.

It’s been fun, but it’ll also be nice to only have one account to manage now…

First steps with nullable reference types

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…

Backward compatibility and overloading

I started writing a blog post about versioning in July 2017. I’ve mostly abandoned it, because I think the topic is too vast for a single post. It potentially needs a whole site/wiki/repository devoted to it. I hope to come back to it at some point, because I believe this is a hugely important topic that doesn’t get as much attention as it deserves.

In particular, the .NET ecosystem is mostly embracing semantic versioning – which sounds great, but does rely on us having a common understanding of what’s meant by a “breaking change”. That’s something I’ve been thinking about quite a lot. One aspect which has struck me forcefully recently is how hard it is to avoid breaking changes when using method overloading. That’s what this post is about, mostly because it’s fun.

First, a quick definition…

Source and binary compatibility

If I can recompile my client code with a new version of the library and it all works fine, that’s source compatible. If I can redeploy my existing client binary with a new version of the library without recompiling, that’s binary compatible. Neither of these is a superset of the other:

  • Some changes are both source and binary incompatible, such as removing a whole public type that you depended on.
  • Some changes are source compatible but binary incompatible, such as changing a public static read-only field into a property.
  • Some changes are binary compatible but source incompatible, such as adding an overload which could cause compile-time ambiguity.
  • Some changes are source and binary compatible, such as reimplementing the body of a method.

So what are we talking about?

I’m going to assume that we have a public library at version 1.0, and we wish to add some overloads in version 1.1. We’re following semantic versioning, so we need to be backward compatible. What does that mean we can and can’t do, and is it a simple binary choice?

In various cases, I’ll present library code at version 1.0 and version 1.1, then “client” code (i.e. code that is using the library) which could be broken by the change. I’m not presenting method bodies or class declarations, as they’re largely irrelevant – focus on the signatures. It should be easy to reproduce any of this if you’re interested though. We’ll imagine that all the methods I present are in a class called Library.

Simplest conceivable change, foiled by method group conversions

The simplest example I can imagine would be adding a parameterized method when there’s a parameterless one already:

// Library version 1.0
public void Foo()

// Library version 1.1
public void Foo()
public void Foo(int x)

Even that’s not completely compatible. Consider this client code:

// Client
static void Method()
{
    var library = new Library();
    HandleAction(library.Foo);
}

static void HandleAction(Action action) {}
static void HandleAction(Action<int> action) {}

In library version 1.0, that’s fine. The call to HandleAction performs a method group conversion of library.Foo to create an Action. In library version 1.1, it’s ambiguous: the method group can be converted to either Action or Action<int>. So it’s not source compatible, if we’re going to be strict about it.

At this point you might be tempted to give up and go home, resolving never to add any overloads, ever again. Or maybe we can say that this is enough of a corner case to not consider it breaking. Let’s call method group conversions out of scope for now.

Unrelated reference types

We get into a different kind of territory when we have overloads with the same number of parameters. You might expect this library change to be non-breaking:

// Library version 1.0
public void Foo(string x)

// Library version 1.1
public void Foo(string x)
public void Foo(FileStream x)

That feels like it should be reasonable. The original method still exists, so we won’t be breaking binary compatibility. The simplest way of breaking source compatibility is to have a call that either works in v1.0 but doesn’t in v1.1, or works in both but does something different in v1.1 than it did in v1.0.

How can a call break between v1.0 and v1.1? We’d have to have an argument that’s compatible with both string and FileStream. But they’re unrelated reference types…

The first failure is if we have a user-defined implicit conversion to both string and FileStream:

// Client
class OddlyConvertible
{
    public static implicit operator string(OddlyConvertible c) => null;
    public static implicit operator FileStream(OddlyConvertible c) => null;
}

static void Method()
{
    var library = new Library();
    var convertible = new OddlyConvertible();
    library.Foo(convertible);
}

Hopefully the problem is obvious: what used to be unambiguous via a conversion to string is now ambiguous as the OddlyConvertible type can be implicitly converted to both string and FileStream. (Both overloads are applicable, neither is better than the other.)

It may be reasonable to exclude user-defined conversions… but there’s a far simpler way of making this fail:

// Client
static void Method()
{
    var library = new Library();
    library.Foo(null);
}

The null literal is implicitly convertible to any reference type or any nullable value type… so again, the call becomes ambiguous in the library v1.1. Let’s try again…

Reference type and non-nullable value type parameters

If we don’t mind user-defined conversions, but don’t like null literals causing a problem, how about introducing an overload with a non-nullable value type?

// Library version 1.0
public void Foo(string x)

// Library version 1.1
public void Foo(string x)
public void Foo(int x)

This looks good – library.Foo(null) will be fine in v1.1. So is it safe? Not in C# 7.1…

// Client
static void Method()
{
    var library = new Library();
    library.Foo(default);
}

The default literal is like the null literal, but for any type. It’s really useful – and a complete pain when it comes to overloading and compatibility :(

Optional parameters

Optional parameters bring their own kind of pain. Suppose we have one optional parameter, but wish to add a second. We have three options, shown as 1.1a, 1.1b and 1.1c below.

// Library version 1.0
public void Foo(string x = "")

// Library version 1.1a
// Keep the existing method, but add another one with two optional parameters.
public void Foo(string x = "")
public void Foo(string x = "", string y = "")

// Library version 1.1b
// Just add the parameter to the existing method.
public void Foo(string x = "", string y = "")

// Library version 1.1c
// Keep the old method but make the parameter required, and add a new method
// with both parameters optional.
public void Foo(string x)
public void Foo(string x = "", string y = "")

Let’s think about a client that makes two calls:

// Client
static void Method()
{
    var library = new Library();
    library.Foo();
    library.Foo("xyz");
}

Library 1.1a keeps binary compatiblity, but breaks source compatibility: the library.Foo() is now ambiguous. The C# overloading rules prefer a method that doesn’t need the compiler to “fill in” any optional parameters, but it doesn’t have any preference in terms of how many optional parameters are filled in.

Library 1.1b keeps source compatibility, but breaks binary compatibility. Existing compiled code will expect to call a method with a single parameter – and that method no longer exists.

Library 1.1c keeps binary compatibility, but is potentially odd around source compatibility. The library.Foo() call now resolves to the two-parameter method, whereas library.Foo("xyz") resolves to the one-parameter method (which the compiler prefers over the two-parameter method because it doesn’t need to fill in any optional parameters). That may very well be okay, if the one-parameter version simply delegates to the two-parameter version using the same default value. It feels odd for the meaning of the first call to change though, when the method it used to resolve to still exists.

Optional parameters get even hairer when you don’t want to add a new one at the end, but in the middle – e.g. if you’re trying to follow a convention of keeping an optional CancellationToken parameter at the end. I’m not going to dive into this…

Generics

Type inference is a tricky beast at the best of times. With overload resolution it goes into full-on nightmare mode.

Let’s have a single non-generic method in v1.0, and then add a generic method in v1.1.

// Library version 1.0
public void Foo(object x)

// Library version 1.1
public void Foo(object x)
public void Foo<T>(T x)

That doesn’t seem too awful… but let’s look closely at what happens to client code:

// Client
static void Method()
{
    var library = new Library();
    library.Foo(new object());
    library.Foo("xyz");
}

In library v1.0, both calls resolve to Foo(object) – the only method that exists.

Library v1.1 is backward-compatible: if we use a client executable compiled against v1.0 but running against v1.1, both calls will still use Foo(object). But if we recompile, the second call (and only the second one) will change to using the generic method. Both methods are applicable for both calls.

In the first call, T would be inferred to be object, so the argument-to-parameter-type conversion is just object to object in both cases. Great. The compiler applies a tie-break rule that prefers non-generic methods over generic methods.

In the second call, T would be inferred to be string, so the argument-to-parameter-type conversion is string to object for the original method and string to string for the generic method. The latter is a “better” conversion, so the second method is picked.

If the two methods behave the same way, that’s fine. If they don’t, you’ve broken compatibility in a very subtle way.

Inheritance and dynamic typing

I’m sorry: I just don’t have the energy. Both inheritance and dynamic typing would interact with overload resolution in “fun” and obscure ways.

If you add a method in one level of the inheritance hierarchy which overloads a method in a base class, the new method will be examined first, and picked over the base class method even when the base class method is more specific in terms of argument-to-parameter-type conversions. There’s lots of scope for messing things up.

Likewise with dynamic typing (within the client code), to some extent all bets are off. You’re already sacrificing a lot of compile-time safety… it shouldn’t come as a surprise when things break.

Conclusion

I’ve tried to keep the examples reasonably simple here. It can get really complicated really quickly as soon as you have multiple optional parameters etc.

Versioning is hard and makes my head hurt.

Stack Overflow Culture

This blog post was most directly provoked by this tweet from my friend Rob Conery, explaining why he’s giving up contributing on Stack Overflow.

However, it’s been a long time coming. A while ago I started writing a similar post, but it got longer and longer without coming to any conclusion. I’m writing this one with a timebox of one hour, and then I’ll post whatever I’ve got. (I may then reformat it later.)

I’m aware of the mixed feelings many people have about Stack Overflow. Some consider it to be completely worthless, but I think more people view it as “a valuable resource, but a scary place to contribute due to potential hostility.” Others contribute on a regular basis, occasionally experiencing or witnessing hostility, but generally having a reasonable time.

This post talks about my experiences and my thoughts on where Stack Overflow has a problem, where I disagree with some of the perceived problems, and what can be done to improve the situation. This is a topic I wish I’d had time to talk about in more detail with the Stack Overflow team when I visited them in New York in February, but we were too busy discussing other important issues.

For a lot of this post I’ll talk about “askers” and “answerers”. This is a deliberate simplification for the sake of, well, simplicity. Many users are both askers and answerers, and a lot of the time I’ll write comments with a view to being an answerer, but without necessarily ending up writing an answer. Although any given user may take on different roles even in the course of an hour, for a single post each person usually has a single role. There are other roles of course “commenter on someone else’s answer” for example – I’m not trying to be exhaustive here.

Differences in goals and expectations

Like most things in life, Stack Overflow works best when everyone has the same goal. We can all take steps towards that goal together. Conversely, when people in a single situation have different goals, that’s when trouble often starts.

On Stack Overflow, the most common disconnect is between these two goals:

  • Asker: minimize the time before I’m unblocked on the problem I’m facing
  • Answerer: maximize the value to the site of any given post, treating the site as a long-lasting resource

In my case, I have often have a sub-goal of “try to help improve the diagnostic skill of software engineers so that they’re in a better position to solve their own problems.”

As an example, consider this question – invented, but not far-fetched:

Random keeps giving me the same numbers. Is it broken?

This is a low-quality question, in my view. (I’ll talk more about that later.) I know what the problem is likely to be, but to work towards my goal I want the asker to improve the question – I want to see their code, the results etc. If I’m right about the problem (creating multiple instances of System.Random in quick succession, which will also use the same system-time-based seed), I’d then almost certainly be able to close the question as a duplicate, and it could potentially be deleted. In its current form, it provides no benefit to the site. I don’t want to close the question as a duplicate without seeing that it really is a duplicate though.

Now from the asker’s perspective, none of that is important. If they know that I have an idea what the problem might be, their perspective is probably that I should just tell them so they can be unblocked. Why take another 10 minutes to reproduce the problem in a good question, if I can just give them the answer now? Worse, if they do take the time to do that and then I promptly close their question as a duplicate, it feels like wasted time.

Now if I ignored emotions, I’d argue that the time wasn’t wasted:

  • The asker learned that when they ask a clearer question, they get to their answer more quickly. (Assuming they follow the link to the duplicate and apply it.)
  • The asker learned that it’s worth searching for duplicate questions in their research phase, as that may mean they don’t need to ask the question at all.

But ignoring emotions is a really bad idea, because we’re all human. What may well happen in that situation – even if I’ve been polite throughout – is that the asker will decide that Stack Overflow is full of “traffic cop” moderators who only care about wielding power. I could certainly argue that that’s unfair – perhaps highlighting my actual goals – but that may not change anyone’s mind.

So that’s one problem. How does the Stack Overflow community agree what the goal of site is, and then make that clearer to users when they ask a question? It’s worth noting that the tour page (which curiously doesn’t seem to be linked from the front page of the site any more) does include this text:

With your help, we’re working together to build a library of detailed answers to every question about programming.

I tend to put it slightly differently:

The goal of Stack Overflow is to create a repository of high-quality questions, and high-quality answers to those questions.

Is that actually a shared vision? If askers were aware of it, would that help? I’d like to hope so, although I doubt that it would completely stop all problems. (I don’t think anything would. The world isn’t a perfect place.)

Let’s move onto another topic where I disagree with some people: low-quality questions.

Yes, there are low-quality questions

I assert that even if it can’t be measured in a totally objective manner, there are high-quality questions and low-quality questions (and lots in between).

I view a high-quality question in the context of Stack Overflow as one which:

  • Asks a question, and is clear in what it’s asking for. It should be reasonably obvious whether any given attempted answer does answer the question. (That’s separate to whether the answer is correct.)
  • Avoids irrelevancies. This can be hard, but I view it as part of due diligence: if you’re encountering a problem as part of writing a web-app, you should at least try to determine whether the context of a web-app is relevant to the problem.
  • Is potentially useful to other people. This is where avoiding irrelevant aspects is important. Lots of people need to parse strings as dates; relatively few will need to parse strings as dates using framework X version Y in conjunction with a client written in COBOL, over a custom and proprietary network protocol.
  • Explains what the asker has already tried or researched, and where they’ve become stuck.
  • Where appropriate (which is often the case) contains a minimal example demonstrating the problem.
  • Is formatted appropriately. No whole-page paragraphs, no code that’s not formatted as code, etc.

There are lots of questions which meet all those requirements, or at least most of them.

I think it’s reasonable to assert that such a question is of higher quality than a question which literally consists of a link to a photo of a homework assignment, and that’s it. Yes, I’ve seen questions like that. They’re not often quite that bad, but if we really can’t agree that that is a low-quality question, I don’t know what we can agree on.

Of course, there’s a huge spectrum in between – but I think it’s important to accept that there are such things as low-quality questions, or at least to debate it and find out where we disagree.

Experience helps write good questions, but isn’t absolutely required

I’ve seen a lot of Meta posts complaining that Stack Overflow is too hard on newcomers, who can’t be expected to write a good question.

I would suggest that a newcomer who accepts the premise of the site and is willing to put in effort is likely to be able to come up with at least a reasonable question. It may take them longer to perform the research and write the question, and the question may well not be as crisp as one written by a more experienced developer in the same situation, but I believe that on the whole, newcomers are capable of writing questions of sufficient quality for Stack Overflow. They may not be aware of what they need to do or why, but that’s a problem with a different solution than just “we should answer awful questions which show no effort because the asker may be new to tech”.

One slightly separate issue is whether people have the diagnostic skills required to write genuinely good questions. This is a topic dear to my heart, and I really wish I had a good solution, but I don’t. I firmly believe that if we can help programmers become better at diagnostics, then that will be of huge benefit to them well beyond asking better Stack Overflow questions.

Some regular users behave like jerks on Stack Overflow, but most don’t

I’m certainly not going to claim that the Stack Overflow community is perfect. I have seen people being rude to people asking bad questions – and I’m not going to excuse that. If you catch me being rude, call me out on it. I don’t believe that requesting improvements to a question is rude in and of itself though. It can be done nicely, or it can be done meanly. I’m all for raising the level of civility on Stack Overflow, but I don’t think that has to be done at the expense of site quality.

I’d also say that I’ve experienced plenty of askers who react very rudely to being asked for more information. It’s far from one way traffic. I think I’ve probably seen more rudeness in this direction than from answerers, in fact – although the questions usually end up being closed and deleted, so anyone just browsing the site casually is unlikely to see that.

My timebox is rapidly diminishing, so let me get to the most important point. We need to be nicer to each other.

Jon’s Stack Overflow Covenant

I’ve deliberately called this my covenant, because it’s not my place to try to impose it on anyone else. If you think it’s something you could get behind (maybe with modifications), that’s great. If Stack Overflow decides to adopt it somewhere in the site guidelines, they’re very welcome to take it and change it however they see fit.

Essentially, I see many questions as a sort of transaction between askers and answerers. As such, it makes sense to have a kind of contract – but that sounds more like business, so I’d prefer to think of a covenant of good faith.

As an answerer, I will…

  • Not be a jerk.
  • Remember that the person I’m responding to is a human being, with feelings.
  • Assume that the person I’m responding to is acting in good faith and wants to be helped.
  • Be clear that a comment on the quality of a question is not a value judgement on the person asking it.
  • Remember that sometimes, the person I’m responding to may feel they’re being judged, even if I don’t think I’m doing that.
  • Be clear in my comments about how a question can be improved, giving concrete suggestions for positive changes rather than emphasizing the negative aspects of the current state.
  • Be clear in my answers, remembering that not everyone has the same technical context that I do (so some terms may need links etc).
  • Take the time to present my answer well, formatting it as readably as I can.

As an asker, I will…

  • Not be a jerk.
  • Remember that anyone who responds to me is a human being, with feelings.
  • Assume that any person who responds to me is acting in good faith and trying to help me.
  • Remember that I’m asking people to give up their time, for free, to help me with a problem.
  • Respect the time of others by researching my question before asking, narrowing it down as far as I can, and then presenting as much information as I think may be relevant.
  • Take the time to present my question well, formatting it as readably as I can.

I hope that most of the time, I’ve already been following that. Sometimes I suspect I’ve fallen down. Hopefully by writing it out explicitly, and then reading it, I’ll become a better community member.

I think if everyone fully took something like this on board before posting anything on Stack Overflow, we’d be in a better place.

Implementing IXmlSerializable in readonly structs

Background

There are three things you need to know to start with:

Operations on read-only variables which are value types copy the variable value first. I’ve written about this before on this blog. C# 7.2 addresses this by introducing the readonly modifier for structs. See the language proposal for more details. I was touched to see that the proposal references my blog post :)

The ref readonly local variables and in parameter features in C# 7.2 mean that “read-only variables” are likely to be more common in C# than they have been in the past.

Noda Time includes many value types which implement IXmlSerializable. Noda Time implements IXmlSerializable.ReadXml by assigning to this: fundamentally IXmlSerializable assumes a mutable type. I use explicit interface implementation to make this less likely to be used directly on an unboxed variable. With a generic method using an interface constraint you can observe a simple method call mutating a non-read-only variable, but that’s generally harmless.

Adding the readonly modifier to Noda Time structs

I relished news of the readonly modifier for struct declarations. At last, I can remove my ReadWriteForEfficiency attribute! Hmm. Not so much. To be fair, some of the structs (7 out of 18) were fine. But every struct that implements IXmlSerializable gave me this error:

Cannot assign to ‘this’ because it is read-only

That’s reasonable: in members of a readonly struct, this effectively becomes an in parameter instead of a ref parameter. But how can we fix that? Like any sensible developer, I turned to Stack Overflow, which has precisely the question I’m interested in. It even has an answer! Unfortunately, it amounts to a workaround: assigning to this via unsafe code.

Violating readonly with unsafe code

To give a concrete example of the answer from Stack Overflow, here’s my current LocalTime code:

void IXmlSerializable.ReadXml([NotNull] XmlReader reader)
{
    Preconditions.CheckNotNull(reader, nameof(reader));
    var pattern = LocalTimePattern.ExtendedIso;
    string text = reader.ReadElementContentAsString();
    this = pattern.Parse(text).Value;
}

Here’s the code that compiles when LocalTime is marked as readonly, after enabling unsafe blocks:

unsafe void IXmlSerializable.ReadXml([NotNull] XmlReader reader)
{
    Preconditions.CheckNotNull(reader, nameof(reader));
    var pattern = LocalTimePattern.ExtendedIso;
    string text = reader.ReadElementContentAsString();
    fixed (LocalTime* thisAddr = &this)
    {
        *thisAddr = pattern.Parse(text).Value;
    }
}

Essentially, the unsafe code is bypassing the controls around read-only structs. Just for kicks, let’s apply the same change
throughout Noda Time, and think about what would happen…

As it happens, that fix doesn’t work universally: ZonedDateTime is a “managed type” because it contains a reference (to a DateTimeZone) which means you can’t create a pointer to it. That’s a pity, but if we can make everything else readonly, that’s a good start. Now let’s look at the knock-on effects…

Safe code trying to abuse the unsafe code

Let’s try to abuse our “slightly dodgy” implementations. Here’s a class we’ll put in a “friendly” assembly which is trying to be as helpful as possible:

using NodaTime;

public class AccessToken
{
    private readonly Instant expiry;
    public ref readonly Instant Expiry => ref expiry;

    public AccessToken(Instant expiry) => this.expiry = expiry;
}

Great – it lets you get at the expiry time of an access token without even copying the value.

The big test is: can we break this friendly’s code’s assumptions about expiry really not changing its value?

Here’s code I expected to mutate the access token:

static void MutateAccessToken(AccessToken accessToken)
{
    ref readonly Instant expiry = ref accessToken.Expiry;
    string xml = "<evil>2100-01-01T00:00:00Z</evil>";
    EvilReadXml(in expiry, xml);
}

static void EvilReadXml<T>(in T value, string xml) where T : IXmlSerializable
{
    var reader = XmlReader.Create(new StringReader(xml));
    reader.Read();
    value.ReadXml(reader);
}

We have an in parameter in EvilReadXml, so the expiry variable is being passed by reference, and then we’re calling ReadXml on that parameter… so doesn’t that mean we’ll modify the parameter, and thus the underlying expiry field in the object?

Nope. Thinking about it, the compiler doesn’t know when it compiles EvilReadXml that T is a readonly struct – it could be a regular struct. So it has to create a copy of the value before calling ReadXml.

Looking the the spec proposal, there’s one interesting point in the section on ref extension methods:

However any use of an in T parameter will have to be done through an interface member. Since all interface members are considered mutating, any such use would require a copy.

Hooray! That suggests it’s safe – at least for now. I’m still worried though: what if the C# team adds a readonly struct generic constraint in C# 8? Would that allow a small modification of the above code to break? And if interface methods are considered mutating anyway, why doesn’t the language know that when I’m trying to implement the interface?

But hey, now that we know unsafe code is able to work around this in our XML implementation, what’s to stop nasty code from using the same trick directly?

Unsafe code abusing safe code

Imagine we didn’t support XML serialization at all. Could unsafe code mutate AccessToken? It turns out it can, very easily:

static void MutateAccessToken(AccessToken accessToken)
{
    ref readonly Instant expiry = ref accessToken.Expiry;
    unsafe
    {
        fixed (Instant* expiryAddr = &expiry)
        {
            *expiryAddr = Instant.FromUtc(2100, 1, 1, 0, 0);
        }
    }
}

This isn’t too surprising – unsafe code is, after all, unsafe. I readily admit I’m not an expert on .NET security, and I know the
landscape has changed quite a bit over time. These days I believe the idea of a sandbox has been somewhat abandoned – if you care about executing code you don’t really trust, you do it in a container and use that as your security boundary. That’s about the limit of my knowledge though, which could be incorrect anyway.

Where do we go from here?

At this point, I’m stuck making a choice between several unpleasant ones:

  • Leave Noda Time public structs as non-read-only. That prevents users from writing efficient code to use them without copying.
  • Remove XML serialization from Noda Time 3.0. We’re probably going to remove binary serialization anyway on the grounds that Microsoft is somewhat-discouraging it going forward, and it’s generally considered a security problem. However, I don’t think XML serialization is considered to be as bad, so long as you use it carefully, preventing arbitrary type loading. (See this paper for more information.)
  • Implement XML serialization using unsafe code as shown above. Even though my attempt at abusing it failed, that doesn’t provide very much comfort. I don’t know what other ramifications there might be for including unsafe code. Does that limit where the code can be deployed? It also doesn’t help with my concern about a future readonly struct constraint.

Thoughts very welcome. Reassurances from the C# team that “readonly struct” constraints won’t happen particularly welcome… along with alternative ways of implementing XML serialization.

NuGet package statistics

For a while, I’ve been considering how useful nuget.org statistics are.

I know there have been issues in the past around accuracy, but that’s not what I’m thinking about. I’ve been
trying to work out what the numbers mean at all and whether that’s useful.

I’ve pretty sure an older version of the nuget.org gallery gave stats on a per-operation basis, but right now it looks like we can break down the downloads by package version, client name and client version. (NodaTime example)

In a way, the lack of NuGet “operation” at least makes it simpler to talk about: we only know about “downloads”. So, what counts as a download?

What’s a download?

Here are a few things that might increment that counter:

  • Manual download from the web page
  • Adding a new package in Visual Studio
  • Adding a new package in Visual Studio Code
  • nuget install from the command line
  • dotnet restore for a project locally
  • dotnet restore in a Continuous Integration system testing a PR
  • dotnet restore in a CI system testing a merged PR

All of them sound plausible, but it’s also possible that they wouldn’t increment the counter:

  • I might have a package in my NuGet cache locally
  • A CI system might have its own global package cache
  • A CI system might use a mirror service somehow

So what does the number really mean? Some set of coincidences in terms of developer behavior and project lifetime? One natural reaction to this is “The precise meaning of the number doesn’t matter, but bigger is better.” I’d suggest that’s overly complacent.

Suppose I’m right that some CI systems have a package cache, but others don’t. Suppose we look at packages X and Y which have download numbers of 1000 and 100,000 respectively. (Let’s ignore
which versions those are for, or how long those versions have been out.) Does that mean Y‘s usage is “better” than X‘s in some way? Not necessarily. Maybe it means there’s a single actively-developed
open source project using Y and a CI system that doesn’t have a NuGet cache (and configured to build each PR on each revision), whereas maybe there are a thousand entirely separate projects using
X, but all using a CI system that just serves up a single version from a cache for everything.

Of course, that’s an extreme position. It’s reasonable to suggest that on average, if package Y has larger download numbers than package X, then it’s likely to be more widely used… but can we
do better?

What are we trying to measure?

Imagine we had perfect information: a view into every machine on the planet, and every operation any of them performed. What number would we want to report? What does it mean for a package to be “popular” or “widely used”?

Maybe we should think in terms of “number of projects that use package X“. Let’s consider some situations:

  • A project created to investigate a problem, and then gets deleted. Never even committed to source control system.
  • A project which is created and committed to source control, but never used.
  • A project created and in production use, maintained by 1 person.
  • A project created and in production use, maintained by a team of
    100 people.
  • A project created by 1 person, but then forked by 10 people and
    never merged.
  • A project created on github by 1 person, and forked by 10 people on github, with them repeatedly creating branches and merging back into the original repo.
  • A project which doesn’t use package X directly, but uses package Y that depends on package X.

If those all happened for the same package, what number would you want each of those projects to contribute to the package usage?

One first-order approximation could be achieved with “take some hash of the name of the project and propagate it (even past caches) when installing a package”. That would allow us to be reasonably confident in some measure of “how many differently-named projects depend on package X” which might at least feel slightly more reasonable, although it’s unclear to me how throwaway projects would end up being represented. (Do people tend to use the same names as each other for throwaway projects? I bet Console1 and WindowsForms1 would be pretty popular…)

That isn’t a serious suggestion, by the way – it’s not clear to me that hashing alone provides sufficient privacy protection, for a start. There are multiple further issues in terms of cache-busting, too. It’s an interesting thought experiment.

What do I actually care about though?

That’s even assuming that “number of projects that use package X is a useful measure. It’s not clear to me that it is.

As an open source contributor, there are two aspects I care about:

  • How many people will I upset, and how badly, if I break something?
  • How many people will I delight, and to what extent, if I implement a particular new feature?

It’s not clear to me that any number is going to answer those questions for me.

So what do you care about? What would you want nuget.org to show if it could? What do you think would be reasonable for it to show in the real world with real world constraints?