Fix to #26344 - SqlNullabilityProcessor and COALESCE with more than t…

…wo arguments

Nested coalesce ops are converted to coalesce with multiple arguments. Fixing null semantics logic that only assumed coalesce function would have two arguments

Fixes #26344

src/EFCore.Relational/Query/ISqlExpressionFactory.cs

@@ -240,12 +240,12 @@ SqlBinaryExpression Or(
 
         // Other
         /// <summary>
-        ///     Creates a <see cref="SqlBinaryExpression" /> which represents a bitwise OR operation.
+        ///     Creates a <see cref="SqlFunctionExpression" /> which represents a COALESCE operation.
         /// </summary>
         /// <param name="left">The left operand.</param>
         /// <param name="right">The right operand.</param>
         /// <param name="typeMapping">A type mapping to be assigned to the created expression.</param>
-        /// <returns>An expression representing a SQL bitwise OR operation.</returns>
+        /// <returns>An expression representing a SQL COALESCE operation.</returns>
         SqlFunctionExpression Coalesce(
             SqlExpression left,
             SqlExpression right,

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

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System;
 using System.Collections;
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
@@ -67,7 +68,40 @@ protected override Expression VisitExtension(Expression extensionExpression)
                 return SimplifySqlBinary(sqlBinaryExpression);
             }
 
+            if (extensionExpression is SqlFunctionExpression sqlFunctionExpression
+                && IsCoalesce(sqlFunctionExpression)
+                && sqlFunctionExpression.Arguments!.Any(a => IsCoalesce(a)))
+            {
+                var instance = Visit(sqlFunctionExpression.Instance);
+                var arguments = new List<SqlExpression>();
+                foreach (var argument in sqlFunctionExpression.Arguments!)
+                {
+                    var newArgument = (SqlExpression)Visit(argument);
+                    if (IsCoalesce(newArgument))
+                    {
+                        arguments.AddRange(((SqlFunctionExpression)newArgument).Arguments!);
+                    }
+                    else
+                    {
+                        arguments.Add(newArgument);
+                    }
+                }
+
+                return new SqlFunctionExpression(
+                    sqlFunctionExpression.Name,
+                    arguments,
+                    sqlFunctionExpression.IsNullable,
+                    argumentsPropagateNullability: arguments.Select(a => false).ToArray(),
+                    sqlFunctionExpression.Type,
+                    sqlFunctionExpression.TypeMapping);
+            }
+
             return base.VisitExtension(extensionExpression);
+
+            static bool IsCoalesce(SqlExpression sqlExpression)
+                => sqlExpression is SqlFunctionExpression sqlFunctionExpression
+                    && string.Equals(sqlFunctionExpression.Name, "COALESCE", StringComparison.OrdinalIgnoreCase)
+                    && sqlFunctionExpression.Arguments?.Count > 1;
         }
 
         private bool IsCompareTo([NotNullWhen(true)] CaseExpression? caseExpression)

src/EFCore.Relational/Query/SqlExpressionFactory.cs

@@ -447,7 +447,7 @@ public virtual SqlFunctionExpression Coalesce(SqlExpression left, SqlExpression
                 "COALESCE",
                 typeMappedArguments,
                 nullable: true,
-                // COALESCE is handled separately since it's only nullable if *both* arguments are null
+                // COALESCE is handled separately since it's only nullable if *all* arguments are null
                 argumentsPropagateNullability: new[] { false, false },
                 resultType,
                 inferredTypeMapping);

src/EFCore.Relational/Query/SqlNullabilityProcessor.cs

