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)

[edwardneal]

Hi @benrr101, thanks for the discussion. The annotations required for nullable reference types touch the entire codebase, but these changes aren't impactful for a code path until every type in it is annotated; the main point of #3092 is to raise the work for triage so it can be done piecemeal.

Although it's similar to async (any implementation would need to work from the low-level dependencies upwards) it's more forgiving. If a type is annotated as a nullable reference type, those annotations are ignored at compile-time by default. The only time they're considered is when it's referenced by another NRT. We can see that in SqlJson - it's marked as #nullable enable, and the rest of the codebase didn't need modification to accommodate it.