Fix to #35393 - GroupJoin in EF Core 9 Returns Null for Joined Entities

Problem was that in EF9 we moved some optimizations from sql nullability processor to SqlExpressionFactory (so that we optimize things early). One of the optimizations:

```
!(true == a) -> false == a
!(false == a) -> true == a
```

is not safe to do when a is a constant or parameter null value, because it evaluates to true IS NULL (two value logic). Fix is to remove this optimization from SqlExpressionFactory and instead put it back into OptimizeNotExpression, once we've converted everything we could to IS NULL checks already.

Fixes #35393

src/EFCore.Relational/Query/SqlExpressionFactory.cs

@@ -660,20 +660,10 @@ private SqlExpression Not(SqlExpression operand, SqlExpression? existingExpressi
             SqlBinaryExpression { OperatorType: ExpressionType.OrElse } binary
                 => AndAlso(Not(binary.Left), Not(binary.Right)),
 
-            // use equality where possible
-            // !(a == true) -> a == false
-            // !(a == false) -> a == true
-            SqlBinaryExpression { OperatorType: ExpressionType.Equal, Right: SqlConstantExpression { Value: bool } } binary
-                => Equal(binary.Left, Not(binary.Right)),
-
-            // !(true == a) -> false == a
-            // !(false == a) -> true == a
-            SqlBinaryExpression { OperatorType: ExpressionType.Equal, Left: SqlConstantExpression { Value: bool } } 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

@@ -1124,7 +1124,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
@@ -1590,10 +1590,10 @@ private SqlExpression RewriteNullSemantics(
         }
 
         var leftIsNull = ProcessNullNotNull(_sqlExpressionFactory.IsNull(left), leftNullable);
-        var leftIsNotNull = _sqlExpressionFactory.Not(leftIsNull);
+        var leftIsNotNull = OptimizeNotExpression(_sqlExpressionFactory.Not(leftIsNull));
 
         var rightIsNull = ProcessNullNotNull(_sqlExpressionFactory.IsNull(right), rightNullable);
-        var rightIsNotNull = _sqlExpressionFactory.Not(rightIsNull);
+        var rightIsNotNull = OptimizeNotExpression(_sqlExpressionFactory.Not(rightIsNull));
 
         SqlExpression body;
         if (leftNegated == rightNegated)
@@ -1625,7 +1625,7 @@ private SqlExpression RewriteNullSemantics(
         if (sqlBinaryExpression.OperatorType == ExpressionType.NotEqual)
         {
             // the factory takes care of simplifying using DeMorgan
-            body = _sqlExpressionFactory.Not(body);
+            body = OptimizeNotExpression(_sqlExpressionFactory.Not(body));
         }
 
         return body;
@@ -1643,18 +1643,40 @@ protected virtual SqlExpression OptimizeNotExpression(SqlExpression expression)
             return expression;
         }
 
-        // !(a > b) -> a <= b
-        // !(a >= b) -> a < b
-        // !(a < b) -> a >= b
-        // !(a <= b) -> a > b
-        if (sqlUnaryExpression.Operand is SqlBinaryExpression sqlBinaryOperand
-            && TryNegate(sqlBinaryOperand.OperatorType, out var negated))
+        if (sqlUnaryExpression.Operand is SqlBinaryExpression sqlBinaryOperand)
         {
-            return _sqlExpressionFactory.MakeBinary(
-                negated,
-                sqlBinaryOperand.Left,
-                sqlBinaryOperand.Right,
-                sqlBinaryOperand.TypeMapping)!;
+            // !(a > b) -> a <= b
+            // !(a >= b) -> a < b
+            // !(a < b) -> a >= b
+            // !(a <= b) -> a > b
+            if (TryNegate(sqlBinaryOperand.OperatorType, out var negated))
+            {
+                return _sqlExpressionFactory.MakeBinary(
+                    negated,
+                    sqlBinaryOperand.Left,
+                    sqlBinaryOperand.Right,
+                    sqlBinaryOperand.TypeMapping)!;
+            }
+
+            // use equality where possible - at this point (true == null) and (false == null) have been converted to
+            // IS NULL / IS NOT NULL (i.e. false), so this optimization is safe to do. See #35393
+            // !(a == true) -> a == false
+            // !(a == false) -> a == true
+            if (sqlBinaryOperand is { OperatorType: ExpressionType.Equal, Right: SqlConstantExpression { Value: bool } })
+            {
+                return _sqlExpressionFactory.Equal(
+                    sqlBinaryOperand.Left,
+                    OptimizeNotExpression(_sqlExpressionFactory.Not(sqlBinaryOperand.Right)));
+            }
+
+            // !(true == a) -> false == a
+            // !(false == a) -> true == a
+            if (sqlBinaryOperand is { OperatorType: ExpressionType.Equal, Left: SqlConstantExpression { Value: bool } })
+            {
+                return _sqlExpressionFactory.Equal(
+                    OptimizeNotExpression(_sqlExpressionFactory.Not(sqlBinaryOperand.Left)),
+                    sqlBinaryOperand.Right);
+            }
         }
 
         // the factory can optimize most `NOT` expressions
@@ -2039,7 +2061,7 @@ private SqlExpression ProcessNullNotNull(SqlExpression sqlExpression, bool opera
                     return result;
                 }
             }
