Fix to #23990 - Relational: IS (NOT) NULL requires parenthesis when n…

…ot in left side of an equality

We were generating incorrect sql for some cases involving nullability check on nullable bool. E.g. true = someBool IS NULL. Instead we need true = (someBool IS NULL)
Note: this is not really an issue for sql server because of search conditions, but sqlite would yield incorrect data for those queries.

Fixes #23990

src/EFCore.Relational/Query/QuerySqlGenerator.cs

@@ -556,7 +556,12 @@ protected override Expression VisitSqlBinary(SqlBinaryExpression sqlBinaryExpres
         }
 
         private static bool RequiresBrackets(SqlExpression expression)
-            => expression is SqlBinaryExpression || expression is LikeExpression;
+            => expression is SqlBinaryExpression
+                || expression is LikeExpression
+                || (expression is SqlUnaryExpression unary
+                    && unary.Operand.Type == typeof(bool)
+                    && (unary.OperatorType == ExpressionType.Equal
+                        || unary.OperatorType == ExpressionType.NotEqual));
 
         /// <inheritdoc />
         protected override Expression VisitSqlConstant(SqlConstantExpression sqlConstantExpression)

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

@@ -1546,6 +1546,101 @@ public virtual Task Multiple_non_equality_comparisons_with_null_in_the_middle(bo
                 ss => ss.Set<NullSemanticsEntity1>().Where(e => e.NullableIntA != 1 && e.NullableIntA != null && e.NullableIntA != 2));
         }
 
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual async Task Bool_equal_nullable_bool_HasValue(bool async)
+        {
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => true == e.NullableBoolA.HasValue));
+
+            var prm = false;
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => prm == e.NullableBoolA.HasValue));
+
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => e.BoolB == e.NullableBoolA.HasValue));
+        }
+
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual async Task Bool_equal_nullable_bool_compared_to_null(bool async)
+        {
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => true == (e.NullableBoolA == null)));
+
+            var prm = false;
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => prm == (e.NullableBoolA != null)));
+        }
+
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual async Task Bool_not_equal_nullable_bool_HasValue(bool async)
+        {
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => true != e.NullableBoolA.HasValue));
+
+            var prm = false;
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => prm != e.NullableBoolA.HasValue));
+
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => e.BoolB != e.NullableBoolA.HasValue));
+        }
+
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual async Task Bool_not_equal_nullable_bool_compared_to_null(bool async)
+        {
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => true != (e.NullableBoolA == null)));
+
+            var prm = false;
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => prm != (e.NullableBoolA != null)));
+        }
+
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual async Task Bool_logical_operation_with_nullable_bool_HasValue(bool async)
+        {
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => true || e.NullableBoolA.HasValue));
+
+            var prm = false;
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => prm && e.NullableBoolA.HasValue));
+
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => e.BoolB | e.NullableBoolA.HasValue));
+        }
+
+        [ConditionalTheory]
+        [MemberData(nameof(IsAsyncData))]
+        public virtual async Task Comparison_compared_to_null_check_on_bool(bool async)
+        {
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => (e.IntA == e.IntB) != e.NullableBoolA.HasValue));
+
+            await AssertQuery(
+                async,
+                ss => ss.Set<NullSemanticsEntity1>().Where(e => (e.IntA != e.IntB) == (e.NullableBoolA != null)));
+        }
+
         [ConditionalTheory]
         [MemberData(nameof(IsAsyncData))]
         public virtual async Task Multiple_non_equality_comparisons_including_null_comparison_work_for_relational_null_semantics(bool async)

test/EFCore.Specification.Tests/TestUtilities/ExpectedQueryRewritingVisitor.cs

@@ -302,7 +302,12 @@ private Expression AddNullProtectionForNonNullableMemberAccess(Expression expres
                     instance.Type,
                     memberExpression.Type.UnwrapNullableType());
 
-                return Expression.Call(methodInfo, instance, maybeLambda);
+                var maybeMethodCall = Expression.Call(methodInfo, instance, maybeLambda);
+
+                return memberExpression.Member.DeclaringType.IsNullableType()
+                    && memberExpression.Member.Name == "HasValue"
+                    ? Expression.Coalesce(maybeMethodCall, Expression.Constant(false))
+                    : maybeMethodCall;
             }
 
             return Visit(expression);

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

@@ -443,7 +443,7 @@ CROSS JOIN [FunkyCustomers] AS [f0]
 WHERE (CASE
     WHEN (([f0].[LastName] = N'') AND [f0].[LastName] IS NOT NULL) OR ([f].[FirstName] IS NOT NULL AND ([f0].[LastName] IS NOT NULL AND (RIGHT([f].[FirstName], LEN([f0].[LastName])) = [f0].[LastName]))) THEN CAST(1 AS bit)
     ELSE CAST(0 AS bit)
-END <> [f].[NullableBool]) OR [f].[NullableBool] IS NULL");
+END <> [f].[NullableBool]) OR ([f].[NullableBool] IS NULL)");
         }
 
         public override async Task String_FirstOrDefault_and_LastOrDefault(bool async)

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

@@ -4518,7 +4518,7 @@ INNER JOIN (
     FROM [Factions] AS [f]
     WHERE [f].[Name] = N'Swarm'
 ) AS [t] ON [l].[Name] = [t].[CommanderName]
-WHERE ([t].[Eradicated] <> CAST(1 AS bit)) OR [t].[Eradicated] IS NULL");
+WHERE ([t].[Eradicated] <> CAST(1 AS bit)) OR ([t].[Eradicated] IS NULL)");
         }
 
         public override async Task Null_semantics_on_nullable_bool_from_left_join_subquery_is_fully_applied(bool async)
@@ -4533,7 +4533,7 @@ LEFT JOIN (
     FROM [Factions] AS [f]
     WHERE [f].[Name] = N'Swarm'
 ) AS [t] ON [l].[Name] = [t].[CommanderName]
-WHERE ([t].[Eradicated] <> CAST(1 AS bit)) OR [t].[Eradicated] IS NULL");
+WHERE ([t].[Eradicated] <> CAST(1 AS bit)) OR ([t].[Eradicated] IS NULL)");
         }
 
         public override async Task Include_on_derived_type_with_order_by_and_paging(bool async)
@@ -6237,7 +6237,7 @@ public override async Task Nullable_bool_comparison_is_translated_to_server(bool
 
             AssertSql(
                 @"SELECT CASE
-    WHEN ([f].[Eradicated] = CAST(1 AS bit)) AND [f].[Eradicated] IS NOT NULL THEN CAST(1 AS bit)
+    WHEN ([f].[Eradicated] = CAST(1 AS bit)) AND ([f].[Eradicated] IS NOT NULL) THEN CAST(1 AS bit)
     ELSE CAST(0 AS bit)
 END AS [IsEradicated]
 FROM [Factions] AS [f]");
@@ -6697,10 +6697,10 @@ FROM [LocustLeaders] AS [l]
 WHERE (CASE
     WHEN [f].[Name] = N'Locust' THEN CAST(1 AS bit)
     ELSE NULL
-END <> CAST(1 AS bit)) OR CASE
+END <> CAST(1 AS bit)) OR (CASE
     WHEN [f].[Name] = N'Locust' THEN CAST(1 AS bit)
     ELSE NULL
-END IS NULL");
+END IS NULL)");
         }
 
         public override async Task Byte_array_contains_literal(bool async)