Efcore: Support C# 8.0 nullable reference types in model building

[MarcoLoetscher]

C# 8 is on the way and better to early than too late should be planned the implementation of the new features (Introducing Nullable Reference Types in C#).

public class Blog
{
    public int BlogId { get; set; }
    public string Url { get; set; } //In C# 8 this should be required by default
    public string? Url2 { get; set; } //In C# 8 this should be optional by default

    //Needs EF support for constructors with parameters
    public Blog(string url) 
    {
        Url = url;
    }
}

A new logic is needed for mapping of e. g. string: Required and Optional Properties

Support for constructors with Parameters is required: Flexible mapping to CLR types and members (Custom O/C Mapping #240)

This list is probably not complete.

[ajcvickers]

@MarcoLoetscher This is absolutely something we want to leverage, but since we are generating additional semantics and behavior based on nullability we will need to consider carefully how to do this without making unintended breaking changes to model/schema generation.

[roji]

@ajcvickers @smitpatel do you want me to work on this? I'd first submit a design proposal for discussion (I'm assuming @smitpatel is pretty darn busy with other stuff). We can also discuss next week.

[ajcvickers]

@roji Yes, go for it. I don't think there needs to be much of a design proposal here, but we can discuss next week. I think maybe when we talked on the phone you suggested doing #14150 first, which makes sense to me to give us an idea of where the feature is at and what we might run into.

[roji]

Don't forget scaffolding - we should use C# 8 nullability to express nullability in generated classes.

[roji]

Notes from triage for scaffolding:

• We've already decided to generate C# 8 code. So we can just generate a #nullable enable at the top of the generated file. This allows us to always generate nullable-aware code.
• We can also parse the project to see if the user has enabled (via Roslyn), but this introduces a pretty big dependency. Also, not sure if there's a cross-platform Roslyn MSBuild workspace (https://github.com/dotnet/roslyn/issues/17974).
• Opt-in flag on the command line. Seems like a bit of a shame to force users to opt into nullable references yet again, and discoverability isn't going to be very good.

So probably just generate #nullable enable.

[roji]

This is currently blocked by https://github.com/dotnet/roslyn/issues/30143

[roji]

One more important note... If you have a non-nullable auto-property, then you must have a constructor which initializes it, otherwise the compiler will warn/error. So we need to decide what we want to scaffold for non-nullable database columns, and also what we want our recommendations/code samples to look like.

The options seem to be:

1. Scaffold a constructor which has all non-nullable properties.
2. Scaffold a nullable backing field, wrapped by a non-nullable property with a getter that throws for null. This means that it's the user's responsibility to populate the property before calling SaveChanges(). It's less correct, and it means a runtime exception if the property isn't populated, which is similar to an NRE - exactly what we're trying to avoid. However, it's a valid/known pattern for some cases where some properties are initialized late (e.g. dependency injection without constructors).
3. Scaffold property initializers to some non-null default value, like empty string for string. This would be specific to each type, so we'd have to have a mechanism to determine it...

Any input?

[ajcvickers]

@roji The constructor feels like the cleanest. However, this makes me wonder how many people will actually want this level of strictness. That is, how many people will actually want to use non-nullable types if it means they need to start calling parameterized constructors (or deal with other code consequences) when they didn't before. I wonder if it should be the default for reverse engineering.

[smitpatel]

At least in current preview of VS 2019, the default tfm for app is netcoreapp2.2 so nullable types are available but the default language version is still 7.0 (since 8.0 is in beta phase, which likely to change before RTM). So while VS shows that adding #nullable enable is possible, it does not compile due to language version. Unless 8.0 becomes default for new app, we may still need to look into project to figure out the lang version.

[roji]

Yeah, it may be a good idea to reach out to the language/compiler people and confirm what they intend to do with this.

@ajcvickers another thought... Our decision of how to scaffold may not be related to the user's choice at the project level - C# allows mixing null-aware and null-oblivious code quite easily. So even if the user hasn't opted into null awareness (because it seems too strict at the moment or whatever), we can still decide to generate a null-aware model. So I think it still makes sense to generate a nice, correct null-aware model with a full constructor - I can't really see much downside to it...

[roji]

Finally, we can always defer the scaffolding part of this for now, until the default language situation becomes more clear.

[gojanpaolo]

I'm not sure if this is an EF issue but how should we define navigational properties?

// Post will always be in a Blog
public class Post
{
    public int BlogId { get; set; }
    public Blog Blog { get; set; } //class will get non-nullable warning
}

My first intuition is to add constructor but EF Core cannot set navigation properties (such as Blog or Posts above) using a constructor. .

We could make Post.Blog nullable but it wouldn't satisfy Post should always be in a Blog.

[roji]

@gojanpaolo that's indeed the kind of stuff that needs to be worked out for nullability to work... As a last resort, even if we can't figure out a better way to deal with this, you could make Blog nullable but still have a [Required] attribute on it.