-                break;
+            break;
         }
 
         return sqlUnaryExpression;

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

@@ -2459,6 +2459,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

@@ -703,6 +703,24 @@ public virtual Task GroupJoin_aggregate_nested_anonymous_key_selectors(bool asyn
                     (c, g) => new { c.CustomerID, Sum = g.Sum(x => x.CustomerID.Length) }),
                 elementSorter: e => e.CustomerID));
 
+    [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

@@ -4452,7 +4452,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
 """);
     }
 
@@ -4469,7 +4469,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
 """);
     }
 
@@ -6978,7 +6978,7 @@ 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
+END <> CAST(1 AS bit) OR CASE
     WHEN [f].[Name] = N'Locust' THEN CAST(1 AS bit)
 END IS NULL
 """);

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

@@ -616,6 +616,22 @@ public override async Task GroupJoin_aggregate_nested_anonymous_key_selectors(bo
         AssertSql();
     }
 
+    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]
+""");
+    }
+
     public override async Task Inner_join_with_tautology_predicate_converts_to_cross_join(bool async)
     {
         await base.Inner_join_with_tautology_predicate_converts_to_cross_join(async);

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

@@ -4568,6 +4568,18 @@ FROM [Entities1] AS [e]
 """);
     }
 
+    public override async Task Compare_constant_true_to_expression_which_evaluates_to_null(bool async)
+    {
+        await base.Compare_constant_true_to_expression_which_evaluates_to_null(async);
+
+        AssertSql(
+"""
+SELECT [e].[Id]
+FROM [Entities1] AS [e]
+WHERE [e].[NullableBoolA] IS NOT NULL
+""");
+    }
+
     private void AssertSql(params string[] expected)
         => Fixture.TestSqlLoggerFactory.AssertBaseline(expected);
 

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

@@ -6051,7 +6051,7 @@ INNER JOIN (
     FROM [LocustHordes] AS [l1]
     WHERE [l1].[Name] = N'Swarm'
 ) AS [l2] ON [u].[Name] = [l2].[CommanderName]
-WHERE [l2].[Eradicated] = CAST(0 AS bit) OR [l2].[Eradicated] IS NULL
+WHERE [l2].[Eradicated] <> CAST(1 AS bit) OR [l2].[Eradicated] IS NULL
 """);
     }
 
@@ -6074,7 +6074,7 @@ LEFT JOIN (
     FROM [LocustHordes] AS [l1]
     WHERE [l1].[Name] = N'Swarm'
 ) AS [l2] ON [u].[Name] = [l2].[CommanderName]
-WHERE [l2].[Eradicated] = CAST(0 AS bit) OR [l2].[Eradicated] IS NULL
+WHERE [l2].[Eradicated] <> CAST(1 AS bit) OR [l2].[Eradicated] IS NULL
 """);
     }
 
@@ -9333,7 +9333,7 @@ FROM [LocustCommanders] AS [l0]
 INNER JOIN [LocustHordes] AS [l1] ON [u].[Name] = [l1].[CommanderName]
 WHERE CASE
     WHEN [l1].[Name] = N'Locust' THEN CAST(1 AS bit)
-END = CAST(0 AS bit) OR CASE
+END <> CAST(1 AS bit) OR CASE
     WHEN [l1].[Name] = N'Locust' THEN CAST(1 AS bit)
 END IS NULL
 """);

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

