Write a DiagnosticSuppressor for CS8618 for DbSet properties

[Youssef1313]

See dotnet/roslyn#45912

[Youssef1313]

If you wish, I may try to give a PR that implements it.

[ajcvickers]

/cc @roji

[roji]

Makes sense to me... We already have Microsoft.EntityFrameworkCore.Analyzers, this could go in there. Please feel free to submit a PR for this.

[davidroth]

IMO it would also make sense to document how simple expression bodied db-sets are an alternative way to define DbSet properties.

So instead of using mutable auto properties like this:

public class NullableReferenceTypesContext : DbContext
{
    public DbSet<Customer> Customers { get; set; } = null!;
    public DbSet<Order> Orders { get; set; } = null!;
}

Using expression bodied get-only properties can be a more natural way to define them.

public class NullableReferenceTypesContext : DbContext
{
    public DbSet<Customer> Customers => Set<Customer>();
    public DbSet<Order> Orders => Set<Order>();
}

Pros:

• Immutable DBSet properties. It makes no sense to have mutable db-sets. So why even declare them mutable if they are not.
• "Natural" fix for CS8618 because Set<>() has a none-nullable return value.
• No property initialization cost because when constructing a new context EF does not have to initialize all DbSet properties in advance. (Although this is highly optimized anyway, so no real measurable benefit).

Note that I am not arguing against the suppressor. I am just wondering why the auto-properties "way" of defining DBSets, is still the default one described in all the docs etc? Probably just because of historical reasons? Does it make sense to re-evaluate this default? Thoughts @ajcvickers @roji ?

[roji]

Hmm, I think this might require a bit of change in EF Core, i.e. when the DbContext constructor is doing its thing. I suspect that will especially be necessary to support read-only properties here.

However, assuming the required changes aren't huge and it yields the same perf, I like it a lot - it definitely looks much better than the current guidance with null - and anything not requiring a suppressor is obviously better.

@ajcvickers thoughts?

[ajcvickers]

@roji What exactly is the question? Is the suggestion to change the guidance for if you're using NRT, or to change what we reverse engineer when NRTs are being used? Or something else?

[roji]

Yeah - @davidroth's suggestion above is clean, resolves the NRT warning and even makes the DbSet properties read-only, which is a bit more correct. I've submitted dotnet/EntityFramework.Docs#2520 to integrate this into our NRT guidance.

[Neme12]

@roji I still think the diagnostic suppressor would be a good addition because defining DbSets as auto-properties is still supported.

[roji]

As suggested, reopening to consider doing this so that the following doesn't generate an NRT warning:

public class SomeContextContext : DbContext
{
    public DbSet<Customer> Customers { get; set; }
}