From 3b949e773ff1b4c02f8aebcab434c0f989858c0d 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 (cherry picked from commit 3cae7a805033a120a0188aa8a7b7953962cb790a) --- .../Internal/ExpressionTreeFuncletizer.cs | 21 +++++++++++++++---- .../Query/NorthwindWhereQueryCosmosTest.cs | 12 +++++++++++ .../Query/NorthwindWhereQueryTestBase.cs | 15 +++++++++++-- .../Query/NorthwindWhereQuerySqlServerTest.cs | 18 ++++++++++++++-- 4 files changed, 58 insertions(+), 8 deletions(-) diff --git a/src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs b/src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs index d301f12df10..7f48c98ca91 100644 --- a/src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs +++ b/src/EFCore/Query/Internal/ExpressionTreeFuncletizer.cs @@ -100,6 +100,9 @@ public class ExpressionTreeFuncletizer : ExpressionVisitor private static readonly IReadOnlySet EmptyStringSet = new HashSet(); + private static readonly bool UseOldBehavior35095 = + AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue35095", out var enabled35095) && enabled35095; + private static readonly MethodInfo ReadOnlyCollectionIndexerGetter = typeof(ReadOnlyCollection).GetProperties() .Single(p => p.GetIndexParameters() is { Length: 1 } indexParameters && indexParameters[0].ParameterType == typeof(int)).GetMethod!; @@ -379,17 +382,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 UseOldBehavior35095 ? returnValue : ConvertIfNeeded(returnValue, binary.Type); + case ExpressionType.OrElse or ExpressionType.AndAlso when Evaluate(left) is bool leftBoolValue: { left = Constant(leftBoolValue); @@ -511,9 +520,10 @@ 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 + var returnValue = testBoolValue ? Visit(conditional.IfTrue, out _state) : Visit(conditional.IfFalse, out _state); + return UseOldBehavior35095 ? returnValue : ConvertIfNeeded(returnValue, conditional.Type); } var ifTrue = Visit(conditional.IfTrue, out var ifTrueState); @@ -2101,6 +2111,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 c1de5a6b834..dbf562f66e0 100644 --- a/test/EFCore.Specification.Tests/Query/NorthwindWhereQueryTestBase.cs +++ b/test/EFCore.Specification.Tests/Query/NorthwindWhereQueryTestBase.cs @@ -2536,7 +2536,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))] @@ -2559,5 +2570,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 132b41e85bb..437ba3f5b4f 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);