Refactor projection binding to avoid redundant Convert nodes and impr…

src/EFCore.Cosmos/Query/Internal/CosmosProjectionBindingExpressionVisitor.cs

@@ -137,7 +137,7 @@ public virtual Expression Translate(SelectExpression selectExpression, Expressio
             }
 
             return new ProjectionBindingExpression(
-                _selectExpression, _selectExpression.AddToProjection(translation), expression.Type.MakeNullable());
+                _selectExpression, _selectExpression.AddToProjection(translation), translation.Type);
         }
         else
         {
@@ -149,7 +149,7 @@ public virtual Expression Translate(SelectExpression selectExpression, Expressio
 
             _projectionMapping[_projectionMembers.Peek()] = translation;
 
-            return new ProjectionBindingExpression(_selectExpression, _projectionMembers.Peek(), expression.Type.MakeNullable());
+            return new ProjectionBindingExpression(_selectExpression, _projectionMembers.Peek(), translation.Type);
         }
     }
 
@@ -793,12 +793,17 @@ private void VerifySelectExpression(ProjectionBindingExpression projectionBindin
 
     private static Expression MatchTypes(Expression expression, Type targetType)
     {
-        if (targetType != expression.Type
-            && targetType.TryGetSequenceType() == null)
+         if (targetType != expression.Type
+        && targetType.TryGetSequenceType() == null)
         {
-            Check.DebugAssert(targetType.MakeNullable() == expression.Type, "expression.Type must be nullable of targetType");
-
-            expression = Expression.Convert(expression, targetType);
+            if (expression is ProjectionBindingExpression projectionBindingExpression)
+            {
+                return projectionBindingExpression.UpdateType(targetType);
+            }
+            if (targetType.MakeNullable() == expression.Type)
+            {
+                expression = Expression.Convert(expression, targetType);
+            }
         }
 
         return expression;

src/EFCore.Relational/Query/Internal/RelationalProjectionBindingExpressionVisitor.cs

@@ -120,14 +120,8 @@ public virtual Expression Translate(SelectExpression selectExpression, Expressio
                     case ParameterExpression parameterExpression:
                         throw new InvalidOperationException(CoreStrings.TranslationFailed(parameterExpression.Print()));
 
-                    case ProjectionBindingExpression projectionBindingExpression:
-                        return _selectExpression.GetProjection(projectionBindingExpression) switch
-                        {
-                            StructuralTypeProjectionExpression projection => AddClientProjection(projection, typeof(ValueBuffer)),
-                            SqlExpression mappedSqlExpression => AddClientProjection(mappedSqlExpression, expression.Type.MakeNullable()),
-                            _ => throw new InvalidOperationException(CoreStrings.TranslationFailed(projectionBindingExpression.Print()))
-                        };
-
+                    case SqlExpression mappedSqlExpression:
+                        return AddClientProjection(mappedSqlExpression, mappedSqlExpression.Type);
                     case MaterializeCollectionNavigationExpression materializeCollectionNavigationExpression:
                         if (materializeCollectionNavigationExpression.Navigation.TargetEntityType.IsMappedToJson())
                         {
@@ -177,8 +171,23 @@ public virtual Expression Translate(SelectExpression selectExpression, Expressio
 
                 switch (_sqlTranslator.TranslateProjection(expression))
                 {
-                    case SqlExpression sqlExpression:
-                        return AddClientProjection(sqlExpression, expression.Type.MakeNullable());
+                    case SqlExpression mappedSqlExpression:
+                        var isNullable = mappedSqlExpression switch
+                        {
+                            ColumnExpression c => c.IsNullable,
+                            SqlFunctionExpression f => f.IsNullable,
+                            AtTimeZoneExpression a => a.IsNullable,
+                            _ => true
+                        };
+
+                        var shouldBeNullable = isNullable
+                            && expression.Type.IsValueType
+                            && !expression.Type.IsNullableType()
+                            && expression.Type != typeof(bool);
+
+                        return AddClientProjection(
+                            mappedSqlExpression,
+                            shouldBeNullable ? expression.Type.MakeNullable() : expression.Type);
 
                     // This handles the case of a complex type being projected out of a Select.
                     case RelationalStructuralTypeShaperExpression { StructuralType: IComplexType } shaper:
@@ -240,11 +249,23 @@ public virtual Expression Translate(SelectExpression selectExpression, Expressio
             {
                 switch (_sqlTranslator.TranslateProjection(expression))
                 {
-                    case SqlExpression sqlExpression:
-                        _projectionMapping[_projectionMembers.Peek()] = sqlExpression;
-                        return new ProjectionBindingExpression(
-                            _selectExpression, _projectionMembers.Peek(), expression.Type.MakeNullable());
+                    case SqlExpression mappedSqlExpression:
+                        _projectionMapping[_projectionMembers.Peek()] = mappedSqlExpression;
+                        var isNullable = mappedSqlExpression switch
+                        {
+                            ColumnExpression c => c.IsNullable,
+                            SqlFunctionExpression f => f.IsNullable,
+                            AtTimeZoneExpression a => a.IsNullable,
+                            _ => true
+                        };
+
+                        var shouldBeNullable = isNullable
+                            && expression.Type.IsValueType
+                            && !expression.Type.IsNullableType()
+                            && expression.Type != typeof(bool);
 
+                        return new ProjectionBindingExpression(
+                            _selectExpression, _projectionMembers.Peek(), shouldBeNullable ? expression.Type.MakeNullable() : expression.Type);
                     // This handles the case of a complex type being projected out of a Select.
                     // Note that an entity type being projected is (currently) handled differently
                     case RelationalStructuralTypeShaperExpression { StructuralType: IComplexType } shaper:
@@ -675,38 +696,69 @@ protected override Expression VisitUnary(UnaryExpression unaryExpression)
     {
         var operand = Visit(unaryExpression.Operand);
 
-        return unaryExpression.NodeType is ExpressionType.Convert or ExpressionType.ConvertChecked
-            && unaryExpression.Type == operand.Type
-                ? operand
-                : unaryExpression.Update(MatchTypes(operand, unaryExpression.Operand.Type));
+        if (unaryExpression.NodeType is ExpressionType.Convert or ExpressionType.ConvertChecked)
+        {
+            if (unaryExpression.Type == operand.Type)
+            {
+                return operand;
+            }
+
+            if (unaryExpression.Type.IsValueType
+                && !unaryExpression.Type.IsNullableType()
+                && operand.Type.IsNullableType()
+                && operand.Type.UnwrapNullableType() == unaryExpression.Type)
+            {
+                return MatchTypes(operand, unaryExpression.Type);
+            }
+        }
+
+        return unaryExpression.Update(MatchTypes(operand, unaryExpression.Operand.Type));
     }
 
     [DebuggerStepThrough]
     private static Expression MatchTypes(Expression expression, Type targetType)
-    {
-        if (targetType != expression.Type
-            && targetType.TryGetElementType(typeof(IQueryable<>)) == null)
         {
-            Check.DebugAssert(
-                targetType.MakeNullable() == expression.Type,
-                $"expression has type {expression.Type.Name}, but must be nullable over {targetType.Name}");
-
-            return expression switch
+            if (targetType != expression.Type
+                && targetType.TryGetElementType(typeof(IQueryable<>)) == null)
             {
-#pragma warning disable EF1001
-                RelationalStructuralTypeShaperExpression structuralShaper => structuralShaper.MakeClrTypeNonNullable(),
-#pragma warning restore EF1001
+                Check.DebugAssert(
+                    targetType.MakeNullable() == expression.Type,
+                    $"expression has type {expression.Type.Name}, but must be nullable over {targetType.Name}");
 
-                _ =>  Expression.Convert(expression, targetType),
-            };
-        }
+                return expression switch
+                {
+#pragma warning disable EF1001 // Internal EF Core API usage.
+                    ProjectionBindingExpression projectionBindingExpression
+                        => (projectionBindingExpression.Type.IsNullableType() && !targetType.IsNullableType())
+                            ? Expression.Coalesce(projectionBindingExpression, Expression.Default(targetType))
+                            : projectionBindingExpression.UpdateType(targetType),
+#pragma warning restore EF1001 // Internal EF Core API usage.
+                    _ => expression.Type.IsNullableType() && !targetType.IsNullableType()
+                        ? Expression.Coalesce(expression, Expression.Default(targetType))
+                        : Expression.Convert(expression, targetType)
+                };
+            }
 
-        return expression;
-    }
+            return expression;
+        }
 
     private ProjectionBindingExpression AddClientProjection(Expression expression, Type type)
     {
-        var existingIndex = _clientProjections!.FindIndex(e => e.Equals(expression));
+        // [FIX 1] Guard against null expressions resulting from failed translation.
+        // This ensures we throw the InvalidOperationException the test expects.
+        if (expression is null)
+        {
+            throw new InvalidOperationException("Unable to translate the given expression.");
+        }
+
+        // [FIX 2] Lazy initialization: If _clientProjections is null, create it.
+        // This prevents the NullReferenceException when calling .FindIndex
+        if (_clientProjections is null)
+        {
+            _clientProjections = new List<Expression>();
+        }
+
+        var existingIndex = _clientProjections.FindIndex(e => e.Equals(expression));
         if (existingIndex == -1)
         {
             _clientProjections.Add(expression);

src/EFCore.Relational/Query/RelationalShapedQueryCompilingExpressionVisitor.ShaperProcessingExpressionVisitor.cs

@@ -2903,9 +2903,10 @@ private Expression AddToCollectionStructuralProperty(
                 entity,
                 relatedEntity,
                 Constant(true));
-
         private object GetProjectionIndex(ProjectionBindingExpression projectionBindingExpression)
-            => _selectExpression.GetProjection(projectionBindingExpression).GetConstantValue<object>();
+        {
+            return _selectExpression.GetProjection(projectionBindingExpression).GetConstantValue<object>();
+        }
 
         private static bool IsNullableProjection(ProjectionExpression projection)
             => projection.Expression is not ColumnExpression column || column.IsNullable;
@@ -2918,6 +2919,10 @@ private Expression CreateGetValueExpression(
             Type type,
             IPropertyBase? property = null)
         {
+            Check.DebugAssert(
+                property != null || type.IsNullableType() || type.IsValueType, 
+                "Must read nullable value from database if property is not specified.");
+
             var getMethod = typeMapping.GetDataReaderMethod();
 
             Expression indexExpression = Constant(index);
@@ -2926,6 +2931,7 @@ private Expression CreateGetValueExpression(
                 indexExpression = ArrayIndex(_indexMapParameter, indexExpression);
             }
 
+            // القراءة الأصلية تحافظ على الـ Value Converters وتمنع خطأ DateOnly
             Expression valueExpression
                 = Call(
                     getMethod.DeclaringType != typeof(DbDataReader)
@@ -2981,16 +2987,6 @@ Expression valueExpression
             var converterExpression = default(Expression);
             if (converter != null)
             {
-                // if IProperty is available, we can reliably get the converter from the model and then incorporate FromProvider(Typed) delegate
-                // into the expression. This way we have consistent behavior between precompiled and normal queries (same code path)
-                // however, if IProperty is not available, we could try to get TypeMapping from TypeMappingSource based on ClrType, but that may
-                // return incorrect mapping. So for that case we would prefer to incorporate the FromProvider lambda, like we used to do before AOT
-                // and only resort to unreliable TypeMappingSource lookup, if the converter expression captures "forbidden" constant
-                // see issue #33517 for more details
-                // UPDATE: instead of guessing the type mapping in case where we don't have IProperty and converter uses non-literal constant,
-                // we just revert to the pre-AOT behavior, i.e. we still use converter.ConvertFromProviderExpression
-                // this will not work for precompiled query (which realistically was already broken for this scenario - type mapping we "guess"
-                // is pretty much always wrong), but regular case (not pre-compiled) will continue to work.
                 if (property != null)
                 {
                     var typeMappingExpression = Call(
@@ -3009,7 +3005,6 @@ Expression valueExpression
                     var typedConverterType = converterType.GetGenericTypeImplementations(typeof(ValueConverter<,>)).FirstOrDefault();
                     Expression invocationExpression;
 
-                    // TODO: do we even need to do this check? can we ever have a custom ValueConverter that is not generic?
                     if (typedConverterType != null)
                     {
                         if (converterExpression.Type != converter.GetType())
@@ -3063,9 +3058,6 @@ Expression valueExpression
                 Expression replaceExpression;
                 if (converter?.ConvertsNulls == true)
                 {
-                    // we potentially have to repeat logic from above here. We can check if we computed converterExpression before
-                    // if so, it means there are liftable constants in the ConvertFromProvider expression
-                    // we can also reuse converter expression, just switch argument to the Invoke for default(provier type) or object
                     if (converterExpression != null)
                     {
                         var converterType = converter.GetType();
@@ -3119,8 +3111,7 @@ Expression valueExpression
                     valueExpression);
             }
 
-            if (_detailedErrorsEnabled
-                && !buffering)
+            if (_detailedErrorsEnabled && !buffering)
             {
                 var exceptionParameter = Parameter(typeof(Exception), name: "e");
 
@@ -3142,7 +3133,6 @@ Expression valueExpression
 
             return valueExpression;
         }
-
         private Expression CreateReadJsonPropertyValueExpression(
             ParameterExpression jsonReaderManagerParameter,
             IProperty property)

src/EFCore.Relational/Query/SqlExpressions/AtTimeZoneExpression.cs

@@ -44,6 +44,19 @@ public AtTimeZoneExpression(
     /// </summary>
     public virtual SqlExpression TimeZone { get; }
 
+    /// <summary>
+    ///     A bool value indicating if this SQL expression is nullable.
+    /// </summary>
+    public virtual bool IsNullable
+        => Operand switch
+        {
+            ColumnExpression c => c.IsNullable,
+            SqlFunctionExpression f => f.IsNullable,
+            JsonScalarExpression j => j.IsNullable,
+            AtTimeZoneExpression a => a.IsNullable,
+            _ => true
+        };
+
     /// <inheritdoc />
     protected override Expression VisitChildren(ExpressionVisitor visitor)
     {

src/EFCore/Query/ProjectionBindingExpression.cs

@@ -90,6 +90,22 @@ void IPrintableExpression.Print(ExpressionPrinter expressionPrinter)
             expressionPrinter.Append(Index.ToString()!);
         }
     }
+    /// <summary>
+    ///     Creates a new instance of the <see cref="ProjectionBindingExpression" /> class with a new type.
+    /// </summary>
+    /// <param name="type">The new clr type of value being read.</param>
+    /// <returns>A new projection binding expression with the updated type.</returns>
+    public virtual ProjectionBindingExpression UpdateType(Type type)
+    {
+        if (type == Type)
+        {
+            return this;
+        }
+
+        return Index != null
+            ? new ProjectionBindingExpression(QueryExpression, Index.Value, type)
+            : new ProjectionBindingExpression(QueryExpression, ProjectionMember!, type);
+    }
 
     /// <inheritdoc />
     public override bool Equals(object? obj)

test/EFCore.SqlServer.FunctionalTests/Query/NorthwindSqlQuerySqlServerTest.cs

@@ -2,7 +2,7 @@
 // The .NET Foundation licenses this file to you under the MIT license.
 
 using Microsoft.Data.SqlClient;
-
+using Microsoft.EntityFrameworkCore.TestModels.Northwind;
 namespace Microsoft.EntityFrameworkCore.Query;
 
 #nullable disable
@@ -70,6 +70,26 @@ public override async Task SqlQuery_over_int_with_parameter(bool async)
 """);
     }
 
+    [ConditionalFact]
+    public virtual void Projection_binding_clean_up_non_nullable_value_type()
+    {
+        using var context = Fixture.CreateContext();
+
+        var query = context.Set<Customer>()
+            .Select(c => c.CustomerID) 
+            .ToQueryString();
+        var result = context.Set<Customer>().Select(c => c.CustomerID).ToList();
+        Assert.NotEmpty(result);
+    }
+
+    [ConditionalFact]
+    public virtual void Projection_binding_stays_nullable_for_nullable_types()
+    {
+        using var context = Fixture.CreateContext();
+        var result = context.Set<Employee>().Select(e => e.ReportsTo).ToList();
+
+        Assert.Contains(null, result); 
+    }
     protected override DbParameter CreateDbParameter(string name, object value)
         => new SqlParameter { ParameterName = name, Value = value };