Don´t emit CA1849 when using DbSet.Add and DbSet.AddRange EntityFramework Methods

[Spacefish]

fixes dotnet/efcore#31431

Ignores DbSet.Add and DbSet.AddRange in CA1849 analyzer as well..

This currently generated a false positive if you call for example Add on a DbSet

myDbContext.Users.Add(new User("John Doe"));

[Spacefish]

Related to #6719
@buyaa-n

[codecov]

Codecov Report

Merging #6858 (5c58d6d) into main (2af4cd8) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6858   +/-   ##
=======================================
  Coverage   96.39%   96.39%           
=======================================
  Files        1403     1403           
  Lines      330954   330977   +23     
  Branches    10889    10890    +1     
=======================================
+ Hits       319031   319056   +25     
+ Misses       9189     9187    -2     
  Partials     2734     2734           

[roji]

@Spacefish I don't think it makes sense to embed exceptions for specific EF types in the analyzers themselves; that definitely doesn't scale to the various libraries out there. An EF diagnostics suppressor is probably the way to do this (let's continue the conversation in dotnet/efcore#31431).

I'm actually surprised to see EF already hard-coded here - is this an accepted approach in the Roslyn analzyers?

[Spacefish]

@roji there was cold sweat on my face, when i saw this at first as well (hardcoded exception for EF.DataContext). But there are a lot of these Library specific Exceptions in here if you look at the WellKnownTypeNames.cs file.. There are even Exceptions for non Microsoft libs like Newtonsoft*. So i guess: Yes it seems to be an accepted approach in roslyn-analyzers.

But I am not qualified to answer that question, i am just a random guy who happened to get a lot of these warnings when updating to .NET 8 Preview 7 and tried to fix it for everyone :)

I first thought it would be better to have this exceptions in the external library (which it is). But if you look at a typical developer. You would have to force them to update the external library if they update the target framework of their project for this to work.. So i guess it´s an acceptable (if not good) solution to do it like that, for common libraries.

Otherwise you would have to introduce some sort of library specific extension Framework which can dynamically add Analyzer rules which can be updated independently from the library.. But that would be quite overkill.

[buyaa-n]

An EF diagnostics suppressor is probably the way to do this

Could explain more about this and how it works?

I'm actually surprised to see EF already hard-coded here - is this an accepted approach in the Roslyn analzyers?

Hard coding types in WellKnownTypeNames.cs and loading APIs at runtime is quite common in this repo, especially in case there is a few APIs that needs to be handled differently.

Note dotnet/efcore#30957 which is the long-term solution here, i.e. obsolete AddAsync altogether.

The fix LGTM, I would like to merge it in order to avoid false positives introduced in 8.0, is the obsoletion going to happen soon and/or if that will be enough to resolve it (I mean what user code referencing the earlier version that is not obsoleted)?

[roji]

An EF diagnostics suppressor is probably the way to do this

Could explain more about this and how it works?

Sure! Roslyn has support for the lesser-known DiagnosticsSuppressor, alongside the more well-known analyzer (which produces diagnostics) and source generators. A suppressor is precisely what this is about - it's about letting a package selectively suppress diagnostics generated by some analyzer in a very specific context.

For example, EF Core already has a suppressor for suppressing uninitialized DbSet properties. I think a suppressor for the DbSet.Add warning would fit nicely alongside it, and would keep things inside the EF context... But if it's common and OK to add stuff like this into the analyzers themselves, I guess that's OK too.

Note dotnet/efcore#30957 which is the long-term solution here, i.e. obsolete AddAsync altogether.

The fix LGTM, I would like to merge it in order to avoid false negatives introduced in 8.0, is the obsoletion going to happen soon and/or if that will be enough to resolve it (I mean what user code referencing the earlier version that is not obsoleted)?

Yeah, no - we're definitely not doing the obsoletion for 8.0, as it requires non-trivial work to redo how value generators work, so that AddAsync is obviated.

