Add missing Converts when simplifying in funcletizer (#35122)

Fixes #35095

Author: roji

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

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)
 """);
             });
 

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<Order>().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<Customer>().Select(c => c.ContactTitle).OrderBy(t => t).Take(3).Distinct());
 
-    #endregion Evaluation order of predicates
+    #endregion Evaluation order of operators
 }

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