Let code know the nullability of its generic type parameter (part deux)

[betty-crokker]

I had started this thread in another issue (#3587) but since there were several proposals in that thread and much discussion, I thought it best to close that and start afresh with the result of that discussion.

The basic problem I am facing, is that ofttimes when one is writing generic classes with nullability enabled (C# NRT), a container that is defined as having a potentially non-nullable generic parameter requires the use of some other class that cannot make any nullability claims. For example, say I have a generic class named Holder that gets a value from PropertyInfo.GetValue:

public class Holder<T>
{
    public readonly T Value;
    public Holder(object source, PropertyInfo prop)
    {
        Value = (T)prop.GetValue(source);  // Compiler warning, possible null reference assignment
    }
}

Obviously if prop.GetValue() returns a value that is not of type T, you get a runtime typecast exception, so I'm going to ignore that case for the rest of this discussion.

If this class is instantiated as Holder<string?> and prop.GetValue() returns a string (which might be null) for the specified property, all is well (although it would be nice to get rid of the compiler warning). But with the exact same code, if the class is instantiated as Holder<string>, that should mean that Value will never be null but there's no way to guarantee that.

Notice the Holder class has absolutely no way to determine whether it should allow nulls or not. There are no NullableAttribute custom attributes on Holder, typeof(T) is string whether it's inside Holder<string> or Holder<string?>, and the custom attributes on the property are often (1) in someone else's code, and (2) it's often reasonable that the property return null.

One solution would be to add a "valueCanBeNull" parameter, like so:

public class Holder<T>
{
    public readonly T Value;
    public Holder(object source, PropertyInfo prop, bool valueCanBeNull)
    {
        T value = (T)prop.GetValue(source);
        if ((value == null) && (!valueCanBeNull))
            throw new Exception("Value can't be null");
       Value = value;
    }
}

But this parameter adds unnecessary complexity and is redundant with the type parameter T, and that means error-prone, and the whole point of the C# 8 NRT system is to reduce errors.

My proposal is to instead add an attribute to System.Runtime.CompilerServices that functions very similarly to the other attributes in that namespace, where a function can request access to information the compiler has:

public class Holder<T>
{
    public readonly T Value;
    public Holder(object source, PropertyInfo prop)
    {
        SetValue((T)prop.GetValue(source));
    }
    void SetValue(T value, [Nullability(T)] nullability = 0)
    {
        if ((value == null) && (!nullability.HasFlag(Nullability.AllowsNull)))
            throw new Exception("Value can't be null");
       Value = value;
    }
}

[jnm2]

I'm not sure why the SetValue method should be throwing an exception. Also, you could get around that exception by casting Holder<C> to Holder<C?> before calling SetValue.

In the PropertyInfo example, why not examine the PropertyInfo object to determine whether the property itself is nullable rather than being dependent on what the caller picked for T?

[betty-crokker]

You've got three questions, and each deserves a good answer, so I'm going to answer them individually.

My thought with my final code snippet was that if the class was instantiated as (for example) Holder<string>, the Nullability enum passed to SetValue would not contain the Nullability.AllowsNull enum, while if the class was instantiated as Holder<string?>, that enum would contain the Nullability.AllowsNull enum.

So if SetValue() does not see the Nullability.AllowsNull, it knows it was instantiated with its generic type parameter T as some non-nullable type (like Holder<string>).

If Holder has been told its value should never be null, but then is told to set its value to null, that's an unresolvable conflict for which throwing an exception is the appropriate response.

[betty-crokker]

You asked about "casting Holder<C> to Holder<C?> before calling SetValue".

There's a couple of problems with that - first of all, I don't think you can cast Holder<C> to Holder<C?>, I think the compiler would object. You could do something obnoxious like cast Holder<\C> to object, and then to Holder<C?>, but ...

This is the exact opposite of the desired behavior. When the consumer of this class constructs an object of type Holder<C>, that is the user saying "I never want Value to be null". The whole point of my proposal is figuring out a way to ensure that the consumer's wishes are met. It's not about finding a sneaky way to store a null into Value even though it's not supposed to be there.

[betty-crokker]

So could the consumer have been "smarter" and said "oh, look, property X can be null so I had better make my Holder<> accept a nullable type"? Yes and no ...

There are a lot of places in my code where I say "based on how I am writing this application, field X can never be null" and then I write my code around that assumption. I even design my NRT-enabled classes around that assumption (in this case, defining my object as Holder<C> instead of Holder<C?>).

One way this can happen is if there is a property that returns a nullable type, but because of how I'm using it, I never set it to null, so I instantiate my Holder<> using a non-nullable type. That's all fine and great, but of course bugs happen. If a null does somehow sneak in, I want to know as soon as possible instead of letting that null value escape and wreak who knows what damage.

[HaloFour]

This seems like a painfully narrow use case to justify a language change. You can accept nullability as a separate parameter to the constructor to this Holder<T> object or default it from metadata of the property itself.

[betty-crokker]

@HaloFour your final thought ("default [nullability] from metadata of the property itself") I have described several times why that's not acceptable in this case. I am specifically describing a situation where I am attempting to use the developer's knowledge (in this case, "I know that even though this property can be null, I'm not going to let it be null") to make a stricter Holder<> than the property.

But you raise a good point about a narrow use case. I agree, my example is quite narrow, but should not be taken to mean my proposed change is only useful in a tiny number of cases.

I've been thinking about "The Pit of Success" and Dictionary<>. I had not heard the phrase "The Pit of Success" until last week, but it has the same meaning as the phrase I use (which is not as catchy) which is "where possible, the easiest way should be the right way"

In the pre-NRT days, Dictionary<> disallowed null keys, but only at runtime. That means you can write this code:

void Test()
{
    Dictionary<string, string> dict = new Dictionary<string, string>();
    dict["ABC"] = "DEF";
    dict["BBB"] = null;
    dict[null] = "CCC";  // Line 6: ArgumentNullException

    int i = 0;
    foreach (var pair in dict)
    {
        if (pair.Key.Length == 3)
            i += pair.Value.Length;     // Line 12: NullReferenceException
    }
}

and get no compile-time warnings or errors. At runtime you get an ArgumentNullException thrown in line 6. So that's good, Dictionary<> is noticing the problem and throwing an exception that makes it easy for the developer to find and fix the problem. If you comment out that line you get a NullReferenceException in line 12 which is understandable, but can be quite difficult to fix because the actual problem (storing an unexpected null value) happened some time in the past and it can be very difficult to figure out where.

In the new C# 8 NRT world, I can enable #nullable and now I get compiler warnings in both lines 5 and 6 which is much better:

void Test()
{
    Dictionary<string, string> dict = new Dictionary<string, string>();
    dict["ABC"] = "DEF";
    dict["BBB"] = null;   // Line 5: Compiler warning
    dict[null] = "CCC";  // Line 6: Compiler warning, ArgumentNullException

    int i = 0;
    foreach (var pair in dict)
    {
        if (pair.Key.Length == 3)
            i += pair.Value.Length;     // Line 12: NullReferenceException
    }
}

Now in this simple case, I can see that I've made a mistake in line 5 and can correct it at compile-time, rather than waiting to get that run-time exception.

But in the case where the dictionary value is being set to a passed-in value (so the function setting the dictionary doesn't directly know whether the value is null or not), one solution is to put the onus on the developer - they should check all their values for null:

void Test()
{
    Dictionary<string, string> dict = new Dictionary<string, string>();
    SetValue(dict, "ABC", "DEF");
    SetValue(dict, "BBB", null);   // Compiler warning
    SetValue(dict, null, "CCC");  //  Compiler warning

    int i = 0;
    foreach (var pair in dict)
    {
        if (pair.Key == null)
            throw new Exception("Key is null");
        if (pair.Value == null)
            throw new Exception("Value is null");
        if (pair.Key.Length == 3)
            i += pair.Value.Length;     // Line 12: NullReferenceException
    }
    void SetValue<K, V>(Dictionary<K, V> dict, K key, V value) where K: notnull
    {
        if (k == null)
            throw new Exception("Can't set Key to null");
        if (v == null)
            throw new Exception("For this dictionary, can't set Value to null");
        dict[key] = value;
    }
}

But you can see that's getting a little crazy.

With my proposed attribute, we could have a very interesting discussion about using that attribute in Dictionary<> itself - if the 'TValue' type parameter was a non-nullable type, Dictionary<> could throw an exception (just like the current ArgumentNullException) at the point where the null is being stored (which makes fixing it SO much easier).

So rather than a narrow use case, this attribute has the potential to change how we debug an entire class of problems in one of the most commonly-used classes in the system.

[HaloFour]

@betty-crokker

What you are describing is a complete reversal of the core design tenets of NRTs, that they have zero impact on the semantic behavior of existing code:

#3587 (comment)

To be clear, this was an explicit goal and not something that "fell out". We very very very intentionally designed and spec'ed with the idea that NRT will not effect runtime behavior.

[betty-crokker]

@HaloFour - the change to Dictionary could definitely be a breaking change, which is why I mention it merely as a possibility of how my proposed change is not just a "narrow use case". My proposed new attribute does not reverse that core design tenet, as existing code won't reference the attribute and thus won't change at all.

[IS4Code]

Very nice idea! At first my intuition was that the nullability property should be something attached to a generic parameter, but the call site is actually the definite place where it is determined: Holder<C> and Holder<C?> are identical types for the runtime and so storing the information anywhere else than at the call site would cause inconsistencies.

However, I think a general solution should be put in place instead of just a check for nullability. C# currently has (to my knowledge) 3 kinds of custom types that share .NET types: dynamic, nullable reference types, and tuples, mapped respectively to object, normal reference types, and ValueTuple. As any new such type could be added in the future (think function pointers mapped to IntPtr, specific boxed value types mapped to ValueType etc.), it is a good time to think about ways to provide information about all of these additions to generic code, or we are getting closer to Java where generic code has no knowledge about its type arguments, and these are not just type annotations. Ultimately, the callee should be able to even differentiate between (string? a, dynamic b)[] and (string c, object? d)[].

Thus we need a compile-time type info object:

void SetValue(T value, [InfoFor(typeof(T))] ILanguageTypeInfo info = null)
{
    if (value == null && !info.PermitsNull) //PermitsNull is true for nullable value types, reference types with NRT off, and nullable reference types with NRT on
         throw new Exception("Value can't be null");
    Value = value;
}

The compiler would create an instance of ILanguageTypeInfo for every used compiler-time type, Such an instance could be similar to System.Type, but more complex types could be represented with other interfaces (i.e. ILanguageArrayTypeInfo, ILanguageGenericTypeInfo etc.). Language-specific types like CSharpTupleTypeInfo or CSharpDynamicTypeInfo, should also be exposed.

This sounds collectively more useful than just nullability information. Such a mechanism would enhance debugging, interaction with dynamic languages or other services and so on.

[jnm2]

@betty-crokker

If Holder has been told its value should never be null, but then is told to set its value to null, that's an unresolvable conflict for which throwing an exception is the appropriate response.

Why is throwing an exception better than the next best thing, whatever that is? I don't have a clear picture of why this is desirable yet.

There's a couple of problems with that - first of all, I don't think you can cast Holder to Holder<C?>, I think the compiler would object. You could do something obnoxious like cast Holder<\C> to object, and then to Holder<C?>, but ...

This is the exact opposite of the desired behavior. When the consumer of this class constructs an object of type Holder, that is the user saying "I never want Value to be null". The whole point of my proposal is figuring out a way to ensure that the consumer's wishes are met. It's not about finding a sneaky way to store a null into Value even though it's not supposed to be there.

I believe you can both explicitly and implicitly cast between Holder<C> and Holder<C?> as long as you use ! to tell the compiler that you know what you're doing. The ! operator is a common reality for BCL types that do things that is beyond the compiler's ability to reason about.

In reply to the rest, it seems like there's an element of matching the nullability to something external, such as a database. Automated tests are great for this kind of guarantee.

[betty-crokker]

1. Currently, the "next best thing" is: store a null in a field that the consumer of this class has specifically said should never be null. This will almost certainly cause a crash (null pointer exception) at some point in the future. The best way to handle a bug is to avoid it at compile time; the next best is to throw a relevant exception at the moment the bad data is created; the worst is to allow the bad data to exist and wait until some future point in time when the bad data causes some unknown error and you have no way to track it back to the actual fault.

2. I'm only starting to work with the C# NRT but my initial feeling is that the "null forgiving operator" is a terrible idea, and one that will bite people hard. My prediction is that the null forgiving operator will end up on the same shelf we now use to store "goto". I started out putting it in my code and then removed it everywhere I could. Here's my reasoning: if you have a field/property/function/whatever that makes a statement about nullability, and you have to tell the compiler "by the way, I lied about that item not being nullable", you are opening yourself up to lots of problems. As in life, say what you mean and mean what you say. If a field/property/function can be null, admit it up front and deal with it.

[jnm2]

Currently, the "next best thing" is: store a null in a field that the consumer of this class has specifically said should never be null. This will almost certainly cause a crash (null pointer exception) at some point in the future. The best way to handle a bug is to avoid it at compile time; the next best is to throw a relevant exception at the moment the bad data is created; the worst is to allow the bad data to exist and wait until some future point in time when the bad data causes some unknown error and you have no way to track it back to the actual fault.

We don't disagree that waiting time bombs are bad. What still isn't clear is how you'd get into a situation where it does much good to know at runtime whether the generic type argument was nullable. In the example above, it seems to me that the PropertyInfo should be single source of truth on this and that anything else is just introducing ways for it to go wrong.

I'm only starting to work with the C# NRT but my initial feeling is that the "null forgiving operator" is a terrible idea, and one that will bite people hard.

It's not an option to avoid this operator given the finite reasoning capacity of the compiler and specific limitations in NRT expressiveness. I can link you to a bunch of issues demonstrating these realities if you're interested. Often it has to do with timing/context or with generics.

[betty-crokker]

I was thinking more about your comment that "the PropertyInfo should be single source of truth on this" ... how can I distinguish between a property that returns a nullable string, and a property that returns a non-nullable string?

[jnm2]

@betty-crokker @canton7 answers this at https://stackoverflow.com/a/58454489. A System.Reflection API is being discussed: dotnet/runtime#29723

[roji]

FWIW the need to know the nullability of generic type parameters goes beyond the example discussed above. For example, Entity Framework Core uses a fluent API calls to allow users to define a "model" which then gets mapped to a database. For example, the following snippet defines a property called Test1 of type int in the model:

modelBuilder
    .Entity<MyEntity>()
    .Property<int>("Test1");

Whether int is specified or int? is captured in model building, and causes non-nullable or nullable columns to be created in the database. The same isn't possible with string and string?. Note that there is no CLR property or field in this particular scenario - no place to check for nullability attributes; the only thing defining the model property is the generic Property<T> method call (see #22352 where this was originally raised).

(to be fair, in EF Core it is possible to use an additional API call to manage the column nullability, and in general this scenario is atypical in that usually a CLR property does exist, which is inspected for nullability).

@CyrusNajmabadi regarding #3587 (comment), the ask here isn't for NRTs to affect runtime behavior, but to make nullability information accessible to code that wishes to inspect it. This doesn't seem any different from exposing nullability information on properties, fields and methods, which is what we already do (and which dotnet/runtime#29723 would expose in a nicer way).

@jnm2 I do not think that dotnet/runtime#29723 is relevant here, as that proposes to add reflection APIs to expose information that already exists and can be accessed as attributes. In other words, there is nothing that dotnet/runtime#29723 would add that you can't already do today (albeit in a complex way and duplicated everywhere). The issue here is that unlike with properties, methods, or fields, with generic type parameters the information simply does not exist.

[jnm2]

@roji It is, however, relevant to the specific question that it is answering: "how can I distinguish between a property that returns a nullable string, and a property that returns a non-nullable string?"

[roji]

@jnm2 yeah, that's true. Having written logic to parse nullable metadata, I'm definitely waiting for a 1st class reflection API to do that.