Change EF Core codebase to use C# nullable references

[roji]

In addition to #10347, which is about user-facing changes related to the new C# 8 nullable reference feature, we can consider changing parts or all of the EF Core codebase to use nullable references internally. This would improve general code quality, test the new C# feature, etc. As the codebase already uses JetBrains annotations ([NotNull], [CanBeNull]), we wouldn't be starting from scratch.

[ajcvickers]

Agreed, but let's make sure we understand how this will work when targeting netstandard2.0 (i.e. .NET Framework) which will not have C#8 support.

[pmiddleton]

From what I have read from the C# and framework teams async streams, indexers and ranges will require netstandard 2.1 which .NET Framework will not support.

The default interface member feature requires a runtime change which will not be put into .NET Framework.

See Platform Dependencies here https://blogs.msdn.microsoft.com/dotnet/2018/11/12/building-c-8-0/

It appears nullable references will be supported. I'm putting the over/under at 5000 warnings being emitted from the current code base :)

[ajcvickers]

@pmiddleton We may need to cross-compile for multiple TFMs, but also we may not to, since often the compiler using things like pattern matching. @divega has been thinking through some of this stuff.

[roji]

Specifically for nullable references, if I understood things correctly there doesn't seem to be any issue with using them in codebases which target netstandard2.0 (or even older) - the compiler simply generates attributes at various levels which inform consuming code as to the nullability status of parameters, properties etc. Even the nullability attributes themselves are generates into the assembly, to prevent any sort of dependency on newer platforms etc.

The situation seems to be indeed different with async streams and indexers/ranges - but we may want to discuss these separately and leave this issue only for nullability.

The big question, as @pmiddleton hinted, is how to tackle the conversion with such a big codebase... It could be a single concentrated effort by a single person, or it could be a gradual process whereby each "owner" converts their own part of the codebase.

In any case, it may be a bit early to attempt this - it's probably a good idea to check with the C#/Roslyn team whether this is a good time or not.

[roji]

Note: this issue covers null-annotating two different things in the EF Core codebase:

1. User-facing APIs. I think this should be high priority: if I understand correctly, EF Core 3.0 will ship at the same time as C# 8, and there will be a very clear user expectation for newly-released APIs to have this.
2. Provider-facing, internal and private code. This is probably lower-priority - it hardens the codebase against null-related errors, but does not otherwise affect users directly.

Apart from this we also have #10347, which is about having the new nullability affect model-building.

[ajcvickers]

Marking for re-triage. Let's decide whether to split this work up or punt for 3.0.

[ajcvickers]

Moving this to the backlog since:

• Other things we could do in 3.0 will add more value
• It won't be a real breaking change if we do it later (binary compat; possible compile warnings)
• Best practices and patterns are still evolving

Note that we do still plan to interpret nullability (when used) in model building. See #10347

[ajcvickers]

@roji This backlog; Public surface above the line.

[obohaciak]

Re nullable reference types support, this could have another nice side-effect. One place where the EF Core use of JetBrains annotations bubbles up to calling code is when creating custom value generators.

At the moment, when implementing a value generator, the below snippet of code is generated automatically by VS 2019. Note that the method signature for Next contains the NotNull attribute because it follows the abstract method signature to the letter. Since I do not use JetBrains.Annotations in my code, I get a compile error (this is of course trivial to solve as I can simply remove the attribute)

using Microsoft.EntityFrameworkCore.ChangeTracking;
using Microsoft.EntityFrameworkCore.ValueGeneration;

namespace MyEFCore31Test
{
    internal class MyValueGenerator : ValueGenerator<int>
    {
        public override bool GeneratesTemporaryValues => throw new System.NotImplementedException();

        public override int Next([NotNullAttribute] EntityEntry entry)
        {
            throw new System.NotImplementedException();
        }
    }
}

[roji]

@obohaciak note that the current plan is to keep the Jetbrains annotations for public APIs, since some users may not use recent versions of VS (which support the new C# NRT feature), but use Resharper/Rider (#14568 tracked this specifically).

[ajcvickers]

@obohaciak Also see https://github.com/dotnet/roslyn/issues/5646

[obohaciak]

@roji. @ajcvickers thanks for the links, makes sense to me.

[roji]

Since we'll be annotating the entire codebase for nullability, closing this as a dup of #19007.

[roji]

Duplicate of #19007.