Add nullability annotations to DaprClient

[rynowak]

Describe the proposal

Once we ingest the .NET 5.0 SDK we we'll be able to add nullability annotations to most of the methods on DaprClient. The reason we didn't do this because is because prior to the C#9 compiler, a core use case didn't work with nullability:

public Task<TValue> DoSomething<TValue>();

There is no supported way (pre C# 9) to say this method returns a task that might return a null without also constraining TValue : class. We didn't want to block the use of value types at a fundamental level especially when records and structs are about to hit.

Now that this barrier has been removed there will be a benefit to adding NNRT annotations.

[dlemstra]

I gave this a try and changed almost everything in the Dapr.Client project. But DaprClientGrpc fails to compile. Details are in the image below. Not sure if this is a bug or that I should implement it differently because the method is abstract? Any ideas @rynowak?

Small reproducable test case that fails to compile:

using System;
using System.Collections.Generic;
using System.Text;
using System.Threading.Tasks;

#nullable enable

namespace Dapr.Client
{
    abstract class Class1
    {
        public abstract Task<T?> CompilationError<T>();
    }
    class Class2 : Class1
    {
        public override Task<T?> CompilationError<T>()
        {
            throw new NotImplementedException();
        }

        public Task<T?> ThisWorks<T>()
        {
            throw new NotImplementedException();
        }
    }
}

[rynowak]

Thanks @dlemstra - I would have expected this to work as well. I was able to track down the documentation for this. https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-9.0/unconstrained-type-parameter-annotations

I wonder if we need to use where T : default on the overrides. Other than the examples, I don't really have a good sense of when that is needed 🤓

If you are interested in taking this on, it might be best to do it in chunks and review each stage separately. Maybe start with something simple like bindings. I'd hate for you to take the trouble of changing everything before we agree on design.

[rynowak]

Note for myself in the future - when we do start rolling this out, we'll want make an announcement about how we're positioning it:

• We expect this to be a source breaking change for users with nullabilty enabled
• We expect that we will get some things wrong and will change over time
• We're not going to wait for 2.0 (next major) to add nullability to new areas because it's easy to work around an issue

[dlemstra]

The where T : default solution seemed to work @rynowak. I also created the first PR for this with one of the small libraries. Can you clarify what you mean with the bindings?

[philliphoff]

@halspang Do you think there's any interest in this still? I just ran into a bug in my own application as a result of a wayward null that would have been caught with proper annotations.

[frankbuckley]

@halspang / @philliphoff - any update on this? it would be helpful. would you accept a PR?