[smitpatel]

In future, we want to support taking navigation properties in constructor too. @AndriySvyryd knows more details.

[gojanpaolo]

I see. Another workaround is to define a parameterless constructor and disable the nullable warning there. Mark it as private to hide from explicit use. Caveat is Post.Blog will be null when taken from DbContext or DbSet so don't forget to Include it if you do eager loading like myself.

EDIT: above workaround is not a good idea. it hides other nullability warnings as well.

class Post
{
    public string Title { get; set; } //should have nullable warning if uninitialized
    public Blog Blog { get; set; }
}

[roji]

I agree that the ideal thing here is to have a constructor and to have EF Core go through that. Another option here is to have a nullable backing field for the non-nullable property, who's getter throws on null. This isn't perfect, but it's a way to instantiate with the post with the assumption that it gets injected later. I'm not sure if that works well for the relationship scenario though.

@ajcvickers maybe this is another good thing to take up in design?

[ajcvickers]

@roji Yes, I think we need to consider this. It's interesting that when relationships are marked as required, it only means that the navigation property is non-null _when loaded_. But, until now, it's always been acceptable to have something that is required but not loaded. I'm not sure the best way to even conceptualize this.

[roji]

@ajcvickers I agree this requires some thought. However, it seems to resemble some other standard scenarios such as dependency injection, where a property is supposed to always be populated conceptually but is injected late; so theoretically it can be null for a short while, but if initialization takes place as its supposed to, it never will be. It seems like you don't want to burden users with null checks everywhere just because during initialization it can be null just for a short while, so a good design could be a nullable field wrapped by a non-nullable property whose getter throws on null.

The same seems to work also for relationships: as long as the user adheres to the programming contract of calling Include() (or lazy loading), they should never see a null - so the property they're interacting with should be non-nullable. The detail that the property is populated late in the process doesn't seem very important or relevant.

[gojanpaolo]

Should models used on both EF6 and EF Core also be considered? Not sure if that is a common use case but I am currently in that situation (trying to slowly port a project from EF6 to EFCore) and the following workaround would not work on EF6 with lazy loading.

nullable backing field for the non-nullable property, who's getter throws on null

my current workaround is to enclose the navigational properties in #nullable disable

public class Post
{
    public string Title { get; set; } = "";
#nullable disable
    public virtual Blog Blog { get; set; }
#nullable enable
    public virtual ICollection<Comment> Comments { get; } = new List<Comment>();
}

[roji]

@gojanpaolo it would indeed be possible disable any EF Core behavior around new non-nullable references by simply disabling the nullable context around the relevant model properties, as you've done (or possibly the entire model). This would make EF Core behave exactly in the same way as today. Note that other means for specifying requiredness (i.e. the [Required] attribute and the fluent API) will also most probably continue to work as today.

There are no plans to modify EF6 to recognize non-nullable references.

[roji]

As @ajcvickers mentioned in an offline discussion, this feature should make it into 3.0, because releasing it later would be a breaking change as non-nullable model properties will suddenly start getting treated as required by EF Core.

For now we seem to agree to punt scaffolding non-nullable references. However, we still have to consider nullability within scaffolded models: we will probably need to disable the nullable context (by placing a #nullable disable at the top of scaffolded files), otherwise properties we scaffold into nullable-aware user projects will be interpreted as non-nullable. This would only compile if C# 8 is enabled in the user's project, which again depends on the C# version we're targeting for scaffolding (see #14545).

[roji]

Removing breaking-change label as this will only affect users opting into the NRT feature.

[MarkusHaslinger]

Should this work for migrations already?

Because I just tried to create a migration and it emitted nullable: false for all columns including DateTime? and string?
When setting IsRequired(false) manually for those properties during model creation the flags for the columns are emitted correctly.

[ajcvickers]

@MarkusHaslinger You might be hitting #16760, which is fixed in the nightly builds

[MarkusHaslinger]

@ajcvickers thanks, will try the nightly

[shoe-diamente]

Is there already some examples for this? I'm having an issue with initialising DbSets in my DbContext. Should every DbSet be nullable or default initialised?

    public class DatabaseContext : DbContext
    {
        public DatabaseContext(DbContextOptions<DatabaseContext> options)
            : base(options)
        {
        }

        public virtual DbSet<User> Users { get; set; } = ...;
    }

[Zenexer]

@shoe-diamente I believe there's been some discussion about a better alternative, but, at least for now, you're best off just surrounding the properties with a #pragma directive that disables the warning.

Edit: Use = null! instead.

[roji]

@shoe-diamente a doc page with best practices for non-nullable types will be up soon. In the meantime, keep your DbSet properties non-nullable and initialize them to null with the null-forgiving operator (bang). See https://github.com/aspnet/EntityFrameworkCore/issues/17212#issuecomment-522503909 for more details.