More conservative operator precedence table approach

Requiring provider opt-in, and falling back to a safer approach if the provider hasn't defined anything

Closes #27253

src/EFCore.Relational/Query/QuerySqlGenerator.cs

@@ -844,59 +844,67 @@ protected virtual bool RequiresParentheses(SqlExpression outerExpression, SqlExp
         {
             case SqlUnaryExpression sqlUnaryExpression:
             {
-                // Double negative sign is interpreted as a comment in SQL, so we need to enclose it in parentheses
-                if (sqlUnaryExpression.OperatorType == ExpressionType.Negate
-                    && outerExpression is SqlUnaryExpression { OperatorType: ExpressionType.Negate })
-                {
-                    return true;
-                }
-
                 // If the same unary operator is used in both outer and inner (e.g. CAST(CAST(...)), no parentheses are needed
                 if (outerExpression is SqlUnaryExpression outerUnary
                     && sqlUnaryExpression.OperatorType == outerUnary.OperatorType)
                 {
-                    return false;
+                    // ... except for double negative (--), which is interpreted as a comment in SQL
+                    return sqlUnaryExpression.OperatorType == ExpressionType.Negate;
+                }
+
+                // If the provider defined precedence for the two expression, use that
+                if (TryGetOperatorInfo(outerExpression, out var outerOperatorInfo)
+                    && TryGetOperatorInfo(innerExpression, out var innerOperatorInfo))
+                {
+                    return outerOperatorInfo.Precedence >= innerOperatorInfo.Precedence;
                 }
 
-                return TryGetOperatorInfo(outerExpression, out var outerOperatorInfo)
-                    && TryGetOperatorInfo(innerExpression, out var innerOperatorInfo)
-                    && outerOperatorInfo.Precedence >= innerOperatorInfo.Precedence;
+                // Otherwise, 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;
+                }
+
+                return false;
             }
 
             case SqlBinaryExpression sqlBinaryExpression:
             {
                 // Precedence-wise AND is above OR but we still add parenthesis for ease of understanding
                 if (sqlBinaryExpression.OperatorType is ExpressionType.AndAlso or ExpressionType.And
                     && outerExpression is SqlBinaryExpression { OperatorType: ExpressionType.OrElse or ExpressionType.Or })
-                {
-                    return sqlBinaryExpression.OperatorType == ExpressionType.AndAlso;
-                }
-
-                // // Precedence-wise AND is above OR but we still add parenthesis for ease of understanding
-                // if (sqlBinaryExpression.OperatorType is ExpressionType.AndAlso or ExpressionType.And
-                //     && outerExpression is SqlBinaryExpression { OperatorType: ExpressionType.OrElse or ExpressionType.Or })
-                // {
-                //     return true;
-                // }
-
-                // Always add parentheses if one of the operators is unknown, to be on the safe side
-                if (!TryGetOperatorInfo(outerExpression, out var outerOperatorInfo)
-                    || !TryGetOperatorInfo(innerExpression, out var innerOperatorInfo))
                 {
                     return true;
                 }
 
-                // If both operators have the same precedence, add parentheses unless they're the same operator, and
-                // that operator is associative (e.g. AND...AND...)
-                if (outerOperatorInfo.Precedence == innerOperatorInfo.Precedence)
+                // If the provider defined precedence for the two expression, use that
+                if (TryGetOperatorInfo(outerExpression, out var outerOperatorInfo)
+                    && TryGetOperatorInfo(innerExpression, out var innerOperatorInfo))
                 {
-                    return outerExpression is not SqlBinaryExpression outerBinary
-                        || outerBinary.OperatorType != sqlBinaryExpression.OperatorType
-                        || !innerOperatorInfo.IsAssociative;
+                    return outerOperatorInfo.Precedence.CompareTo(innerOperatorInfo.Precedence) switch
+                    {
+                        > 0 => true,
+                        < 0 => false,
+
+                        // If both operators have the same precedence, add parentheses unless they're the same operator, and
+                        // that operator is associative (e.g. a + b + c)
+                        0 => outerExpression is not SqlBinaryExpression outerBinary
+                            || outerBinary.OperatorType != sqlBinaryExpression.OperatorType
+                            || !innerOperatorInfo.IsAssociative
+                            // Arithmetic operators on floating points aren't associative, because of rounding errors.
+                            || outerExpression.Type == typeof(float)
+                            || outerExpression.Type == typeof(double)
+                            || innerExpression.Type == typeof(float)
+                            || innerExpression.Type == typeof(double)
+                    };
                 }
 
-                // Finally, add parentheses based on the precedence
-                return outerOperatorInfo.Precedence > innerOperatorInfo.Precedence;
+                // Otherwise always parenthesize for safety
+                return true;
             }
 
             case CollateExpression:
@@ -913,70 +921,22 @@ protected virtual bool RequiresParentheses(SqlExpression outerExpression, SqlExp
     }
 
     /// <summary>
-    ///     Returns a numeric value representing the precedence of the given <paramref name="expression" />. If an expression with a lower
-    ///     precedence value is contained with an expression with a higher value, parentheses are added.
+    ///     Returns a numeric value representing the precedence of the given <paramref name="expression" />, as well as its associativity.
+    ///     These control whether parentheses are generated around the expression.
     /// </summary>
-    /// <param name="expression">The expression for which to get the precedence value.</param>
-    /// <param name="operatorInfo">If t</param>
+    /// <param name="expression">The expression for which to get the precedence and associativity.</param>
+    /// <param name="operatorInfo">
+    ///     If the method returned <see langword="true" />, contains the precedence and associativity of the provided
+    ///     <paramref name="expression" />. Otherwise, contains default values.
+    /// </param>
     /// <returns>
     ///     <see langword="true" /> if the expression operator info is known and was returned in <paramref name="operatorInfo" />.
     ///     Otherwise, <see langword="false" />.
     /// </returns>
     protected virtual bool TryGetOperatorInfo(SqlExpression expression, out (int Precedence, bool IsAssociative) operatorInfo)
     {
-        operatorInfo = expression switch
-        {
-            SqlBinaryExpression sqlBinaryExpression => sqlBinaryExpression.OperatorType switch
-            {
-                //ExpressionType.ArrayIndex => 1200,
-                //ExpressionType.Power => 1000,
-                ExpressionType.Multiply => (900, true),
-                ExpressionType.Divide => (800, false),
-                ExpressionType.Modulo => (800, false),
-                ExpressionType.Add => (800, true),
-                ExpressionType.Subtract => (700, false),
-                ExpressionType.And => (600, true),
-                ExpressionType.Or => (600, true),
-                //ExpressionType.ExclusiveOr => (600, false),
-                //ExpressionType.RightShift => (600, false),
-                //ExpressionType.LeftShift => (600, false),
-                ExpressionType.LessThan => (500, false),
-                ExpressionType.LessThanOrEqual => (500, false),
-                ExpressionType.GreaterThan => (500, false),
-                ExpressionType.GreaterThanOrEqual => (500, false),
-                ExpressionType.Equal => (500, false),
-                ExpressionType.NotEqual => (500, false),
-                ExpressionType.AndAlso => (200, true),
-                ExpressionType.OrElse => (100, true),
-
-                _ => default,
-            },
-
-            SqlUnaryExpression sqlUnaryExpression => sqlUnaryExpression.OperatorType switch
-            {
-                ExpressionType.Convert => (1300, false),
-                ExpressionType.Not when sqlUnaryExpression.Type != typeof(bool) => (1200, false),
-                ExpressionType.Negate => (1100, false),
-                ExpressionType.Equal => (400, false), // IS NULL
-                ExpressionType.NotEqual => (400, false), // IS NOT NULL
-                ExpressionType.Not when sqlUnaryExpression.Type == typeof(bool) => (300, false),
-
-                _ => default,
-            },
-
-            CollateExpression => (700, false),
-            LikeExpression => (200, false),
-
-            _ => default,
-        };
-
-        // Technically, arithmetic operators on floating points aren't arithmetic, because of rounding errors.
-        if (operatorInfo.IsAssociative && (expression.Type == typeof(float) || expression.Type == typeof(double)))
-        {
-            operatorInfo.IsAssociative = false;
-        }
-
-        return operatorInfo != default;
+        operatorInfo = default;
+        return false;
     }
 
     /// <summary>

src/EFCore.SqlServer/Query/Internal/SqlServerQuerySqlGenerator.cs

@@ -169,7 +169,12 @@ protected override Expression VisitExtension(Expression extensionExpression)
         return base.VisitExtension(extensionExpression);
     }
 
