Fix to #23990 - Relational: IS (NOT) NULL requires parenthesis when not in left side of an equality

We were generating incorrect sql when comparing bool value to a nullableBool.HasValue. Fix is to add parentheses to the right side when we detect the pattern:
- left side doesn't already have parentheses
- right side is a IS NULL / IS NOT NULL check
- argument to that null check is bool

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

Author: maumar

src/EFCore.Relational/Query/QuerySqlGenerator.cs

@@ -538,7 +538,16 @@ protected override Expression VisitSqlBinary(SqlBinaryExpression sqlBinaryExpres
 
             _relationalCommandBuilder.Append(GetOperator(sqlBinaryExpression));
 
-            requiresBrackets = RequiresBrackets(sqlBinaryExpression.Right);
+            // IS NULL / IS NOT NULL on bool requires brackets if it's on the right side 
+            // e.g. TRUE = (someBool IS NULL) rather than TRUE = someBool IS NULL
+            var requiresBracketsForNullCheckOnBool = !requiresBrackets
+                && sqlBinaryExpression.Right is SqlUnaryExpression sqlUnaryRight
+                && (sqlUnaryRight.OperatorType == ExpressionType.Equal
+                    || sqlUnaryRight.OperatorType == ExpressionType.NotEqual)
+                && sqlUnaryRight.Operand.Type == typeof(bool);
+
+            requiresBrackets = RequiresBrackets(sqlBinaryExpression.Right)
+                || requiresBracketsForNullCheckOnBool;
 
             if (requiresBrackets)
             {

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

@@ -1546,6 +1546,88 @@ 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 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);