@@ -1036,12 +1036,17 @@ protected virtual SqlExpression VisitSqlFunction(
                 && sqlFunctionExpression.Arguments != null
                 && string.Equals(sqlFunctionExpression.Name, "COALESCE", StringComparison.OrdinalIgnoreCase))
             {
-                var left = Visit(sqlFunctionExpression.Arguments[0], out var leftNullable);
-                var right = Visit(sqlFunctionExpression.Arguments[1], out var rightNullable);
+                var coalesceArguments = new List<SqlExpression>();
+                var coalesceNullable = true;
+                foreach (var argument in sqlFunctionExpression.Arguments)
+                {
+                    coalesceArguments.Add(Visit(argument, out var argumentNullable));
+                    coalesceNullable = coalesceNullable && argumentNullable;
+                }
 
-                nullable = leftNullable && rightNullable;
+                nullable = coalesceNullable;
 
-                return sqlFunctionExpression.Update(sqlFunctionExpression.Instance, new[] { left, right });
+                return sqlFunctionExpression.Update(sqlFunctionExpression.Instance, coalesceArguments);
             }
 
             var instance = Visit(sqlFunctionExpression.Instance, out _);
@@ -1734,35 +1739,28 @@ private SqlExpression ProcessNullNotNull(SqlUnaryExpression sqlUnaryExpression,
                 case SqlFunctionExpression sqlFunctionExpression:
                 {
                     if (sqlFunctionExpression.IsBuiltIn
-                        && string.Equals("COALESCE", sqlFunctionExpression.Name, StringComparison.OrdinalIgnoreCase))
+                        && string.Equals("COALESCE", sqlFunctionExpression.Name, StringComparison.OrdinalIgnoreCase)
+                        && sqlFunctionExpression.Arguments != null)
                     {
-                        // for coalesce:
-                        // (a ?? b) == null -> a == null && b == null
-                        // (a ?? b) != null -> a != null || b != null
-                        var left = ProcessNullNotNull(
-                            _sqlExpressionFactory.MakeUnary(
-                                sqlUnaryExpression.OperatorType,
-                                sqlFunctionExpression.Arguments![0],
-                                typeof(bool),
-                                sqlUnaryExpression.TypeMapping)!,
-                            operandNullable);
-
-                        var right = ProcessNullNotNull(
-                            _sqlExpressionFactory.MakeUnary(
-                                sqlUnaryExpression.OperatorType,
-                                sqlFunctionExpression.Arguments[1],
-                                typeof(bool),
-                                sqlUnaryExpression.TypeMapping)!,
-                            operandNullable);
-
-                        return SimplifyLogicalSqlBinaryExpression(
+                        // for coalesce
+                        // (a ?? b ?? c) == null -> a == null && b == null && c == null
+                        // (a ?? b ?? c) != null -> a != null || b != null || c != null
+                        return sqlFunctionExpression.Arguments
+                            .Select(a => ProcessNullNotNull(
+                                _sqlExpressionFactory.MakeUnary(
+                                    sqlUnaryExpression.OperatorType,
+                                    a,
+                                    typeof(bool),
+                                    sqlUnaryExpression.TypeMapping)!,
+                                operandNullable))
+                            .Aggregate((l, r) => SimplifyLogicalSqlBinaryExpression(
                             _sqlExpressionFactory.MakeBinary(
                                 sqlUnaryExpression.OperatorType == ExpressionType.Equal
                                     ? ExpressionType.AndAlso
                                     : ExpressionType.OrElse,
-                                left,
-                                right,
-                                sqlUnaryExpression.TypeMapping)!);
+                                l,
+                                r,
+                                sqlUnaryExpression.TypeMapping)!));
                     }
 
                     if (!sqlFunctionExpression.IsNullable)

test/EFCore.Relational.Specification.Tests/Query/NullSemanticsQueryTestBase.cs