-    /// <inheritdoc />
+    /// <summary>
+    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+    /// </summary>
     protected override void CheckComposableSqlTrimmed(ReadOnlySpan<char> sql)
     {
         base.CheckComposableSqlTrimmed(sql);
@@ -179,4 +184,59 @@ protected override void CheckComposableSqlTrimmed(ReadOnlySpan<char> sql)
             throw new InvalidOperationException(RelationalStrings.FromSqlNonComposable);
         }
     }
+
+    /// <summary>
+    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+    /// </summary>
+    protected override bool TryGetOperatorInfo(SqlExpression expression, out (int Precedence, bool IsAssociative) operatorInfo)
+    {
+        // See https://docs.microsoft.com/sql/t-sql/language-elements/operator-precedence-transact-sql
+        operatorInfo = expression switch
+        {
+            SqlBinaryExpression sqlBinaryExpression => sqlBinaryExpression.OperatorType switch
+            {
+                ExpressionType.Multiply => (900, true),
+                ExpressionType.Divide => (900, false),
+                ExpressionType.Modulo => (900, false),
+                ExpressionType.Add => (800, true),
+                ExpressionType.Subtract => (800, false),
+                ExpressionType.And => (700, true),
+                ExpressionType.Or => (700, true),
+                ExpressionType.LeftShift => (700, true),
+                ExpressionType.RightShift => (700, true),
+                ExpressionType.LessThan => (600, false),
+                ExpressionType.LessThanOrEqual => (600, false),
+                ExpressionType.GreaterThan => (600, false),
+                ExpressionType.GreaterThanOrEqual => (600, false),
+                ExpressionType.Equal => (500, false),
+                ExpressionType.NotEqual => (500, false),
+                ExpressionType.AndAlso => (200, true),
+                ExpressionType.OrElse => (100, true),
+
+                _ => default,
+            },
+
+            SqlUnaryExpression sqlUnaryExpression => sqlUnaryExpression.OperatorType switch
+            {
+                ExpressionType.Convert => (1300, false),
+                ExpressionType.Not when sqlUnaryExpression.Type != typeof(bool) => (1200, false),
+                ExpressionType.Negate => (1100, false),
+                ExpressionType.Equal => (500, false), // IS NULL
+                ExpressionType.NotEqual => (500, false), // IS NOT NULL
+                ExpressionType.Not when sqlUnaryExpression.Type == typeof(bool) => (300, false),
+
+                _ => default,
+            },
+
+            CollateExpression => (900, false),
+            LikeExpression => (350, false),
+
+            _ => default,
+        };
+
+        return operatorInfo != default;
+    }
 }