@@ -5129,7 +5129,7 @@ public override async Task Null_semantics_on_nullable_bool_from_inner_join_subqu
         await base.Null_semantics_on_nullable_bool_from_inner_join_subquery_is_fully_applied(async);
 
         AssertSql(
-            """
+"""
 SELECT [s].[Id], [s].[CapitalName], [s].[Name], [s].[ServerAddress], [s].[CommanderName], [s].[Eradicated], [s].[Discriminator]
 FROM [LocustLeaders] AS [l]
 INNER JOIN (
@@ -5140,7 +5140,7 @@ FROM [Factions] AS [f]
     LEFT JOIN [LocustHordes] AS [l0] ON [f].[Id] = [l0].[Id]
     WHERE [l0].[Id] IS NOT NULL AND [f].[Name] = N'Swarm'
 ) AS [s] ON [l].[Name] = [s].[CommanderName]
-WHERE [s].[Eradicated] = CAST(0 AS bit) OR [s].[Eradicated] IS NULL
+WHERE [s].[Eradicated] <> CAST(1 AS bit) OR [s].[Eradicated] IS NULL
 """);
     }
 
@@ -5160,7 +5160,7 @@ FROM [Factions] AS [f]
     LEFT JOIN [LocustHordes] AS [l0] ON [f].[Id] = [l0].[Id]
     WHERE [l0].[Id] IS NOT NULL AND [f].[Name] = N'Swarm'
 ) AS [s] ON [l].[Name] = [s].[CommanderName]
-WHERE [s].[Eradicated] = CAST(0 AS bit) OR [s].[Eradicated] IS NULL
+WHERE [s].[Eradicated] <> CAST(1 AS bit) OR [s].[Eradicated] IS NULL
 """);
     }
 
@@ -7918,7 +7918,7 @@ public override async Task Join_inner_source_custom_projection_followed_by_filte
         await base.Join_inner_source_custom_projection_followed_by_filter(async);
 
         AssertSql(
-            """
+"""
 SELECT CASE
     WHEN [s].[Name] = N'Locust' THEN CAST(1 AS bit)
 END AS [IsEradicated], [s].[CommanderName], [s].[Name]
@@ -7931,7 +7931,7 @@ WHERE [l0].[Id] IS NOT NULL
 ) AS [s] ON [l].[Name] = [s].[CommanderName]
 WHERE CASE
     WHEN [s].[Name] = N'Locust' THEN CAST(1 AS bit)
-END = CAST(0 AS bit) OR CASE
+END <> CAST(1 AS bit) OR CASE
     WHEN [s].[Name] = N'Locust' THEN CAST(1 AS bit)
 END IS NULL
 """);

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

@@ -2160,7 +2160,7 @@ SELECT CASE
 INNER JOIN [Factions] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' 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
+END <> CAST(1 AS bit) OR CASE
     WHEN [f].[Name] = N'Locust' THEN CAST(1 AS bit)
 END IS NULL
 """);
@@ -3962,7 +3962,7 @@ INNER JOIN (
     FROM [Factions] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' 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
 """);
     }
 
@@ -5676,7 +5676,7 @@ LEFT JOIN (
     FROM [Factions] FOR SYSTEM_TIME AS OF '2010-01-01T00:00:00.0000000' 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
 """);
     }
 

test/EFCore.Sqlite.FunctionalTests/Query/GearsOfWarQuerySqliteTest.cs

@@ -2314,7 +2314,7 @@ SELECT CASE
 INNER JOIN "Factions" AS "f" ON "l"."Name" = "f"."CommanderName"
 WHERE CASE
     WHEN "f"."Name" = 'Locust' THEN 1
-END = 0 OR CASE
+END <> 1 OR CASE
     WHEN "f"."Name" = 'Locust' THEN 1
 END IS NULL
 """);
@@ -4692,7 +4692,7 @@ LEFT JOIN (
     FROM "Factions" AS "f"
     WHERE "f"."Name" = 'Swarm'
 ) AS "f0" ON "l"."Name" = "f0"."CommanderName"
-WHERE "f0"."Eradicated" = 0 OR "f0"."Eradicated" IS NULL
+WHERE "f0"."Eradicated" <> 1 OR "f0"."Eradicated" IS NULL
 """);
     }
 
@@ -4760,7 +4760,7 @@ INNER JOIN (
     FROM "Factions" AS "f"
     WHERE "f"."Name" = 'Swarm'
 ) AS "f0" ON "l"."Name" = "f0"."CommanderName"
-WHERE "f0"."Eradicated" = 0 OR "f0"."Eradicated" IS NULL
+WHERE "f0"."Eradicated" <> 1 OR "f0"."Eradicated" IS NULL
 """);
     }
 

test/EFCore.Sqlite.FunctionalTests/Query/NorthwindJoinQuerySqliteTest.cs

@@ -57,4 +57,10 @@ public override async Task Take_in_collection_projection_with_FirstOrDefault_on_
             SqliteStrings.ApplyNotSupported,
             (await Assert.ThrowsAsync<InvalidOperationException>(
                 () => base.Take_in_collection_projection_with_FirstOrDefault_on_top_level(async))).Message);
+
+    public override async Task GroupJoin_on_true_equal_true(bool async)
+        => Assert.Equal(
+            SqliteStrings.ApplyNotSupported,
+            (await Assert.ThrowsAsync<InvalidOperationException>(
+                () => base.GroupJoin_on_true_equal_true(async))).Message);
 }