Simpler/safer patch fix for #35393 that doesn't compensate for some changes

Author: maumar

src/EFCore.Relational/Query/SqlExpressionFactory.cs

@@ -9,6 +9,9 @@ namespace Microsoft.EntityFrameworkCore.Query;
 /// <inheritdoc />
 public class SqlExpressionFactory : ISqlExpressionFactory
 {
+    private static readonly bool UseOldBehavior35393 =
+        AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue35393", out var enabled35393) && enabled35393;
+
     private readonly IRelationalTypeMappingSource _typeMappingSource;
     private readonly RelationalTypeMapping _boolTypeMapping;
 
@@ -660,20 +663,45 @@ private SqlExpression Not(SqlExpression operand, SqlExpression? existingExpressi
             SqlBinaryExpression { OperatorType: ExpressionType.OrElse } binary
                 => AndAlso(Not(binary.Left), Not(binary.Right)),
 
-            // use equality where possible
+            // use equality where possible - we can only do this when we know a is not null
+            // at this point we are limited to constants, parameters and columns
+            // see issue #35393
             // !(a == true) -> a == false
             // !(a == false) -> a == true
             SqlBinaryExpression { OperatorType: ExpressionType.Equal, Right: SqlConstantExpression { Value: bool } } binary
+                when UseOldBehavior35393
+                => Equal(binary.Left, Not(binary.Right)),
+
+            SqlBinaryExpression
+            {
+                OperatorType: ExpressionType.Equal,
+                Right: SqlConstantExpression { Value: bool },
+                Left: SqlConstantExpression { Value: bool }
+                    or SqlParameterExpression { IsNullable: false }
+                    or ColumnExpression { IsNullable: false }
+            } binary
                 => Equal(binary.Left, Not(binary.Right)),
 
             // !(true == a) -> false == a
             // !(false == a) -> true == a
             SqlBinaryExpression { OperatorType: ExpressionType.Equal, Left: SqlConstantExpression { Value: bool } } binary
+                when UseOldBehavior35393
+                => Equal(Not(binary.Left), binary.Right),
+
+            SqlBinaryExpression
+            {
+                OperatorType: ExpressionType.Equal,
+                Left: SqlConstantExpression { Value: bool },
+                Right: SqlConstantExpression { Value: bool }
+                    or SqlParameterExpression { IsNullable: false }
+                    or ColumnExpression { IsNullable: false }
+            } binary
                 => Equal(Not(binary.Left), binary.Right),
 
             // !(a == b) -> a != b
             SqlBinaryExpression { OperatorType: ExpressionType.Equal } sqlBinaryOperand => NotEqual(
                 sqlBinaryOperand.Left, sqlBinaryOperand.Right),
+
             // !(a != b) -> a == b
             SqlBinaryExpression { OperatorType: ExpressionType.NotEqual } sqlBinaryOperand => Equal(
                 sqlBinaryOperand.Left, sqlBinaryOperand.Right),

src/EFCore.Relational/Query/SqlNullabilityProcessor.cs

@@ -19,6 +19,9 @@ namespace Microsoft.EntityFrameworkCore.Query;
 /// </summary>
 public class SqlNullabilityProcessor
 {
+    private static readonly bool UseOldBehavior35393 =
+        AppContext.TryGetSwitch("Microsoft.EntityFrameworkCore.Issue35393", out var enabled35393) && enabled35393;
+
     private readonly List<ColumnExpression> _nonNullableColumns;
     private readonly List<ColumnExpression> _nullValueColumns;
     private readonly ISqlExpressionFactory _sqlExpressionFactory;
@@ -1343,7 +1346,7 @@ protected virtual SqlExpression VisitSqlBinary(
             // we assume that NullSemantics rewrite is only needed (on the current level)
             // if the optimization didn't make any changes.
             // Reason is that optimization can/will change the nullability of the resulting expression
-            // and that inforation is not tracked/stored anywhere
+            // and that information is not tracked/stored anywhere
             // so we can no longer rely on nullabilities that we computed earlier (leftNullable, rightNullable)
             // when performing null semantics rewrite.
             // It should be fine because current optimizations *radically* change the expression

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

@@ -92,22 +92,26 @@ public virtual async Task Rewrite_compare_int_with_int(bool async)
     public virtual async Task Rewrite_compare_bool_with_bool(bool async)
     {
         var bools = new[] { false, true };
+        var onetwothree = new[] { 1, 2, 3 };
 
         foreach (var neq in bools)
         {
             foreach (var negated in bools)
             {
                 foreach (var negateB in bools)
                 {
-                    foreach (var nullableA in bools)
+                    foreach (var nullableA in onetwothree)
                     {
                         foreach (var negateA in bools)
                         {
-                            foreach (var nullableB in bools)
+                            foreach (var nullableB in onetwothree)
                             {
+                                // filter out tests comparing two constants
+                                if (nullableA == 3 && nullableB == 3) continue;
+
                                 var queryBuilder = (ISetSource ss) =>
                                 {
-                                    var data = nullableA
+                                    var data = nullableA == 2
                                         ? ss.Set<NullSemanticsEntity1>().Select(
                                             e => new
                                             {
@@ -116,30 +120,47 @@ public virtual async Task Rewrite_compare_bool_with_bool(bool async)
                                                 e.BoolB,
                                                 e.NullableBoolB
                                             })
-                                        : ss.Set<NullSemanticsEntity1>().Select(
-                                            e => new
-                                            {
-                                                e.Id,
-                                                A = (bool?)e.BoolA,
-                                                e.BoolB,
-                                                e.NullableBoolB
-                                            });
-
-                                    var query = nullableB
+                                        : nullableA == 1
+                                            ? ss.Set<NullSemanticsEntity1>().Select(
+                                                e => new
+                                                {
+                                                    e.Id,
+                                                    A = (bool?)e.BoolA,
+                                                    e.BoolB,
+                                                    e.NullableBoolB
+                                                })
+                                            : ss.Set<NullSemanticsEntity1>().Select(
+                                                e => new
+                                                {
+                                                    e.Id,
+                                                    A = (bool?)true,
+                                                    e.BoolB,
+                                                    e.NullableBoolB
+                                                });
+
+                                    var query = nullableB == 2
                                         ? data.Select(
                                             e => new
                                             {
                                                 e.Id,
                                                 e.A,
                                                 B = e.NullableBoolB
                                             })
-                                        : data.Select(
-                                            e => new
-                                            {
-                                                e.Id,
-                                                e.A,
-                                                B = (bool?)e.BoolB
-                                            });
+                                        : nullableB == 1
+                                            ? data.Select(
+                                                e => new
+                                                {
+                                                    e.Id,
+                                                    e.A,
+                                                    B = (bool?)e.BoolB
+                                                })
+                                            : data.Select(
+                                                e => new
+                                                {
+                                                    e.Id,
+                                                    e.A,
+                                                    B = (bool?)true
+                                                });
 
                                     query = negateA
                                         ? query.Select(
@@ -2462,6 +2483,18 @@ await AssertQueryScalar(
             ss => ss.Set<NullSemanticsEntity1>().Where(e => true).Select(e => e.Id));
     }
 
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual async Task Compare_constant_true_to_expression_which_evaluates_to_null(bool async)
+    {
+        var prm = default(bool?);
+
+        await AssertQueryScalar(
+            async,
+            ss => ss.Set<NullSemanticsEntity1>().Where(x => x.NullableBoolA != null
+                && !object.Equals(true, x.NullableBoolA == null ? null : prm)).Select(x => x.Id));
+    }
+
     // We can't client-evaluate Like (for the expected results).
     // However, since the test data has no LIKE wildcards, it effectively functions like equality - except that 'null like null' returns
     // false instead of true. So we have this "lite" implementation which doesn't support wildcards.

test/EFCore.Specification.Tests/Query/NorthwindJoinQueryTestBase.cs

@@ -654,6 +654,24 @@ join o in ss.Set<Order>().OrderBy(o => o.OrderID).Take(100) on c.CustomerID equa
                   from o in lo.Where(x => x.CustomerID.StartsWith("A"))
                   select new { c.CustomerID, o.OrderID });
 
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task GroupJoin_on_true_equal_true(bool async)
+    => AssertQuery(
+        async,
+        ss => ss.Set<Customer>().GroupJoin(
+            ss.Set<Order>(),
+            x => true,
+            x => true,
+            (c, g) => new { c, g })
+        .Select(x => new { x.c.CustomerID, Orders = x.g }),
+        elementSorter: e => e.CustomerID,
+        elementAsserter: (e, a) =>
+        {
+            Assert.Equal(e.CustomerID, a.CustomerID);
+            AssertCollection(e.Orders, a.Orders, elementSorter: ee => ee.OrderID);
+        });
+
     [ConditionalTheory]
     [MemberData(nameof(IsAsyncData))]
     public virtual Task Inner_join_with_tautology_predicate_converts_to_cross_join(bool async)

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

@@ -5021,7 +5021,7 @@ INNER JOIN (
     FROM [Factions] AS [f]
     WHERE [f].[Name] = N'Swarm'
 ) AS [f0] ON [l].[Name] = [f0].[CommanderName]
-WHERE [f0].[Eradicated] = CAST(0 AS bit) OR [f0].[Eradicated] IS NULL
+WHERE [f0].[Eradicated] <> CAST(1 AS bit) OR [f0].[Eradicated] IS NULL
 """);
     }
 
@@ -5038,7 +5038,7 @@ LEFT JOIN (
     FROM [Factions] AS [f]
     WHERE [f].[Name] = N'Swarm'
 ) AS [f0] ON [l].[Name] = [f0].[CommanderName]
-WHERE [f0].[Eradicated] = CAST(0 AS bit) OR [f0].[Eradicated] IS NULL
+WHERE [f0].[Eradicated] <> CAST(1 AS bit) OR [f0].[Eradicated] IS NULL
 """);
     }
 
@@ -7585,17 +7585,17 @@ public override async Task Join_inner_source_custom_projection_followed_by_filte
 
         AssertSql(
             """
-SELECT CASE
-    WHEN [f].[Name] = N'Locust' THEN CAST(1 AS bit)
-END AS [IsEradicated], [f].[CommanderName], [f].[Name]
-FROM [LocustLeaders] AS [l]
-INNER JOIN [Factions] AS [f] ON [l].[Name] = [f].[CommanderName]
-WHERE CASE
-    WHEN [f].[Name] = N'Locust' THEN CAST(1 AS bit)
-END = CAST(0 AS bit) OR CASE
-    WHEN [f].[Name] = N'Locust' THEN CAST(1 AS bit)
-END IS NULL
-""");
+    SELECT CASE
+        WHEN [f].[Name] = N'Locust' THEN CAST(1 AS bit)
+    END AS [IsEradicated], [f].[CommanderName], [f].[Name]
+    FROM [LocustLeaders] AS [l]
+    INNER JOIN [Factions] AS [f] ON [l].[Name] = [f].[CommanderName]
+    WHERE CASE
+        WHEN [f].[Name] = N'Locust' THEN CAST(1 AS bit)
+    END <> CAST(1 AS bit) OR CASE
+        WHEN [f].[Name] = N'Locust' THEN CAST(1 AS bit)
+    END IS NULL
+    """);
     }
 
     public override async Task Byte_array_contains_literal(bool async)

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

@@ -992,6 +992,22 @@ ORDER BY [o].[OrderID]
 """);
     }
 
+    public override async Task GroupJoin_on_true_equal_true(bool async)
+    {
+        await base.GroupJoin_on_true_equal_true(async);
+
+        AssertSql(
+            """
+SELECT [c].[CustomerID], [o0].[OrderID], [o0].[CustomerID], [o0].[EmployeeID], [o0].[OrderDate]
+FROM [Customers] AS [c]
+OUTER APPLY (
+    SELECT [o].[OrderID], [o].[CustomerID], [o].[EmployeeID], [o].[OrderDate]
+    FROM [Orders] AS [o]
+) AS [o0]
+ORDER BY [c].[CustomerID]
+""");
+    }
+
     private void AssertSql(params string[] expected)
         => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);