Scaffolding and C# 8 nullable reference types

[roji]

#10347 makes EF Core recognize the new C# 8 nullable reference types (NRT), and treat them as [Required]. However, we still need to take care of scaffolding: the model we currently scaffold will not compile under projects with NRT enabled, as reference properties will be treated as non-nullable and the compiler will complain about them being uninitialized.

According to @bricelam it should be relatively easy to detect whether the project we're scaffolding into has turned on NRT or not. This would allow us to scaffold differently based on that setting.

Nullable database columns should obviously be scaffolded as nullable properties:

public string? SomeNullableProperty { get; set; }

For non-nullable columns, it seems that guidance is aligning to having non-nullable properties, forced-initialized to null as follows:

public string SomeNonNullableProperty { get; set; } = default!;

Note the null-forgiving bang operator which suppresses the warning. Alternatives for non-nullable properties include:

• A single pragma suppressing the uninitialized warning for the entire file. This seems less obvious and more error-prone if the scaffolded model is later updated manually by the user.
• Scaffold non-nullable properties backed by nullable fields, with a getter throwing InvalidOperationException if the value is null (i.e. if the property is accessed before initialization). While this is a better pattern than null-initialization (early and informative exception), it is also considerably more verbose and would probably put people off the NRT feature.

[MiKom]

I'll also add my two cents, that documentation for EF probably needs an update wrt to Nullable Reference Types. Right now, all tutorials and guides guide you to write your DbContext class as follows

public class MyDbContext : DbContext
{
    public MyDbContext(DbContextOptions options) : base(options) {}

    public DbSet<MyEntity> MyEntities { get; set; }
}

And this will produce a warning when compiling with NRTs.
I couldn't find an issue about tackling this in docs. Should I create one?

[ajcvickers]

Triage: for 5.0, at least scaffold the disable.

[grosch]

Why was this punted for version 5? It's painful when the entire project uses nullable, and then you have no idea with the model whether or not it's nullable.

[roji]

@grosch in 5.0, we scaffold #nullable disable into the source files we scaffold (this is #21190, and #23060 backports that to 3.1). This issue tracks having EF actually scaffold source code with proper nullability (i.e. with #nullable enable), based on whether you've turned the feature on in your project.

Scaffolding nullable code was scoped out of EF 5.0 because of prioritization/time constraints, especially considering that EF 5.0 doesn't support other aspects of nullability (e.g. EF's API itself isn't annotated). We plan to address all that for EF 6.0.

[ErikEJ]

@roji I have done some experiment here, would be very grateful if you could sanity check the changes to this file (both for properties and Navigations):

https://github.com/ErikEJ/EFCorePowerTools/pull/716/files#diff-0b837076ebb587ebcfd0963e96552fa7d78c3ccdf16384410afa1d85115764ea

[roji]

@ErikEJ did a quick review.

[roji]

Design decision:

• We considered various patterns for scaffolding non-nullable properties and navigations:
  1. Scaffold a constructor with a mandatory parameter.
  2. Scaffold a nullable backing field accessed by a non-nullable property whose getter throws when the field is null (uninitialized)
  3. Scaffold a non-nullable property that's bang-initialized to null.
• We decided on option 3, which is closest to today's usage of EF Core and existing code, so improves chances of users switching on the NRT feature without too much pain. Option 2 is more verbose with very little additional value.
• For DbSets, we considered the following:
  1. Scaffold a getter-only property which calls Set<T>(). This adds an additional dictionary lookup for every query.
  2. Scaffold an auto-property and a constructor which initializes it by calling Set<T>()
  3. Scaffold an auto-property that's bang-initialized to null (DbContext's base constructor injects the DbSet instance in any case)
  4. Look into making the Set<T>() method static, and initialize the auto-property to that. However, DbSets do reference their DbContext instance, since the model may be needed (lazily), so this isn't feasible.
• For simplicity's sake, we chose to simply bang-initialize to null. This is again in line with today's practices, is terse, and represents well the fact that DbContext implicitly initializes all DbSets as part of construction - a fact of which the compiler is unaware.