Skip to content

Fix null check optimization for IQueryable/DbSet types in LINQ queries #36986

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.

Sign up for GitHub

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+98 −1
Draft

Fix null check optimization for IQueryable/DbSet types in LINQ queries #36986

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3a23a86
Initial plan
Copilot Oct 16, 2025
b9a9722
Fix null check optimization for IQueryable/DbSet types
Copilot Oct 16, 2025
3de19b9
Add additional test for IQueryable == null scenario
Copilot Oct 16, 2025
e36862c
Fix on top of Copilot's work.
cincuranet Oct 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 31 additions & 1 deletion src/EFCore/Query/Internal/NullCheckRemovingExpressionVisitor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ protected override Expression VisitBinary(BinaryExpression binaryExpression)
{
var visitedExpression = base.VisitBinary(binaryExpression);

return TryOptimizeConditionalEquality(visitedExpression) ?? visitedExpression;
return TryOptimizeConditionalEquality(visitedExpression)
?? TryOptimizeQueryableNullCheck(visitedExpression)
?? visitedExpression;
}

/// <summary>
Expand Down Expand Up @@ -115,6 +117,34 @@ protected override Expression VisitConditional(ConditionalExpression conditional
return null;
}

private static Expression? TryOptimizeQueryableNullCheck(Expression expression)
{
// Optimize IQueryable/DbSet null checks:
// * IQueryable != null => true
// * IQueryable == null => false
if (expression is BinaryExpression
{
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 •
edited
Loading

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 isRightNull = IsNullConstant(binaryExpression.Right);

if (isLeftNull != isRightNull)
{
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 •
edited
Loading

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();

{
var result = binaryExpression.NodeType == ExpressionType.NotEqual;
return Expression.Constant(result);
}
}
}

return null;
}

private sealed class NullSafeAccessVerifyingExpressionVisitor : ExpressionVisitor
{
private readonly ISet<Expression> _nullSafeAccesses = new HashSet<Expression>(ExpressionEqualityComparer.Instance);
Expand Down
16 changes: 16 additions & 0 deletions test/EFCore.Cosmos.FunctionalTests/Query/NorthwindWhereQueryCosmosTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1699,6 +1699,22 @@ public override async Task Where_Queryable_AsEnumerable_Contains_negated(bool as
AssertSql();
}

public override async Task Where_Queryable_not_null_check_with_Contains(bool async)
{
// Cosmos client evaluation. Issue #17246.
await AssertTranslationFailed(() => base.Where_Queryable_not_null_check_with_Contains(async));

AssertSql();
}

public override async Task Where_Queryable_null_check_with_Contains(bool async)
{
// Cosmos client evaluation. Issue #17246.
await AssertTranslationFailed(() => base.Where_Queryable_null_check_with_Contains(async));

AssertSql();
}

public override Task Where_list_object_contains_over_value_type(bool async)
=> Fixture.NoSyncTest(
async, async a =>
Expand Down
21 changes: 21 additions & 0 deletions test/EFCore.Specification.Tests/Query/NorthwindWhereQueryTestBase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1438,6 +1438,27 @@ public virtual Task Where_Queryable_ToArray_Length_member(bool async)
assertOrder: true,
elementAsserter: (e, a) => AssertCollection(e, a));

[ConditionalTheory, MemberData(nameof(IsAsyncData))]
public virtual Task Where_Queryable_not_null_check_with_Contains(bool async)
=> AssertQuery(
async,
ss =>
{
var ids = ss.Set<Customer>().Select(c => c.CustomerID);
return ss.Set<Customer>().Where(c => ids != null && ids.Contains(c.CustomerID));
});

[ConditionalTheory, MemberData(nameof(IsAsyncData))]
public virtual Task Where_Queryable_null_check_with_Contains(bool async)
=> AssertQuery(
async,
ss =>
{
var ids = ss.Set<Customer>().Select(c => c.CustomerID);
return ss.Set<Customer>().Where(c => ids == null || !ids.Contains(c.CustomerID));
},
assertEmpty: true);

[ConditionalTheory, MemberData(nameof(IsAsyncData))]
public virtual Task Where_collection_navigation_ToList_Count(bool async)
=> AssertQuery(
Expand Down
30 changes: 30 additions & 0 deletions test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1890,6 +1890,36 @@ ORDER BY [c].[CustomerID]
""");
}

public override async Task Where_Queryable_not_null_check_with_Contains(bool async)
{
await base.Where_Queryable_not_null_check_with_Contains(async);

AssertSql(
"""
SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
FROM [Customers] AS [c]
WHERE [c].[CustomerID] IN (
SELECT [c0].[CustomerID]
FROM [Customers] AS [c0]
)
""");
}

public override async Task Where_Queryable_null_check_with_Contains(bool async)
{
await base.Where_Queryable_null_check_with_Contains(async);

AssertSql(
"""
SELECT [c].[CustomerID], [c].[Address], [c].[City], [c].[CompanyName], [c].[ContactName], [c].[ContactTitle], [c].[Country], [c].[Fax], [c].[Phone], [c].[PostalCode], [c].[Region]
FROM [Customers] AS [c]
WHERE [c].[CustomerID] NOT IN (
SELECT [c0].[CustomerID]
FROM [Customers] AS [c0]
)
""");
}

public override async Task Where_collection_navigation_ToList_Count(bool async)
{
await base.Where_collection_navigation_ToList_Count(async);
Expand Down