Nullable Reference Type annotations

[edwardneal]

Is your feature request related to a problem? Please describe.

C# 8.0 introduced Nullable Reference Types. Since .NET Core 3.0, the runtime has been annotating its assemblies - this was tracked for .NET 5.0 with dotnet/runtime#2339, and from .NET 6.0 onwards with dotnet/runtime#41720.

The SqlClient driver doesn't have Nullable Reference Type annotations. The prevents upstream assemblies from performing a complete set of null-safety checks. It also means that the compiler support for null-safety checks within these projects is switched off, increasing the risk of unexpected NullReferenceExceptions.

Describe the solution you'd like

I'd like to see the SqlClient assemblies annotated in such a way to enable NRT support. This will enable those two cases: the compiler will be able to use static flow analysis to perform null-safety checks within the project, and for libraries which reference SqlClient.

Additional context

There are several code analysis attributes which only exist in .NET Core. We might need to replicate the API surface of these attributes in the same way that Newtonsoft.Json does (link.)

I'd propose that we copy the .NET Runtime approach of working upwards from base dependencies. Most of these dependencies were simple enough that the codebase merges have already been performed for them. NRT annotations in the public-facing/complex types should probably wait until those types' codebases are merged.

Any NRT annotation work will probably be made slightly more complex by our use of three SNIs: native, managed and SqlClientX. We'd need to make sure that null-safety checks are identical across these SNIs, and that might need changes within the native SNI.

[benrr101]

Hi @edwardneal - as always, thanks for your suggestions here. I agree that we should be looking into using nullable reference types going forward, but I'm not sure it's something we should do right now. I think the consensus among the team is that we want to adopt it for SqlClientX, but don't want to take it on for the current codebase. My suspicion is that if we try to start adopting it in the current codebase, it will likely spread across the codebase in a way similar to async. Although it is likely beneficial, I think it would also be a large set of changes that we don't really have the bandwidth to develop/test at the moment.

However, let's discuss how it could be done going forward. (Also please let me know if I'm misunderstanding something here)