src/EFCore.Sqlite.Core/Query/Internal/SqliteQuerySqlGenerator.cs

@@ -72,4 +72,58 @@ protected override void GenerateLimitOffset(SelectExpression selectExpression)
     protected override void GenerateSetOperationOperand(SetOperationBase setOperation, SelectExpression operand)
         // Sqlite doesn't support parentheses around set operation operands
         => Visit(operand);
+
+    /// <summary>
+    ///     This is an internal API that supports the Entity Framework Core infrastructure and not subject to
+    ///     the same compatibility standards as public APIs. It may be changed or removed without notice in
+    ///     any release. You should only use it directly in your code with extreme caution and knowing that
+    ///     doing so can result in application failures when updating to a new Entity Framework Core release.
+    /// </summary>
+    protected override bool TryGetOperatorInfo(SqlExpression expression, out (int Precedence, bool IsAssociative) operatorInfo)
+    {
+        // See https://sqlite.org/lang_expr.html#operators_and_parse_affecting_attributes
+        operatorInfo = expression switch
+        {
+            SqlBinaryExpression sqlBinaryExpression => sqlBinaryExpression.OperatorType switch
+            {
+                ExpressionType.Multiply => (900, true),
+                ExpressionType.Divide => (900, false),
+                ExpressionType.Modulo => (900, false),
+                ExpressionType.Add when sqlBinaryExpression.Type == typeof(string) => (1100, true),
+                ExpressionType.Add when sqlBinaryExpression.Type != typeof(string) => (800, true),
+                ExpressionType.Subtract => (800, false),
+                ExpressionType.And => (600, true),
+                ExpressionType.Or => (600, true),
+                ExpressionType.LessThan => (500, false),
+                ExpressionType.LessThanOrEqual => (500, false),
+                ExpressionType.GreaterThan => (500, false),
+                ExpressionType.GreaterThanOrEqual => (500, false),
+                ExpressionType.Equal => (500, false),
+                ExpressionType.NotEqual => (500, false),
+                ExpressionType.AndAlso => (200, true),
+                ExpressionType.OrElse => (100, true),
+
+                _ => default,
+            },
+
+            SqlUnaryExpression sqlUnaryExpression => sqlUnaryExpression.OperatorType switch
+            {
+                ExpressionType.Convert => (1300, false),
+                ExpressionType.Not when sqlUnaryExpression.Type != typeof(bool) => (1200, false),
+                ExpressionType.Negate => (1200, false),
+                ExpressionType.Equal => (500, false), // IS NULL
+                ExpressionType.NotEqual => (500, false), // IS NOT NULL
+                ExpressionType.Not when sqlUnaryExpression.Type == typeof(bool) => (300, false),
+
+                _ => default,
+            },
+
+            CollateExpression => (1100, false),
+            LikeExpression => (500, false),
+
+            _ => default,
+        };
+
+        return operatorInfo != default;
+    }
 }

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

