From 3cae7a805033a120a0188aa8a7b7953962cb790a Mon Sep 17 00:00:00 2001 From: Shay Rojansky Date: Mon, 25 Nov 2024 10:50:41 +0100 Subject: [PATCH] Add missing Converts when simplifying in funcletizer (#35122) Fixes #35095 --- .../Internal/ExpressionTreeFuncletizer.cs | 32 ++++++++++++------- .../Query/NorthwindWhereQueryCosmosTest.cs | 12 +++++++ .../Query/NorthwindWhereQueryTestBase.cs | 15 +++++++-- .../Query/NorthwindWhereQuerySqlServerTest.cs | 18 +++++++++-- 4 files changed, 61 insertions(+), 16 deletions(-) diff --git a/src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs b/src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs index 1486ce90137..7395913e44b 100644 --- a/src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs +++ b/src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs @@ -367,17 +367,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); @@ -499,9 +505,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); @@ -1937,12 +1945,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) { @@ -2103,6 +2108,9 @@ static Expression RemoveConvert(Expression expression) } } + private static Expression ConvertIfNeeded(Expression expression, Type type) + => expression.Type == type ? expression : Convert(expression, type); + private bool IsGenerallyEvaluatable(Expression expression) => _evaluatableExpressionFilter.IsEvaluatableExpression(expression, _model) && (_parameterize diff --git a/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindWhereQueryCosmosTest.cs b/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindWhereQueryCosmosTest.cs index 81298094b74..93ac1555485 100644 --- a/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindWhereQueryCosmosTest.cs +++ b/test/EFCore.Cosmos.FunctionalTests/Query/NorthwindWhereQueryCosmosTest.cs @@ -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) """); }); diff --git a/test/EFCore.Specification.Tests/Query/NorthwindWhereQueryTestBase.cs b/test/EFCore.Specification.Tests/Query/NorthwindWhereQueryTestBase.cs index 8aa37e57a37..b048b555010 100644 --- a/test/EFCore.Specification.Tests/Query/NorthwindWhereQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/NorthwindWhereQueryTestBase.cs @@ -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().Where(o => o.OrderID == (orderId ?? 0))); + } + + #region Evaluation order of operators [ConditionalTheory] [MemberData(nameof(IsAsyncData))] @@ -2558,5 +2569,5 @@ public virtual Task Take_and_Distinct_evaluation_order(bool async) async, ss => ss.Set().Select(c => c.ContactTitle).OrderBy(t => t).Take(3).Distinct()); - #endregion Evaluation order of predicates + #endregion Evaluation order of operators } diff --git a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs index c633e0dedbf..24602ceec9b 100644 --- a/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs +++ b/test/EFCore.SqlServer.FunctionalTests/Query/NorthwindWhereQuerySqlServerTest.cs @@ -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) { @@ -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);