Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 20 additions & 12 deletions src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -379,17 +379,23 @@ protected override Expression VisitBinary(BinaryExpression binary)
case ExpressionType.Coalesce:
leftValue = Evaluate(left);

Expression returnValue;
switch (leftValue)
{
case null:
return Visit(binary.Right, out _state);
returnValue = Visit(binary.Right, out _state);
break;
case bool b:
_state = leftState with { StateType = StateType.EvaluatableWithoutCapturedVariable };
return Constant(b);
returnValue = Constant(b);
break;
default:
return left;
returnValue = left;
break;
}

return ConvertIfNeeded(returnValue, binary.Type);

case ExpressionType.OrElse or ExpressionType.AndAlso when Evaluate(left) is bool leftBoolValue:
{
left = Constant(leftBoolValue);
Expand Down Expand Up @@ -511,9 +517,11 @@ protected override Expression VisitConditional(ConditionalExpression conditional
// If the test evaluates, simplify the conditional away by bubbling up the leg that remains
if (testState.IsEvaluatable && Evaluate(test) is bool testBoolValue)
{
return testBoolValue
? Visit(conditional.IfTrue, out _state)
: Visit(conditional.IfFalse, out _state);
return ConvertIfNeeded(
testBoolValue
? Visit(conditional.IfTrue, out _state)
: Visit(conditional.IfFalse, out _state),
conditional.Type);
}

var ifTrue = Visit(conditional.IfTrue, out var ifTrueState);
Expand Down Expand Up @@ -1935,12 +1943,9 @@ private static StateType CombineStateTypes(StateType stateType1, StateType state
return evaluatableRoot;
}

var returnType = evaluatableRoot.Type;
var constantExpression = Constant(value, value?.GetType() ?? returnType);

return constantExpression.Type != returnType
? Convert(constantExpression, returnType)
: constantExpression;
return ConvertIfNeeded(
Constant(value, value is null ? evaluatableRoot.Type : value.GetType()),
evaluatableRoot.Type);

bool TryHandleNonEvaluatableAsRoot(Expression root, State state, bool asParameter, [NotNullWhen(true)] out Expression? result)
{
Expand Down Expand Up @@ -2101,6 +2106,9 @@ static Expression RemoveConvert(Expression expression)
}
}

private static Expression ConvertIfNeeded(Expression expression, Type type)
=> expression.Type == type ? expression : Convert(expression, type);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is exact type match correct here? Should derived types be allowed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, AFAIK there are scenarios where a Convert node is indeed needed when inserting a node of type Subclass into an expected context of Superclass... In any case, the worse case here from the exact type match is an extra unneeded Convert node where it shouldn't be needed - this still needs to be handled (since the user may always insert it explicitly via a cast)... So I think it's OK..


private bool IsGenerallyEvaluatable(Expression expression)
=> _evaluatableExpressionFilter.IsEvaluatableExpression(expression, _model)
&& (_parameterize
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3303,6 +3303,18 @@ FROM root c
SELECT VALUE c["OrderID"]
FROM root c
WHERE ((c["$type"] = "Order") AND (c["OrderID"] = 10252))
""");
});

public override Task Simplifiable_coalesce_over_nullable(bool async)
=> Fixture.NoSyncTest(
async, async a =>
{
await base.Simplifiable_coalesce_over_nullable(a);

AssertSql(
"""
ReadItem(None, Order|10248)
""");
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2535,7 +2535,18 @@ await AssertQuery(
elementAsserter: (e, a) => AssertEqual(e.Id, a.Id));
}

#region Evaluation order of predicates
[ConditionalTheory] // #35095
[MemberData(nameof(IsAsyncData))]
public virtual Task Simplifiable_coalesce_over_nullable(bool async)
{
int? orderId = 10248;

return AssertQuery(
async,
ss => ss.Set<Order>().Where(o => o.OrderID == (orderId ?? 0)));
}

#region Evaluation order of operators

[ConditionalTheory]
[MemberData(nameof(IsAsyncData))]
Expand All @@ -2558,5 +2569,5 @@ public virtual Task Take_and_Distinct_evaluation_order(bool async)
async,
ss => ss.Set<Customer>().Select(c => c.ContactTitle).OrderBy(t => t).Take(3).Distinct());

#endregion Evaluation order of predicates
#endregion Evaluation order of operators
}
Original file line number Diff line number Diff line change
Expand Up @@ -3425,7 +3425,21 @@ FROM [Orders] AS [o]
""");
}

#region Evaluation order of predicates
public override async Task Simplifiable_coalesce_over_nullable(bool async)
{
await base.Simplifiable_coalesce_over_nullable(async);

AssertSql(
"""
@__p_0='10248'

SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate]
FROM [Orders] AS [o]
WHERE [o].[OrderID] = @__p_0
""");
}

#region Evaluation order of operators

public override async Task Take_and_Where_evaluation_order(bool async)
{
Expand Down Expand Up @@ -3483,7 +3497,7 @@ ORDER BY [c].[ContactTitle]
""");
}

#endregion Evaluation order of predicates
#endregion Evaluation order of operators

private void AssertSql(params string[] expected)
=> Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
Expand Down