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

Scaffolded DbContext produces CS8618 nullability warning since upgrade to EF Core 7 #29582

Closed
tobiaswatzek opened this issue Nov 16, 2022 · 23 comments

Comments

@tobiaswatzek
Copy link

Ask a question

I hava a project which generates models and a DbContext using the dotnet ef dbcontext scaffold command. This worked great until today when it was upgraded from .NET 6 and EF Core 6 to use .NET 7 and EF Core 7. Now the generated DbContext no longer initializes the DbSet properties to null! (see #26877) and we get CS8618 nullability warnings when compiling. These break our build because we have TreatWarningsAsErrors set to true for our projects.

I tried to add the Microsoft.EntityFrameworkCore.Analyzers package to the project which as I understand should include a new analyzer that suppresses these warnings (see #26795). Unfortunately, this did not change anything.

My workaround for now is to suppress the CS8618 warning for the whole project that includes the generated DbContext.

Is there anything I am missing or is there a bug somewhere in the analyzer or generated code?

Include verbose output

Output when running scaffold:

> dotnet ef dbcontext scaffold -f -p "../ReadOnly" -c "ReadOnlyContext" --no-onconfiguring "Name=ConnectionStrings:DataContext" "Microsoft.EntityFrameworkCore.SqlServer"
Build started...
Build succeeded.

Output when running dotnet build (full paths and unrelated projects removed):

>dotnet build
MSBuild version 17.4.0+18d5aef85 for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
...
ReadOnlyContext.cs(9,12): error CS8618: Non-nullable property 'Bookmarks' must contain
 a non-null value when exiting constructor. Consider declaring the property as nullable. [ReadOnly.csproj]

Include provider and version information

EF Core version: 7.0.0
Database provider: Microsoft.EntityFrameworkCore.SqlServer 7.0.0
Target framework: .NET 7.0
Operating system: Windows 10 Enterprise
IDE: -

@ajcvickers
Copy link
Member

Are you using any kind of IDE (Visual Studio, VS Code, Rider, etc.)?

@tobiaswatzek
Copy link
Author

Yes, I tried to build the affected project in Rider and in Visual Studio 2022, but I executed all of these commands in a new sandbox project that I created using only the dotnet CLI before I wrote the question. I only opened the csproj file once in VS Code to add <TreatWarningsAsErrors>true</TreatWarningsAsErrors>.

My steps were:

> dotnet new sln
The template "Solution File" was created successfully.

> dotnet new tool-manifest
The template "Dotnet local tool manifest file" was created successfully.

> dotnet tool install dotnet-ef
You can invoke the tool from this directory using the following commands: 'dotnet tool run dotnet-ef' or 'dotnet dotnet-ef'.
Tool 'dotnet-ef' (version '7.0.0') was successfully installed. Entry is added to the manifest file C:\...\.config\dotnet-tools.json.

> dotnet new classlib -n ReadOnly
The template "Class Library" was created successfully.
Processing post-creation actions...
Restoring C:\...\ReadOnly\ReadOnly.csproj:
  Determining projects to restore...
  Restored C:\...\ReadOnly\ReadOnly.csproj (in 75 ms).
Restore succeeded.

> dotnet sln add .\ReadOnly\ReadOnly.csproj
Project `ReadOnly\ReadOnly.csproj` added to the solution.

> cd .\ReadOnly
> dotnet add package Microsoft.EntityFrameworkCore.Design
Determining projects to restore...
...

> dotnet add package Microsoft.EntityFrameworkCore.SqlServer
Determining projects to restore...
...

> dotnet add package Microsoft.EntityFrameworkCore.Analyzers
Determining projects to restore...
...

Here I opened the ReadOnly.csproj and added <TreatWarningsAsErrors>true</TreatWarningsAsErrors>.

> dotnet ef dbcontext scaffold -f -p "." -c "ReadOnlyContext" --no-onconfiguring "[CONNECTION_STRING]" "Microsoft.EntityFrameworkCore.SqlServer"
Build started...
Build succeeded.

>  dotnet build
MSBuild version 17.4.0+18d5aef85 for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
C:\...\ReadOnly\ReadOnlyContext.cs(9,12): error CS8618: Non-nullable property 'Bookmarks' must contain a n
on-null value when exiting constructor. Consider declaring the property as nullable. [C:\...\ReadOnly\ReadOnly.
csproj]
...
Build FAILED.

@roji
Copy link
Member

roji commented Nov 16, 2022

@tobiaswatzek can you please share your project so we can see the error happening? If you're rather share it privately, my email is on my github profile.

@tobiaswatzek
Copy link
Author

tobiaswatzek commented Nov 16, 2022

I've created a repo with the project https://github.com/tobiaswatzek/efcore-29582. This repo includes the scaffolded context I created as described in my comment above. I had to remove some of the models for confidentiality reasons, but the issue remains reproducible. Additionally, I added a global.json file to pin the SDK version to 7.0.100.

@roji roji self-assigned this Nov 16, 2022
@ErikEJ
Copy link
Contributor

ErikEJ commented Nov 16, 2022

So this suddenly bubbles up many warnings that are otherwise not shown !?

 <TreatWarningsAsErrors>true</TreatWarningsAsErrors>

@ajcvickers
Copy link
Member

Note for triage: confirmed that the suppressor doesn't work when the context has a constructor and TreatWarningsAsErrors is used.

@roji
Copy link
Member

roji commented Nov 18, 2022

I'll be investigating this soon.

@roji
Copy link
Member

roji commented Nov 22, 2022

@ajcvickers @tobiaswatzek can you please try running dotnet build-server shutdown and then dotnet build on the command-line (avoid GUIs just for this test)?

I also encountered the warning at first, but it disappeared after doing the above: an uninitialized DbSet property on a DbContext doesn't generate a warning/error, while an uninitialized string one does. I suspect this is because there was lingering state from the EF Core 6 analyzer package or similar (see #19597 (comment)). If so, this is simply a temporary transition problem from 6 to 7.

Re <TreatWarningsAsErrors>, there shouldn't be anything special here; if the diagnostic is emitted (because the suppressor isn't working), it should be either a warning or an error based on this setting.

@ajcvickers
Copy link
Member

@roji I created a brand new console app. Built it; error. Shutdown the build server and built again; error.

PowerShell 7.3.0
PS C:\Users\ajcvi> cd C:\local\code\ConsoleApp1\ConsoleApp1\
PS C:\local\code\ConsoleApp1\ConsoleApp1> dotnet build
MSBuild version 17.4.0+18d5aef85 for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
C:\local\code\ConsoleApp1\ConsoleApp1\Program.cs(14,12): error CS8618: Non-nullable property 'Blogs' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. [C:\local\code\Co
nsoleApp1\ConsoleApp1\ConsoleApp1.csproj]

Build FAILED.

C:\local\code\ConsoleApp1\ConsoleApp1\Program.cs(14,12): error CS8618: Non-nullable property 'Blogs' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. [C:\local\code\Co
nsoleApp1\ConsoleApp1\ConsoleApp1.csproj]
    0 Warning(s)
    1 Error(s)

Time Elapsed 00:00:02.00
PS C:\local\code\ConsoleApp1\ConsoleApp1> dotnet build-server shutdown
Shutting down MSBuild server...
Shutting down VB/C# compiler server...
VB/C# compiler server shut down successfully.
MSBuild server shut down successfully.
PS C:\local\code\ConsoleApp1\ConsoleApp1> dotnet build
MSBuild version 17.4.0+18d5aef85 for .NET
  Determining projects to restore...
  All projects are up-to-date for restore.
C:\local\code\ConsoleApp1\ConsoleApp1\Program.cs(14,12): error CS8618: Non-nullable property 'Blogs' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. [C:\local\code\Co
nsoleApp1\ConsoleApp1\ConsoleApp1.csproj]

Build FAILED.

C:\local\code\ConsoleApp1\ConsoleApp1\Program.cs(14,12): error CS8618: Non-nullable property 'Blogs' must contain a non-null value when exiting constructor. Consider declaring the property as nullable. [C:\local\code\Co
nsoleApp1\ConsoleApp1\ConsoleApp1.csproj]
    0 Warning(s)
    1 Error(s)

Time Elapsed 00:00:01.95
PS C:\local\code\ConsoleApp1\ConsoleApp1>
<Project Sdk="Microsoft.NET.Sdk">

    <PropertyGroup>
        <OutputType>Exe</OutputType>
        <TargetFramework>net7.0</TargetFramework>
        <ImplicitUsings>enable</ImplicitUsings>
        <Nullable>enable</Nullable>
        <TreatWarningsAsErrors>true</TreatWarningsAsErrors>
    </PropertyGroup>

    <ItemGroup>
        <PackageReference Include="Microsoft.EntityFrameworkCore.SqlServer" Version="7.0.0" />
    </ItemGroup>
    
</Project>
// See https://aka.ms/new-console-template for more information

using Microsoft.EntityFrameworkCore;

Console.WriteLine("Hello, World!");

public class Blog
{
    public int Id { get; set; }
}

public class MyContext : DbContext
{
    public MyContext(DbContextOptions<MyContext> options)
        : base(options)
    {
    }

    public DbSet<Blog> Blogs { get; set; }
}

@roji
Copy link
Member

roji commented Nov 23, 2022

Ah, I see now... The suppressor does work without TreatWarningsAsErrors (no warning emitted), but with TreatWarningsAsErrors the error isn't suppressed.

@mavasani is it possible to get your input here? In a nutshell, EF Core 7.0 has UninitializedDbSetDiagnosticSuppressor delivered via Microsoft.EntityFrameworkCore.Analyzers, which suppresses the uninitialized non-nullable property warning (CS8618) for properties of type DbSet on types inheriting from DbContext. Everything is working fine when TreatWarningsAsErrors is off (no warning), but with TreatWarningsAsErrors enabled the build fails with an unsuppressed error.

I've seen various issues where TreatWarningsAsErrors is mentioned, but AFAIK, CS8618 should be suppressible (according to these criteria).

To repro, use @ajcvickers minimal console program above; the Blogs property shouldn't generate CS8618 (but e.g. an uninitialized string property alongside it should).

@iqmeta
Copy link

iqmeta commented Nov 24, 2022

same here... after move to 7

image

image

for now
image

@roji
Copy link
Member

roji commented Dec 3, 2022

@mavasani @sharwell is it possible to get your input on #29582 (comment) (or can you refer me to the right person)?

@channeladam
Copy link

Just tried upgrading to EF Core 7 and ran into the same issue... :(

@Neme12
Copy link

Neme12 commented Dec 15, 2022

TBH, I think a better solution than using a diagnostic suppressor would be to mark the DbSet properties as required and add [SetsRequiredMembers] to the base DbContext constructors.

@roji
Copy link
Member

roji commented Dec 15, 2022

@Neme12 that's possible, but it's something each and every user of EF Core would have to do on their own DbContext type. The point of the suppressor is to do it once for all DbSet properties on all DbContext types, everywhere.

@mavasani
Copy link

@roji I was able to repro this and ran it under the debugger and it seems like an issue in the suppressor. I noted the following:

  1. Regardless of the TreatWarningsAsErrors setting, the analyzer driver is invoking DiagnosticSuppressor.ReportSuppressions callback into the suppressor
  2. When TreatWarningsAsErrors is false, the suppressor invokes SuppressionAnalysisContext.ReportSuppression. However, when this property is true the suppressor never invokes this method to register a suppression.

@mavasani
Copy link

mavasani commented Dec 16, 2022

Ah, I looked at the source code for the suppressor and figured out it is looking for an additional location on the diagnostic. The reported diagnostic seems to have an additional location when it is warning, but when bumped to an error with /warnaserror, the diagnostic doesn't have any additional locations - this seems to be the bug on the compiler side then.

// CS8618 contains the location of the uninitialized property in AdditionalLocations; note that if the class has a constructor,
// the diagnostic main Location points to the constructor rather than to the uninitialized property.
// The AdditionalLocations was added in 7.0.0-preview.3, fall back to the main location just in case and older compiler is
// being used (the check below for PropertyDeclarationSyntax will filter out the diagnostic if it's pointing to a constructor).
var location = diagnostic.AdditionalLocations.Count > 0
? diagnostic.AdditionalLocations[0]
: diagnostic.Location;

@roji
Copy link
Member

roji commented Dec 16, 2022

Thanks @mavasani, here's the PR which introduced the additional location on the compiler side: dotnet/roslyn#58096.

Should I open an issue for this on the compiler side?

@mavasani
Copy link

@roji I am doing it now - I was able to write a compiler unit test showing the bug.

@roji
Copy link
Member

roji commented Dec 16, 2022

Blocked on dotnet/roslyn#66037

@mavasani
Copy link

@roji FYI: The core roslyn fix for this got merged into VS2022 17.5 release branch: dotnet/roslyn#66280

@roji
Copy link
Member

roji commented Jan 31, 2023

Great news, thanks @mavasani!

@asleire
Copy link

asleire commented Jul 25, 2023

In case Google leads anyone else here, this issue is still present when using Rider: https://youtrack.jetbrains.com/issue/RIDER-45021/Support-Roslyn-based-suppressors

@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 11, 2023
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

9 participants