@@ -138,7 +138,7 @@ public override async Task Include_collection_with_conditional_order_by(bool asy
 FROM [LevelOne] AS [l]
 LEFT JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id]
 ORDER BY CASE
-    WHEN [l].[Name] IS NOT NULL AND ([l].[Name] LIKE N'%03') THEN 1
+    WHEN [l].[Name] IS NOT NULL AND [l].[Name] LIKE N'%03' THEN 1
     ELSE 2
 END, [l].[Id]");
     }

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

@@ -519,7 +519,7 @@ FROM [Level1] AS [l0]
     WHERE [l0].[OneToOne_Required_PK_Date] IS NOT NULL AND [l0].[Level1_Required_Id] IS NOT NULL AND [l0].[OneToMany_Required_Inverse2Id] IS NOT NULL
 ) AS [t] ON [l].[Id] = [t].[OneToMany_Optional_Inverse2Id]
 ORDER BY CASE
-    WHEN [l].[Name] IS NOT NULL AND ([l].[Name] LIKE N'%03') THEN 1
+    WHEN [l].[Name] IS NOT NULL AND [l].[Name] LIKE N'%03' THEN 1
     ELSE 2
 END, [l].[Id], [t].[Id]");
     }

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

@@ -1629,15 +1629,15 @@ public override async Task Include_collection_with_conditional_order_by(bool asy
             @"SELECT [l].[Id], [l].[Date], [l].[Name], [l].[OneToMany_Optional_Self_Inverse1Id], [l].[OneToMany_Required_Self_Inverse1Id], [l].[OneToOne_Optional_Self1Id]
 FROM [LevelOne] AS [l]
 ORDER BY CASE
-    WHEN [l].[Name] IS NOT NULL AND ([l].[Name] LIKE N'%03') THEN 1
+    WHEN [l].[Name] IS NOT NULL AND [l].[Name] LIKE N'%03' THEN 1
     ELSE 2
 END, [l].[Id]",
             //
             @"SELECT [l0].[Id], [l0].[Date], [l0].[Level1_Optional_Id], [l0].[Level1_Required_Id], [l0].[Name], [l0].[OneToMany_Optional_Inverse2Id], [l0].[OneToMany_Optional_Self_Inverse2Id], [l0].[OneToMany_Required_Inverse2Id], [l0].[OneToMany_Required_Self_Inverse2Id], [l0].[OneToOne_Optional_PK_Inverse2Id], [l0].[OneToOne_Optional_Self2Id], [l].[Id]
 FROM [LevelOne] AS [l]
 INNER JOIN [LevelTwo] AS [l0] ON [l].[Id] = [l0].[OneToMany_Optional_Inverse2Id]
 ORDER BY CASE
-    WHEN [l].[Name] IS NOT NULL AND ([l].[Name] LIKE N'%03') THEN 1
+    WHEN [l].[Name] IS NOT NULL AND [l].[Name] LIKE N'%03' THEN 1
     ELSE 2
 END, [l].[Id]");
     }