Create DiagnosticSuppressor for CS8602 in Linq expressions and Includes

[Dreamescaper]

Currently CS8602 (Dereference of possibly null reference) is triggered for nullable properties in linq expressions, even though no NRE possible when query is executed on DB side.

Example 1 (Linq):

class User
{
    public string FirstName {get;set;}
    public string? MiddleName {get;set;}
    public string LastName {get;set;} 
}
...
string searchTerm = "...";

await dbContext.Users
    .Where(u => u => u.FirstName.Contains(searchTerm)
          || u.MiddleName.Contains(searchTerm)       <---------- CS8602 is reported, as 'MiddleName' is nullable
          || u.LastName.Contains(searchTerm))
    .ToListAsync();

Example 2 (Includes):

class SomeModel
{
    public class NavigationModel Navigation? {get;set;}
}

class NavigationModel 
{
    public class AnotherNavigationModel DeeperNavigation {get;set;}
}

dbContext.Models
   .Include(m => m.Navigation)
        .ThenInclude(n => n.DeeperNavigation)   <----- CS8602 is reported, as 'n' is nullable

Suggestion:
Create DiagnosticSuppressor for CS8602 for IQueryable expressions in Linq methods and for Include/ThenInclude methods.

[Dreamescaper]

Related to #21608

[smitpatel]

This may not be a good idea. While I understand the logic behind it, it is hard to tell if the LINQ query is being run over EF here null safety won't matter vs objects where code which is null unsafe will certainly throw an error if evaluated.

[Dreamescaper]

@smitpatel
I assume it relates to Linq case only?
WDYT about Includes? It should always be safe to suppress warnings there, right?

[roji]

I'm guessing/hoping there are some cases where we can unambiguously determine we're in an EF-evaluated expression (at least in Includes as @Dreamescaper suggested, possibly even other cases)... We can look into this.

[Dreamescaper]

For Linq case - we can try suppression only when expression starts with DbSet.
E.g.

await dbContext.Users
    .Where(u => u => u.FirstName.Contains(searchTerm)
          || u.MiddleName.Contains(searchTerm) // <---------- query starts from DbSet<User>, suppress warning
          || u.LastName.Contains(searchTerm))
    .ToListAsync();

but

IQueryable<UserProfile> SearchQuery(IQueryable<User> query, string searchTerm)
{
     return query.Where(up => up.FirstName.Contains(searchTerm)
        || up.MiddleName.Contains(searchTerm)  // <------------ can't know if IQueryable is EF Core specific, so do not suppress warning
        || up.LastName.Contains(searchTerm);
}

[ajcvickers]

Note from triage: we're not going to put any DiagnosticSuppressors in for 5.0. We should discuss post-5.0 whether this is something that we should invest in.

[Youssef1313]

@ajcvikers Are there any plans for this suppressor? I'm interested to implement it and submit a PR.

[roji]

@Youssef1313 we don't have any concrete plans for implementing this soon (because of priorities). We'd welcome a community contribution, but as this is about code analysis it's best to start off with a quick and informal spec of exactly what you plan on doing, to make sure we're all on the same page.

[Youssef1313]

@roji I'd check the diagnostic location, and borrow the following method from roslyn-analyzers repo:

https://github.com/dotnet/roslyn-analyzers/blob/7bd2c98e798042e59121961640e124f76b03fdb9/src/Utilities/Compiler/Extensions/IOperationExtensions.cs#L518-L521

and report a suppression if both of the following applies:

• IsWithinExpressionTree returns true.
• The diagnostic location is inside a method call that's known to be safe (I'll need a list of these from the team)

[roji]

The diagnostic location is inside a method call that's known to be safe (I'll need a list of these from the team)

While there are indeed a few EF-specific methods where we can just suppress (e.g. Include), the interesting cases are general IQueryable operators (e.g. Where) which aren't EF-specific. In theory, we'd verify that the operator is rooted on an EF Core DbSet, though the likelihood of another LINQ provider in the same project is very low (can the user disable the suppression in that case via .editorconfig or something?). Needless to say, diagnostics in non-queryable LINQ to Objects should not be suppressed.

@smitpatel @ajcvickers thoughts?

[Youssef1313]

(can the user disable the suppression in that case via .editorconfig or something?)

I'm not aware of that (probably @mavasani or @sharwell knows?), but even if there is a way. I think the suppressor should not suppress correct CS8602. Either it works correctly in almost all cases, or leaves CS8602 warnings that are considered incorrect warnings. But it should not hide correct warnings. (i.e. false negatives are okay, but false positives are not).

A false negative will lead to the user getting CS8602 and having to manually suppress it. But a false positive will hide a warning that shouldn't be hidden.

[roji]

I agree with that - any correct suppressions we do are useful (even if we don't cover all cases), but we shouldn't incorrectly suppress any diagnostic.

[Youssef1313]

@roji Does working with the solution require the current preview version of Visual Studio or any special requirements?

I'm getting several warnings with VS 16.8:

[ErikEJ]

Did you follow this guide? https://www.github.com/dotnet/efcore/tree/main/docs%2Fgetting-and-building-the-code.md

[roji]

Design discussion:

• If we suppress NRE warnings in EF-rooted expression trees, we risk suppressing real warnings for code that will get client-evaluated
• It is not possible to reliably identify client-evaluated code; the top-level select is only one instance, but it is possible for earlier operators to contain bits that will end up being client-evaluated as well.
• As such, we will make no attempt to identify client-evaluation, and apply the suppression to the entire EF-rooted tree.
• Since we'd be suppressing potentially valuable warnings, we want to explore opt-in/opt-out possibilities here.
• Ideally, we'd be able to suppress the warning, but emit a compilation suggestion with a clear, EF-specific text describing the situation, and directing users what to do.
• An editorconfig knob could allow removing the suggestion entirely, or removing the suppression, making the original warning show (as today).
• If it's not possible to downgrade the warning to a suggestion, then we need to reach a decision on whether the complete suppression should be the default or opt-in.

[Youssef1313]

@roji With the current Roslyn APIs, I can't see a way to have a suppression disabled by default to be opt-in. But that sounds like a reasonable feature request for Roslyn. Opened dotnet/roslyn#51278 for that in Roslyn.

[roji]

@Youssef1313 the suppressor can simply decide not to call ReportSuppression based on some flag in .editorconfig or elsewhere, no?

[roji]

Ideally, we'd be able to suppress the warning, but emit a compilation suggestion with a clear, EF-specific text describing the situation, and directing users what to do.

A quick look seems to show that a suppressor cannot report new diagnostics, nor mutate existing ones - only suppress.

[roji]

Design decision: as there's no way for a suppressor to report new diagnostics, we'll suppress these warnings in all DbSet-rooted expression trees by default. An .editorconfig opt-in should allow opting back in to the warnings, effectively disabling the suppressor.

@Youssef1313 I think we know what we want to do here - you can continue work on this.

[roji]

Actually, it seems like standard rules (editorconfig or XML) can be used to disable suppressions: https://github.com/nunit/nunit.analyzers/pull/325/files#diff-47f25205db28dc2d9aebf924f1be9ba85b2d18fbcedb7cad85710a371ec3468aR80