Invalid ThenInclude() Nullable Reference Type Warning

[kateract]

I am trying to use EF Core with the new C# 8.0 nullable reference types, and I am trying to find the right pattern to use. This leads me to a few issues, but the most concrete one is a warning when I try to use ThenInclude with a property that is nullable.

//a.Beta is nullable, b.Delta is nullable
// Dereference of a possibly null reference for b.Delta 
//                                                 \/
context.Alpha.Include(a => a.Beta).ThenInclude(b => b.Delta);

However, based on #9038 it appears that ThenInclude() should not dereference the null if the Include() object is null. Again, I'm not sure if this is a bug or just ignorance of the pattern on my part.

Steps to reproduce

https://gist.github.com/kateract/3b07b479c5d284d8a5b859cb99299371

Further technical details

EF Core version: 3.0.0-preview8.19405.11
Database Provider: Microsoft.EntityFrameworkCore.SqlServer
Operating system: Windows
IDE: Visual Studio Professional 2019 16.3.0 Preview 1.0

[smitpatel]

Not a bug. Nullable reference type warnings are compiler generated. What the warning say is true that you are potentially dereferencing a null.
Though at runtime, EF Core will not cause any error as, it would not try to Include Delta if Beta was null.
You can just suppress the warning.

[ajcvickers]

I think there are things to discuss here--if only docs.
/cc @divega @roji

[kateract]

This leaves me with questions about how much suppressing the warning is acceptable. I am building a project targeting 3.0, and I decided to try out the NRT feature too as I think it is really valuable for preventing problems before they happen.
So far to use EF core in the project, I have to suppress warnings in many different areas:

• Creating a DbSet Parameter on the Context - is there a "correct" initialization for this type as it is populated by the context? or should they be marked as nullable?
• Adding a relationship with fluid API when one of the keys is nullable/optional (relationship not always present)
• The aforementioned issue
• Creating a non-nullable property on a model class - maybe there is an annotation that can be added to DataAnnotations for NotNull along the lines of the TypeName parameter of Column

There's a part of me that thinks if I am using a library class that I shouldn't have to suppress warnings to use it in the intended manner, otherwise I feel like it adds friction to using NRT with EF. Is there some attribute or something in the implementation of NRT that can mark a function's intention to be null safe at runtime?
In this case, I also worry about suppressing this warning when there can be other references on or near this line that would get hidden by suppression and defeating the purpose.
If the resolution for this is documentation, at the least it would be good to remind people that they can exclude just the part of the statement in question:

Context.Alpha.Include(a => a.Beta)
#nullable disable
.ThenInclude(b => b.Delta)
#nullable restore
.ToListAsync()

[smitpatel]

Another similar case
Where(od => od.Order.Customer.City == "Seattle") EF makes everything null safe when accessing navigation. NRT would still warn if your navigations are marked as nullable.

[ajcvickers]

The wider issue here is that these are just expression trees. This code is never executed, so does it even make sense to flag nullability issues here? As Smit's case makes very clear, the SQL that is actually executed doesn't have the same nullability semantics anyway. For the Include case, the call to Include is just telling EF to create a certain type of query--clearly the executing SQL is very different.

[kateract]

