Add CS8602 suppressor

[Youssef1313]

The actual implementation (which is still incomplete) is currently commented out (for testing purposes).

On its current form, it should suppress every CS8602 warning. However, the test reports CS8602.

@sharwell Is this a test framework issue? or roslyn issue? or something I'm doing wrong in the test? (Fixed)

@roji Can you review? This currently checks for expression trees where the invoked method is in Microsoft.EntityFrameworkCore.EntityFrameworkQueryableExtensions. Are there any other methods I need to handle?

Fixes #21663

[roji]

This looks like it checks only for operations directly within an expression lambda. Consider the following:

_ = await ctx.Blogs
    .Where(b => b.Posts.Any(p => p.Title.StartsWith("A")))
    .ToListAsync();

p.Title.StartsWith may generate a nullability warning, but the Any it's in is on Posts, which is an enumerable, not a queryable. To catch this you need to go further up the tree; this may not be trivial to do correctly, while keeping analyzer perf in mind.

[Youssef1313]

@roji I haven't used ASP.NET and EF for long, so I might be wrong, but I think Where in the above snippet is from System.Linq and there is no expression tree involved, is that correct?
If that's correct, how it's guaranteed there will be no exception thrown if expression trees aren't involved?

[roji]

The Where above is indeed Queryable.Where, which means it accepts an expression tree as a parameter. However, note that b.Posts (the navigation property) is not a DbSet/IQueryable, but rather an ordinary List (or similar); this means that Any is Enumerable.Any, not Queryable.Any.

The main point here is that EF Core ensures that anything within the Expression tree would be null-safe; for example, if there's any Post with a null title, the above wouldn't throw an NRE when executed with EF Core (although it would definitely throw if executed in Linq to Objects). This suppressor would ideally identify this by walking all the way up from the warning; if it finds an EF Core DbSet, it knows that the warning can be suppressed.

[roji]

nit: empty lines

[Youssef1313]

Suggested change

[roji]

As I wrote in #21663 (comment), this won't take into account the common case of queryable operators, which aren't on EntityFrameworkQueryableExtensions (e.g. Where). To handle those reliably, rather than checking the queryable operators themselves, you'd need to go up the tree to make sure the tree is rooted on an EF Core DbSet.

[roji]

@Youssef1313 please hold off on updating this PR - there is disagreement in the team on whether EF should be responsible for suppressing the warnings in the first place. I'll post an update here as a decision is made.

[Youssef1313]

Temporarily converting to draft until I get back to it.

[roji]

@Youssef1313 do you intend to pick up this work? We typically prefer not to have PRs open for too long - you can always resubmit a PR later if you want.

[Youssef1313]

@roji I haven't picked the PR per your comment #24151 (comment). Has a decision been made?

Edit: Oh sorry, I see you commented on the original issue, will work on this again. Thanks for pinging me

[roji]

@Youssef1313 am going over old PRs - I apologize for my part in not moving this faster than I could... Are you still interested in working on this (see specifically my comment on walking up the tree)? The window for merging new features into the 6.0 release is almost closed, so this will almost certainly not make it in (we're also heads down stabilizing the release so I'm unlikely to have lots of time to discuss and review in the next couple of months).

On the up side, I'm hoping to work on a mini-wave of analyzer/suppressor tasks in EF Core 7.0 (nothing confirmed yet), so this could fit in nicely. Would you prefer we close this PR for now and pick it up a bit later?

[roji]

Suggested change

	<PackageReference Include="Microsoft.Extensions.DependencyModel" Version="$(MicrosoftExtensionsDependencyModelVersion)" />
	<PackageReference Include="Microsoft.Extensions.DependencyModel" Version="$(MicrosoftExtensionsDependencyModelVersion)" />

[roji]

The Where above is indeed Queryable.Where, which means it accepts an expression tree as a parameter. However, note that b.Posts (the navigation property) is not a DbSet/IQueryable, but rather an ordinary List (or similar); this means that Any is Enumerable.Any, not Queryable.Any.

The main point here is that EF Core ensures that anything within the Expression tree would be null-safe; for example, if there's any Post with a null title, the above wouldn't throw an NRE when executed with EF Core (although it would definitely throw if executed in Linq to Objects). This suppressor would ideally identify this by walking all the way up from the warning; if it finds an EF Core DbSet, it knows that the warning can be suppressed.

[Youssef1313]

On the up side, I'm hoping to work on a mini-wave of analyzer/suppressor tasks in EF Core 7.0 (nothing confirmed yet), so this could fit in nicely. Would you prefer we close this PR for now and pick it up a bit later?

Closing. Will reopen later.