efcore: Invalid ThenInclude() Nullable Reference Type Warning

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

About this issue

  • Original URL
  • State: closed
  • Created 5 years ago
  • Reactions: 5
  • Comments: 19 (13 by maintainers)

Most upvoted comments

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

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.

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.

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.

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.

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()