The main reason I marked this particular thing as an issue is that all of my other suppression in this case is inside the context and associated model classes (where I have a non-nullable string property that doesn't have a sensible default initialization). This is the only place that I have to suppress null warnings outside of the context and model classes.

[kateract]

@smitpatel The weird thing is that if I mark the DbSet parameter as nullable, it gives me a null dereference warning if I use Where() on the inner access, but Include() works just fine, no warning.

{
...
        public DbSet<Alpha>? Alpha { get; set; }
        public DbSet<Beta>? Beta { get; set; }
        public DbSet<Delta>? Delta { get; set; }
}
...
            //warning
            context.Alpha.Where(a => a.Beta.ID > 3)
                //fine
                .Include(a => a.Beta)
                //warning
                .ThenInclude(b => b.Delta);

[divega]

I suspect what is triggering the first warning is the access to Beta.ID: Since a.Beta can be null Beta.ID can fail (I am inferring this, haven't seen the definition of the Beta class).

The call to Include doesn't cause a warning because although a.Beta can be null the delegate type Include accepts allows nulls.

The call to ThenInclude produces a warning because the compiler knows that the access to b.Delta flows from a.Beta on the previous call, which can be null.

Most of these warnings in query methods are superfluous because as @smitpatel and @ajcvickers mentioned before, EF Core in most cases translate the expressions to SQL (which has different null semantics) rather than executing them in memory.

Do you have an example of your previous point "Adding a relationship with fluid API when one of the keys is nullable/optional". I assume the warning occurs when configuring the keys, am I right?

[divega]

I am starting to wonder if it would be worth annotating the extension methods on IQueryable<T> to ignore nullability.

We could do it right away for some of the extension methods defined by EF Core, but for LINQ operators in the BCL it requires more thinking:

We don't know that all IQueryable providers translate to something that propagates nulls like SQL. Ideally there would be a way for the EF Core provider in use in the DbContext to influence this, but that seems unfeasible.

Another option that perhaps exists (I don't really know) is to have providers somehow carry "warning suppressions" for specific methods, so if you just have that provider installed then the LINQ operators won't warn.

@roji any thoughts about this?

[roji]

tl;dr IMHO DbSets should definitely be non-nullable, and possibly also navigation properties themselves (or use null-forgiving for the last).

DbSets on the context are a great example of late-initialized properties - they are 100% guaranteed to never be null by EF Core (DbContext's constructor initializes them), but the compiler doesn't know this. The best practice for this kind of case is to keep them non-nullable and to force-initialize to null with the null-forgiving operator: public DbSet<Alpha> Alpha { get; set; } = null!;.

The case of required properties without a sensible default is similar but a bit weaker - there is no airtight guarantee that they're always initialized, but assuming the user properly materialized an entity from the database (including with the proper Includes), they would never observe a null. For these cases it's also fine to force-initialize to null, although a non-nullable property wrapping a nullable field and throwing InvalidOperationException for nulls is a bit tighter (but more verbose).

Navigation properties within queries is obviously more tricky... For nullable non-collection navigations, it seems like sprinkling the null-forgiving operator is OK (better than warning suppression because more precise):

Where(od => od.Order!.Customer!.City == "Seattle")

It's not perfect, but the user (unlike the compiler) knows the query is translated by EF and that null semantics will be taken care of - so it's their responsibility to communicate that.

A more extreme but possibly better solution, is to make the navigation non-nullable, and to tell EF via other means that the relationship isn't required (via [Required] or the fluent API). This could make sense if we think that navigation traversal occurs mainly (or even exclusively) inside LINQ queries - which seems to me to be the common case. To avoid NRE when the navigation is traversed outside EF, the same pattern with an InvalidOperationException-throwing getter can be used.

(Note: collection navigations should obviously be non-nullable - not having a related entity is expressed by having an empty list rather than a null.)

@divega

We could do it right away for some of the extension methods defined by EF Core, but for LINQ operators in the BCL it requires more thinking:

We are pretty confident (or at least hopeful) that LINQ async operators will make their way into the BCL at some point, so I'd be wary doing anything like this. It seems fundamentally problematic (probably impossible) to distinguish between invocations of operators on EF (where we want to suppress) and outside EF (where we don't)...

Another option that perhaps exists (I don't really know) is to have providers somehow carry "warning suppressions" for specific methods, so if you just have that provider installed then the LINQ operators won't warn.

I'm not aware of anything like this, especially not in a way that would suppress the warnings only when applied to EF DbSets. We'd almost need a "reverse analyzer", which suppresses warnings based on analysis instead of generating them... This might not be a bad idea but it doesn't exist AFAIK (and would be hard to make a case for).

We should definitely document the above best practices (once we agree on them), a specific doc page on nullability may make sense.

[divega]

(or at least hopeful) that LINQ async operators will make their way into the BCL at some point

Are you thinking about LINQ to Objects operators or async operators for IQueryable<T>? I was talking only about operators on IQueryable<T>.

It seems fundamentally problematic (probably impossible) to distinguish between invocations of operators on EF (where we want to suppress) and outside EF (where we don't)...

That is why I was thinking that we could do this right away on the extension methods that EF Core defines for IQueryable<T>, like Include, ThenInclude, etc. We practically know what those will be invoked only for EF Core (and if they are ever used with something else, I don't think that expecting null propagation semantics is too bad).

We'd almost need a "reverse analyzer"...

Yes, I was thinking exactly about this. Anti-analyzers.

It's not perfect, but the user (unlike the compiler) knows the query is translated by EF and that null semantics will be taken care of - so it's their responsibility to communicate that.

I get what you are saying, and maybe there isn't much we can do now, but it is a lot of paper cuts.

We should definitely document the above best practices...

Definitively, we should. I think you previous comment is a good starting point for the docs section.

[roji]

That is why I was thinking that we could do this right away on the extension methods that EF Core defines for IQueryable, like Include, ThenInclude, etc.

Ah right, I misunderstood. For these we may actually be able to annotate with the new attributes. I'll take a quick look at this later today.

But this would still leave the standard LINQ operators (e.g. Where) where we can't really do anything...

[roji]

From a quick look I think we can't do anything about Include/ThenInclude. The generic type definition of the property - including its nullability - flows from the Include's selector to the ThenInclude's selector, and there's no method or property where we could put a [NotNull] attribute. We would basically need a way to tell the compiler that the Include returns an IIncludableQueryable over a TProperty with a different nullability than the one inferred from the user-specified property in the lambda.

[kateract]

Do you have an example of your previous point "Adding a relationship with fluid API when one of the keys is nullable/optional". I assume the warning occurs when configuring the keys, am I right?

@divega The warning comes up when I do a WithOne after a HasMany where the one property may be null.

One of my mistakes before creating this issue was my own lack of awareness of the null-forgiveness operator !. When I did see it, I searched for "! C#" with predictably useless results, and now that I know the name I can see where I missed it in the documentation. It might be worth suggesting that the ! operator get more prominent positioning in the NRT documentation. The tutorial I read here actually uses one but doesn't mention anything about it in the discussion.

For my two cents, I agree that using ! in the expression tree is better than suppressing the warning. One of my concerns is writing code that a new or less experienced coder can understand and replicate without causing a problem. Using warning suppression frequently makes me think it suggests that it's an accepted practice rather than an outstanding situation. Without loading the code up with comments when we're already adding two extra lines in for suppression it makes it difficult to justify.

I definitely feel like a EF & NRT doc page outlining best practices would be most helpful.

[AfzalSohaib]

I learned from this guide from Microsoft that we can auto-include navigational properties. I used that to auto-include the grandchild navigation property whenever the child entity is queried from the db. Is that a reasonable workaround?

[VWilcox2000]

I completely agree with @roji -- both that I'd use !:

             context.Alpha.Include(a => a.Beta).ThenInclude(b => b!.Delta);

And the EF team should probably rewrite the ThenInclude declaration so it accepts a nullable reference.

[roji]

And the EF team should probably rewrite the ThenInclude declaration so it accepts a nullable reference.

ThenInclude does accept a nullable reference - or rather, it accepts whatever type flows from the previous Include (which is a nullable Beta in the above case). The problem is when you access a property on that nullable reference - the compiler issues a warning, since as far as it's concerned, b may indeed be null.

[roji]

The solution here is most likely a diagnostics suppressor that identifies EF LINQ queries, and suppresses nullability warnings within them - that's covered by #21663. Note that statically identifying EF LINQ queries connects with AOT work we're planning, where those queries are pre-compiled (see #22086 for examples of these).

[roji]

Duplicate of #21663