If possible, please let me just confirm on the team that we're OK with this suppression (I think we are, just want to make sure).

/cc @ajcvickers

[roji]

Confirmed with @ajcvickers and from our side we're OK with suppressing this (regardless of where).

[buyaa-n]

I think a suppressor for the DbSet.Add warning would fit nicely alongside it, and would keep things inside the EF context...

Thanks for explanation, agree it was better to stay within EF context, I wish we knew that while resolving #6684, now as EF context is already added in the repo for EntityFrameworkCore.DbContext I am OK with adding fix for EntityFrameworkCore.DbSet. Looks the question is how soon a suppressor for DbSet can be added into EF Core and suppress the 8.0 warnings. It's up to you @roji where the fix would/should go

Potentially we could also add a suppressor for DbContext and remove the workaround here.

[Spacefish]

Potentially we could also add a suppressor for DbContext and remove the workaround here.

The problem with that: If a project uses the newer SDK for building, it will get Analyzer warnings, if it doesn´t update EF-Core as well.
As DbSets are used pretty widely i would like to see this Excluded in the Analyzer, but add supressions in EF as well.. Then remove the analyzer EF specific code here in a later .NET release (10.x for example).

That way the impact on developers / projects is minimal. Otherwise 100th of developers would probably see the warnings, maybe even replace .Add with .AddAsync which then get´s deprecated (as planed) in a newer EF release and they have to refactor it again..

[roji]

The problem with that: If a project uses the newer SDK for building, it will get Analyzer warnings, if it doesn´t update EF-Core as well.

That's a pretty good point.

Otherwise 100th of developers would probably see the warnings, maybe even replace .Add with .AddAsync which then get´s deprecated (as planed) in a newer EF release and they have to refactor it again..

Yeah, that's my main motivation for wanting this to get suppressed as well (otherwise moving to AddAsync isn't itself that problematic).

Bottom line, LGTM.

[buyaa-n]

LGTM, thank you @Spacefish, as it is .NET 8 RC2 timeframe its need management approval, I will work on getting approval before merge

[jeffhandley]

I agree with the comments that state:

• It's unfortunate that the fix for this is to augment the analyzer code to exclude the false-positive API matches where the library doesn't want to promote the async method
• It's currently acceptable to fix it this way, as we need the fix to stay local within the analyzer without requiring the library make a change and the user updates it
• We don't want to produce this false-positive in .NET 8

The code changes look good to me given the agreed-upon approach for the fix. Thank you, @Spacefish!

This is approved for .NET 8 RC2, as it's a broken scenario for a new analyzer in .NET 8.

In addition to getting this fix in though, we need to make sure the documentation for CA1849 gives instructions for how best to suppress diagnostics project-wide for specific APIs. In other words, if we didn't fix this, how would we instruct users to suppress all DbSet.Add and DbSet.AddRange calls from getting this diagnostic without having to scatter #pragma statements all through their code? This documentation will serve users who hit these types of diagnostics with other libraries and APIs where the library author says, "Don't switch to the Async method; it's not better for you."

[roji]

Thanks @jeffhandley!

[buyaa-n]

In addition to getting this fix in though, we need to make sure the documentation for CA1849 gives instructions for how best to suppress diagnostics project-wide for specific APIs. In other words, if we didn't fix this, how would we instruct users to suppress all DbSet.Add and DbSet.AddRange calls from getting this diagnostic without having to scatter #pragma statements all through their code

Good point, the docs has instructions on how to suppress diagnostics project-wide, but without an option for specific APis, just a standard suppression instruction same for all analyzer docs: https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/quality-rules/ca1849#suppress-a-warning

In order to exclude DbSet.Add and DbSet.AddRange specifically the rule need to be configurable https://learn.microsoft.com/en-us/dotnet/fundamentals/code-analysis/code-quality-rule-options#excluded_type_names_with_derived_types which also needs a code change in the analyzer, created an issue for that