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 (Replacement for Microsoft.CodeAnalysis.MSBuild.MSBuildWorkspace? roslyn#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 dotnet/roslyn#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]

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 #17212 (comment) for more details.