Put parentheses around IS NULL for non-bool operands, where needed only

Fixes dotnet#26767
roji · Jan 22, 2022 · efb2180 · efb2180
1 parent 767afd1
commit efb2180
Show file tree

Hide file tree

Showing 3 changed files with 16 additions and 16 deletions.
diff --git a/src/EFCore.Relational/Query/QuerySqlGenerator.cs b/src/EFCore.Relational/Query/QuerySqlGenerator.cs
@@ -846,10 +846,12 @@ protected virtual bool RequiresParentheses(SqlExpression outerExpression, SqlExp
 
             case SqlUnaryExpression sqlUnaryExpression:
             {
-                // Wrap IS (NOT) NULL operation when applied on bool column.
-                if ((sqlUnaryExpression.OperatorType == ExpressionType.Equal
-                        || sqlUnaryExpression.OperatorType == ExpressionType.NotEqual)
-                    && sqlUnaryExpression.Operand.Type == typeof(bool))
+                // Wrap IS (NOT) NULL operation, except if it's in a logical operator
+                if (sqlUnaryExpression.OperatorType is ExpressionType.Equal or ExpressionType.NotEqual
+                    && outerExpression is not SqlBinaryExpression
+                    {
+                        OperatorType: ExpressionType.AndAlso or ExpressionType.OrElse or ExpressionType.Not
+                    })
                 {
                     return true;
                 }

diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/GearsOfWarQuerySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/GearsOfWarQuerySqliteTest.cs
@@ -875,7 +875,7 @@ public override async Task Nullable_bool_comparison_is_translated_to_server(bool
         await base.Nullable_bool_comparison_is_translated_to_server(async);
 
         AssertSql(
-            @"SELECT ""f"".""Eradicated"" = 1 AND (""f"".""Eradicated"" IS NOT NULL) AS ""IsEradicated""
+            @"SELECT ""f"".""Eradicated"" = 1 AND ""f"".""Eradicated"" IS NOT NULL AS ""IsEradicated""
 FROM ""Factions"" AS ""f""");
     }
 
@@ -2458,10 +2458,10 @@ ELSE NULL
 WHERE CASE
     WHEN ""f"".""Name"" = 'Locust' THEN 1
     ELSE NULL
-END <> 1 OR (CASE
+END <> 1 OR CASE
     WHEN ""f"".""Name"" = 'Locust' THEN 1
     ELSE NULL
-END IS NULL)");
+END IS NULL");
     }
 
     public override async Task Non_unicode_string_literals_is_used_for_non_unicode_column_in_subquery(bool async)
@@ -4639,7 +4639,7 @@ LEFT JOIN (
     FROM ""Factions"" AS ""f""
     WHERE ""f"".""Name"" = 'Swarm'
 ) AS ""t"" ON ""l"".""Name"" = ""t"".""CommanderName""
-WHERE ""t"".""Eradicated"" <> 1 OR (""t"".""Eradicated"" IS NULL)");
+WHERE ""t"".""Eradicated"" <> 1 OR ""t"".""Eradicated"" IS NULL");
     }
 
     public override async Task Double_order_by_binary_expression(bool async)
@@ -4697,7 +4697,7 @@ INNER JOIN (
     FROM ""Factions"" AS ""f""
     WHERE ""f"".""Name"" = 'Swarm'
 ) AS ""t"" ON ""l"".""Name"" = ""t"".""CommanderName""
-WHERE ""t"".""Eradicated"" <> 1 OR (""t"".""Eradicated"" IS NULL)");
+WHERE ""t"".""Eradicated"" <> 1 OR ""t"".""Eradicated"" IS NULL");
     }
 
     public override async Task Where_subquery_join_firstordefault_boolean(bool async)

diff --git a/test/EFCore.Sqlite.FunctionalTests/Query/NullSemanticsQuerySqliteTest.cs b/test/EFCore.Sqlite.FunctionalTests/Query/NullSemanticsQuerySqliteTest.cs
@@ -8,10 +8,11 @@ namespace Microsoft.EntityFrameworkCore.Query;
 
 public class NullSemanticsQuerySqliteTest : NullSemanticsQueryTestBase<NullSemanticsQuerySqliteFixture>
 {
-    public NullSemanticsQuerySqliteTest(NullSemanticsQuerySqliteFixture fixture)
+    public NullSemanticsQuerySqliteTest(NullSemanticsQuerySqliteFixture fixture, ITestOutputHelper testOutputHelper)
         : base(fixture)
     {
         Fixture.TestSqlLoggerFactory.Clear();
+        //Fixture.TestSqlLoggerFactory.SetTestOutputHelper(testOutputHelper);
     }
 
     public override async Task Bool_equal_nullable_bool_HasValue(bool async)
@@ -72,10 +73,7 @@ public override async Task Bool_not_equal_nullable_bool_HasValue(bool async)
 
     public override async Task Bool_not_equal_nullable_int_HasValue(bool async)
     {
-        Assert.Equal(
-            "18",
-            (await Assert.ThrowsAsync<EqualException>(
-                () => base.Bool_not_equal_nullable_int_HasValue(async))).Actual);
+        await base.Bool_not_equal_nullable_int_HasValue(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""
@@ -86,11 +84,11 @@ public override async Task Bool_not_equal_nullable_int_HasValue(bool async)
 
 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 @__prm_0 <> ""e"".""NullableIntA"" IS NOT NULL",
+WHERE @__prm_0 <> (""e"".""NullableIntA"" IS NOT NULL)",
             //
             @"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 ""e"".""BoolB"" <> ""e"".""NullableIntA"" IS NOT NULL");
+WHERE ""e"".""BoolB"" <> (""e"".""NullableIntA"" IS NOT NULL)");
     }
 
     public override async Task Bool_not_equal_nullable_bool_compared_to_null(bool async)