@@ -929,9 +929,9 @@ public virtual Task Projecting_nullable_bool_with_coalesce(bool async)
 
         [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
-        public virtual Task Projecting_nullable_bool_with_coalesce_nested(bool async)
+        public virtual async Task Projecting_nullable_bool_with_coalesce_nested(bool async)
         {
-            return AssertQuery(
+            await AssertQuery(
                 async,
                 ss => ss.Set<NullSemanticsEntity1>().Select(e => new { e.Id, Coalesce = e.NullableBoolA ?? (e.NullableBoolB ?? false) }),
                 elementSorter: e => e.Id,
@@ -940,6 +940,16 @@ public virtual Task Projecting_nullable_bool_with_coalesce_nested(bool async)
                     Assert.Equal(e.Id, a.Id);
                     Assert.Equal(e.Coalesce, a.Coalesce);
                 });
+
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Select(e => new { e.Id, Coalesce = (e.NullableBoolA ?? e.NullableBoolB) ?? false }),
+                elementSorter: e => e.Id,
+                elementAsserter: (e, a) =>
+                {
+                    Assert.Equal(e.Id, a.Id);
+                    Assert.Equal(e.Coalesce, a.Coalesce);
+                });
         }
 
         [ConditionalTheory]
@@ -1863,6 +1873,28 @@ public virtual Task Sum_function_is_always_considered_non_nullable(bool async)
                 ss => ss.Set<NullSemanticsEntity1>().GroupBy(e => e.NullableIntA).Select(g => new { g.Key, Sum = g.Sum(x => x.IntA) != g.Key }));
         }
 
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual Task Nullability_is_computed_correctly_for_chained_coalesce(bool async)
+        {
+            return AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => (e.NullableIntA ?? e.NullableIntB ?? e.IntC) != e.NullableIntC));
+        }
+
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual async Task Nullability_check_is_computed_correctly_for_chained_coalesce(bool async)
+        {
+            await AssertQueryScalar(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => (e.NullableIntA ?? e.NullableIntB ?? e.NullableIntC) == null).Select(e => e.Id));
+
+            await AssertQueryScalar(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => (e.NullableIntA ?? e.NullableIntB ?? e.NullableIntC) != null).Select(e => e.Id));
+        }
+
         private string NormalizeDelimitersInRawString(string sql)
             => Fixture.TestStore.NormalizeDelimitersInRawString(sql);
 

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

@@ -5151,11 +5151,11 @@ public override async Task Select_subquery_int_with_outside_cast_and_coalesce(bo
             await base.Select_subquery_int_with_outside_cast_and_coalesce(async);
 
             AssertSql(
-                @"SELECT COALESCE(COALESCE((
+                @"SELECT COALESCE((
     SELECT TOP(1) [w].[Id]
     FROM [Weapons] AS [w]
     WHERE [g].[FullName] = [w].[OwnerFullName]
-    ORDER BY [w].[Id]), 0), 42)
+    ORDER BY [w].[Id]), 0, 42)
 FROM [Gears] AS [g]");
         }
 

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

@@ -1237,7 +1237,10 @@ public override async Task Projecting_nullable_bool_with_coalesce_nested(bool as
             await base.Projecting_nullable_bool_with_coalesce_nested(async);
 
             AssertSql(
-                @"SELECT [e].[Id], COALESCE([e].[NullableBoolA], COALESCE([e].[NullableBoolB], CAST(0 AS bit))) AS [Coalesce]
+                @"SELECT [e].[Id], COALESCE([e].[NullableBoolA], [e].[NullableBoolB], CAST(0 AS bit)) AS [Coalesce]
+FROM [Entities1] AS [e]",
+                //
+                @"SELECT [e].[Id], COALESCE([e].[NullableBoolA], [e].[NullableBoolB], CAST(0 AS bit)) AS [Coalesce]
 FROM [Entities1] AS [e]");
         }
 
@@ -2276,6 +2279,30 @@ FROM [Entities1] AS [e]
 GROUP BY [e].[NullableIntA]");
         }
 
