Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change EF Core codebase to use C# nullable references #14150

Closed
roji opened this issue Dec 12, 2018 · 14 comments
Closed

Change EF Core codebase to use C# nullable references #14150

roji opened this issue Dec 12, 2018 · 14 comments

Comments

@roji
Copy link
Member

roji commented Dec 12, 2018

In addition to #10347, which is about user-facing changes related to the new C# 8 nullable reference feature, we can consider changing parts or all of the EF Core codebase to use nullable references internally. This would improve general code quality, test the new C# feature, etc. As the codebase already uses JetBrains annotations ([NotNull], [CanBeNull]), we wouldn't be starting from scratch.

@ajcvickers
Copy link
Member

Agreed, but let's make sure we understand how this will work when targeting netstandard2.0 (i.e. .NET Framework) which will not have C#8 support.

@pmiddleton
Copy link
Contributor

From what I have read from the C# and framework teams async streams, indexers and ranges will require netstandard 2.1 which .NET Framework will not support.

The default interface member feature requires a runtime change which will not be put into .NET Framework.

See Platform Dependencies here https://blogs.msdn.microsoft.com/dotnet/2018/11/12/building-c-8-0/

It appears nullable references will be supported. I'm putting the over/under at 5000 warnings being emitted from the current code base :)

@ajcvickers
Copy link
Member

@pmiddleton We may need to cross-compile for multiple TFMs, but also we may not to, since often the compiler using things like pattern matching. @divega has been thinking through some of this stuff.

@roji
Copy link
Member Author

roji commented Dec 13, 2018

Specifically for nullable references, if I understood things correctly there doesn't seem to be any issue with using them in codebases which target netstandard2.0 (or even older) - the compiler simply generates attributes at various levels which inform consuming code as to the nullability status of parameters, properties etc. Even the nullability attributes themselves are generates into the assembly, to prevent any sort of dependency on newer platforms etc.

The situation seems to be indeed different with async streams and indexers/ranges - but we may want to discuss these separately and leave this issue only for nullability.

The big question, as @pmiddleton hinted, is how to tackle the conversion with such a big codebase... It could be a single concentrated effort by a single person, or it could be a gradual process whereby each "owner" converts their own part of the codebase.

In any case, it may be a bit early to attempt this - it's probably a good idea to check with the C#/Roslyn team whether this is a good time or not.

@roji
Copy link
Member Author

roji commented Dec 31, 2018

Note: this issue covers null-annotating two different things in the EF Core codebase:

  1. User-facing APIs. I think this should be high priority: if I understand correctly, EF Core 3.0 will ship at the same time as C# 8, and there will be a very clear user expectation for newly-released APIs to have this.
  2. Provider-facing, internal and private code. This is probably lower-priority - it hardens the codebase against null-related errors, but does not otherwise affect users directly.

Apart from this we also have #10347, which is about having the new nullability affect model-building.

roji added a commit to roji/efcore that referenced this issue Jan 31, 2019
This takes care of:
* EFCore.Abstractions
* EFCore/Storage
* EFCore/Internal
* EFCore/

Part of dotnet#14150

Updates the entire solution to use C# 8.0
roji added a commit to roji/efcore that referenced this issue Feb 2, 2019
This takes care of:
* EFCore.Abstractions
* EFCore/Storage
* EFCore/Internal
* EFCore/

Part of dotnet#14150

Updates the entire solution to use C# 8.0
roji added a commit to roji/efcore that referenced this issue Feb 2, 2019
This takes care of:
* EFCore.Abstractions
* EFCore/Storage
* EFCore/Internal
* EFCore/

Part of dotnet#14150

Updates the entire solution to use C# 8.0
roji added a commit that referenced this issue Feb 2, 2019
This takes care of:
* EFCore.Abstractions
* EFCore/Storage
* EFCore/Internal
* EFCore/

Part of #14150

Updates the entire solution to use C# 8.0
@ajcvickers ajcvickers removed this from the 3.0.0 milestone Mar 26, 2019
@ajcvickers
Copy link
Member

Marking for re-triage. Let's decide whether to split this work up or punt for 3.0.

@ajcvickers
Copy link
Member

Moving this to the backlog since:

  • Other things we could do in 3.0 will add more value
  • It won't be a real breaking change if we do it later (binary compat; possible compile warnings)
  • Best practices and patterns are still evolving

Note that we do still plan to interpret nullability (when used) in model building. See #10347

@ajcvickers
Copy link
Member

@roji This backlog; Public surface above the line.

@obohaciak
Copy link

Re nullable reference types support, this could have another nice side-effect. One place where the EF Core use of JetBrains annotations bubbles up to calling code is when creating custom value generators.

At the moment, when implementing a value generator, the below snippet of code is generated automatically by VS 2019. Note that the method signature for Next contains the NotNull attribute because it follows the abstract method signature to the letter. Since I do not use JetBrains.Annotations in my code, I get a compile error (this is of course trivial to solve as I can simply remove the attribute)

using Microsoft.EntityFrameworkCore.ChangeTracking;
using Microsoft.EntityFrameworkCore.ValueGeneration;

namespace MyEFCore31Test
{
    internal class MyValueGenerator : ValueGenerator<int>
    {
        public override bool GeneratesTemporaryValues => throw new System.NotImplementedException();

        public override int Next([NotNullAttribute] EntityEntry entry)
        {
            throw new System.NotImplementedException();
        }
    }
}

@roji
Copy link
Member Author

roji commented Feb 3, 2020

@obohaciak note that the current plan is to keep the Jetbrains annotations for public APIs, since some users may not use recent versions of VS (which support the new C# NRT feature), but use Resharper/Rider (#14568 tracked this specifically).

@ajcvickers
Copy link
Member

@obohaciak Also see dotnet/roslyn#5646

@obohaciak
Copy link

@roji. @ajcvickers thanks for the links, makes sense to me.

@roji
Copy link
Member Author

roji commented Dec 11, 2020

Since we'll be annotating the entire codebase for nullability, closing this as a dup of #19007.

@roji
Copy link
Member Author

roji commented Dec 11, 2020

Duplicate of #19007.

@roji roji closed this as completed Dec 11, 2020
@roji roji removed this from the 6.0.0 milestone Dec 11, 2020
@roji roji removed their assignment Dec 11, 2020
@ajcvickers ajcvickers reopened this Oct 16, 2022
@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants