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

3.1.11: Disable NRTs in scaffolding #23060

Merged
merged 2 commits into from
Nov 13, 2020

Conversation

roji
Copy link
Member

@roji roji commented Oct 20, 2020

Original PR merged for 5.0: #21190

Note for Tactics

This issue was previously approved for 3.1.11. However, our ever astute MVP @ErikEJ pointed out that since 3.1 targets .NET Standard 2.0, we will potentially be generating

#nullable disable

in projects where that won't compile.

Therefore, we could either not do this at all, or instead generate something like:

// Code scaffolded by EF Core assumes nullable reference types (NRTs) are disabled.
// If you have enabled NRTs for your project, then un-comment the following line:
// #nullable disable

This backports the fix for #23016 to 3.1.

Description

This adds #nullable disable at the top of all C# files generated by EF Core's scaffolding.

Customer Impact

EF Core scaffolding is currently unaware of C# 8.0 Nullable Reference Types (NRT), and generates a code model without annotations. If the project has NRTs turned on, the model get incorrectly interpreted, i.e. nullable database text columns are represented by string SomeProperty, which with NRTs is interpreted as non-nullable.

How found

Reported by users (e.g. #23016). The issue for adding fully supporting NRTs, i.e. correctly scaffolding code with annotations (#15520) also has 28 votes (making it the 29th most requested feature), giving reason to believe many users are running into this.

Test coverage

This PR includes testing that the #nullable disable actually gets scaffolded.

Regression?

No.

Risk

Low. This touches scaffolding code only - which does not affect EF Core runtime behavior - and in a minor way.

(replaces #23053 which had a persistent CI issue)

@ajcvickers
Copy link
Member

@roji Approved for 3.1.11 by Tactics. Please wait until the branch is open for 3.1.11 before merging.

@ajcvickers ajcvickers removed this from the 3.1.11 milestone Oct 20, 2020
@ajcvickers ajcvickers changed the title 3.1.x: Disable NRTs in scaffolding 3.1.11: Disable NRTs in scaffolding Oct 23, 2020
@ErikEJ
Copy link
Contributor

ErikEJ commented Nov 4, 2020

Will this affect usage of the scaffolded code with .NET Standard 2.0 projects in a bad way?

@ErikEJ
Copy link
Contributor

ErikEJ commented Nov 4, 2020

@roji
Copy link
Member Author

roji commented Nov 4, 2020

Good point @ErikEJ. 5.0 targets netstandard2.1 so this makes sense there, but depending on the of netstandard2.0 users this could be disruptive.

@ajcvickers let's cancel backporting this?

@ajcvickers
Copy link
Member

@roji @ErikEJ It depends on the compiler version, right? Which compiler versions don't understand this?

@ErikEJ
Copy link
Contributor

ErikEJ commented Nov 4, 2020

As I understand, .NET Standard 2.0 does not support C# 8 and 7.3 does not understand this (see linked comment)

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/configure-language-version#defaults

@roji
Copy link
Member Author

roji commented Nov 4, 2020

Yeah, it's possible (but unsupported) to use C# 8 with netstandard2.0, but for many people out there the language version is 7.3 (the default), and if #nullable disable starts to appear that won't compile.

@ajcvickers
Copy link
Member

@roji Let's discuss this in triage tomorrow.

@leecow leecow added this to the 3.1.11 milestone Nov 5, 2020
@ajcvickers
Copy link
Member

@dotnet/efteam Should we still do this on 3.1, but change it to a comment?

@ajcvickers ajcvickers modified the milestones: 3.1.11, 3.1.x Nov 9, 2020
@leecow leecow removed this from the 3.1.x milestone Nov 12, 2020
@leecow leecow added this to the 3.1.11 milestone Nov 12, 2020
Part of #15520

(cherry picked from commit 0b50d83)
@ajcvickers
Copy link
Member

Approved by Tactics for 3.1 with the #nullable disable in a comment. PR updated.

@ajcvickers ajcvickers requested review from a team and ajcvickers November 12, 2020 21:51
@ajcvickers ajcvickers merged commit 405f549 into release/3.1 Nov 13, 2020
@ajcvickers ajcvickers deleted the ScaffoldingDisableNullable5.0 branch November 13, 2020 15:52
@ajcvickers ajcvickers removed this from the 3.1.11 milestone Nov 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Output #nullable disable in reverse-engineered model Scaffolding and C# 8 nullable reference types
5 participants