Skip to content

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Oct 16, 2025

Fixes #35598.

@Copilot Copilot AI changed the title [WIP] Fix comparing DbSet<T> to null in query Fix null check optimization for IQueryable/DbSet types in LINQ queries Oct 16, 2025
@Copilot Copilot AI requested a review from cincuranet October 16, 2025 07:54
Copilot finished work on behalf of cincuranet October 16, 2025 07:54
@cincuranet cincuranet force-pushed the copilot/fix-dbset-query-null-checks branch 3 times, most recently from dae8232 to ebcb425 Compare October 16, 2025 12:19
@cincuranet cincuranet force-pushed the copilot/fix-dbset-query-null-checks branch from ebcb425 to e36862c Compare October 16, 2025 12:57
@cincuranet cincuranet marked this pull request as ready for review October 16, 2025 16:27
@cincuranet cincuranet requested a review from a team as a code owner October 16, 2025 16:27
@cincuranet cincuranet requested a review from a team October 16, 2025 16:28
NodeType: ExpressionType.Equal or ExpressionType.NotEqual
} binaryExpression)
{
var isLeftNull = IsNullConstant(binaryExpression.Left);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: with pattern matching just writing out is ConstantExpression { Value: null } seems simpler, IMHO

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method IsNullConstant is already there, hence it seems reasonable to use it.

Copy link
Member

@roji roji Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm proposing to remove it, as a code modernization.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In that case I'd like to do in another PR, because that method exists in other places too.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

{
var nonNullExpression = isLeftNull ? binaryExpression.Right : binaryExpression.Left;

if (nonNullExpression.Type.IsAssignableTo(typeof(IQueryable)))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct? What happens if there's a local variable of type DbSet/IQueryable and which really does happen to be null (because of some condition in the user's code)?

(in #35598 it's asserted that this is a regression, I wonder what was the change from 8 to 9 which triggered it)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if there's a local variable of type DbSet/IQueryable and which really does happen to be null (because of some condition in the user's code)?

Can you show a code?

Copy link
Member

@roji roji Oct 16, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Something like:

using var db = new MyContext();
var ids = someFlag ? db.Items.Select(c => c.Id) : null;
var results = await db.Items.Where(r => ids != null && ids.Contains(r.Id)).ToListAsync();

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more note:

(in #35598 it's asserted that this is a regression, I wonder what was the change from 8 to 9 which triggered it)

It would be good to understand this... Apparently this scenario was working before without this special-casing, which we'd ideally not need (it's always better to avoid adding more code/special cases if possible)

@cincuranet
Copy link
Contributor

It would be good to understand this... Apparently this scenario was working before without this special-casing, which we'd ideally not need (it's always better to avoid adding more code/special cases if possible)

I don't think Copilot will be able to do this. So let me do it myself.

@cincuranet cincuranet marked this pull request as draft October 16, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Comparing DbSet<T>/IQueryable<T> to null in query fails

3 participants