+        public override async Task Nullability_is_computed_correctly_for_chained_coalesce(bool async)
+        {
+            await base.Nullability_is_computed_correctly_for_chained_coalesce(async);
+
+            AssertSql(
+                @"SELECT [e].[Id], [e].[BoolA], [e].[BoolB], [e].[BoolC], [e].[IntA], [e].[IntB], [e].[IntC], [e].[NullableBoolA], [e].[NullableBoolB], [e].[NullableBoolC], [e].[NullableIntA], [e].[NullableIntB], [e].[NullableIntC], [e].[NullableStringA], [e].[NullableStringB], [e].[NullableStringC], [e].[StringA], [e].[StringB], [e].[StringC]
+FROM [Entities1] AS [e]
+WHERE (COALESCE([e].[NullableIntA], [e].[NullableIntB], [e].[IntC]) <> [e].[NullableIntC]) OR [e].[NullableIntC] IS NULL");
+        }
+
+        public override async Task Nullability_check_is_computed_correctly_for_chained_coalesce(bool async)
+        {
+            await base.Nullability_check_is_computed_correctly_for_chained_coalesce(async);
+
+            AssertSql(
+                @"SELECT [e].[Id]
+FROM [Entities1] AS [e]
+WHERE ([e].[NullableIntA] IS NULL AND [e].[NullableIntB] IS NULL) AND [e].[NullableIntC] IS NULL",
+                //
+                @"SELECT [e].[Id]
+FROM [Entities1] AS [e]
+WHERE ([e].[NullableIntA] IS NOT NULL OR [e].[NullableIntB] IS NOT NULL) OR [e].[NullableIntC] IS NOT NULL");
+        }
+
         private void AssertSql(params string[] expected)
             => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 

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

@@ -5641,14 +5641,14 @@ FROM [CompetitionSeasons] AS [c1]
                     @"SELECT [a].[Id], [a].[ActivityTypeId], [a].[DateTime], [a].[Points], (
     SELECT TOP(1) [c].[Id]
     FROM [CompetitionSeasons] AS [c]
-    WHERE ([c].[StartDate] <= [a].[DateTime]) AND ([a].[DateTime] < [c].[EndDate])) AS [CompetitionSeasonId], COALESCE([a].[Points], COALESCE((
+    WHERE ([c].[StartDate] <= [a].[DateTime]) AND ([a].[DateTime] < [c].[EndDate])) AS [CompetitionSeasonId], COALESCE([a].[Points], (
     SELECT TOP(1) [a1].[Points]
     FROM [ActivityTypePoints12456] AS [a1]
     INNER JOIN [CompetitionSeasons] AS [c0] ON [a1].[CompetitionSeasonId] = [c0].[Id]
     WHERE ([a0].[Id] = [a1].[ActivityTypeId]) AND ([c0].[Id] = (
         SELECT TOP(1) [c1].[Id]
         FROM [CompetitionSeasons] AS [c1]
-        WHERE ([c1].[StartDate] <= [a].[DateTime]) AND ([a].[DateTime] < [c1].[EndDate])))), 0)) AS [Points]
+        WHERE ([c1].[StartDate] <= [a].[DateTime]) AND ([a].[DateTime] < [c1].[EndDate])))), 0) AS [Points]
 FROM [Activities] AS [a]
 INNER JOIN [ActivityType12456] AS [a0] ON [a].[ActivityTypeId] = [a0].[Id]");
             }

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

@@ -6093,11 +6093,11 @@ public override async Task Select_subquery_int_with_outside_cast_and_coalesce(bo
             await base.Select_subquery_int_with_outside_cast_and_coalesce(async);
 
             AssertSql(
-                @"SELECT COALESCE(COALESCE((
+                @"SELECT COALESCE((
     SELECT TOP(1) [w].[Id]
     FROM [Weapons] AS [w]
     WHERE [g].[FullName] = [w].[OwnerFullName]
-    ORDER BY [w].[Id]), 0), 42)
+    ORDER BY [w].[Id]), 0, 42)
 FROM [Gears] AS [g]");
         }