Full management of SQL operator precedence/associativity

Removing unnecessary parentheses

Closes dotnet#26767

src/EFCore.Relational/Query/QuerySqlGenerator.cs

@@ -1,6 +1,7 @@
 // Licensed to the .NET Foundation under one or more agreements.
 // The .NET Foundation licenses this file to you under the MIT license.
 
+using System.Runtime.CompilerServices;
 using Microsoft.EntityFrameworkCore.Query.SqlExpressions;
 using Microsoft.EntityFrameworkCore.Storage.Internal;
 
@@ -954,62 +955,6 @@ protected override Expression VisitAtTimeZone(AtTimeZoneExpression atTimeZoneExp
     protected virtual string GetOperator(SqlBinaryExpression binaryExpression)
         => OperatorMap[binaryExpression.OperatorType];
 
-    /// <summary>
-    ///     Returns a bool value indicating if the inner SQL expression required to be put inside parenthesis
-    ///     when generating SQL for outer SQL expression.
-    /// </summary>
-    /// <param name="outerExpression">The outer expression which provides context in which SQL is being generated.</param>
-    /// <param name="innerExpression">The inner expression which may need to be put inside parenthesis.</param>
-    /// <returns>A bool value indicating that parenthesis is required or not. </returns>
-    protected virtual bool RequiresParentheses(SqlExpression outerExpression, SqlExpression innerExpression)
-    {
-        switch (innerExpression)
-        {
-            case AtTimeZoneExpression or LikeExpression:
-                return true;
-
-            case SqlUnaryExpression sqlUnaryExpression:
-            {
-                // Wrap IS (NOT) NULL operation
-                if (sqlUnaryExpression.OperatorType is ExpressionType.Equal or ExpressionType.NotEqual)
-                {
-                    return true;
-                }
-
-                if (sqlUnaryExpression.OperatorType == ExpressionType.Negate
-                    && outerExpression is SqlUnaryExpression { OperatorType: ExpressionType.Negate })
-                {
-                    // double negative sign is interpreted as a comment in SQL, so we need to enclose it in brackets
-                    return true;
-                }
-
-                return false;
-            }
-
-            case SqlBinaryExpression sqlBinaryExpression:
-            {
-                if (outerExpression is SqlBinaryExpression outerBinary)
-                {
-                    // Math, bitwise, comparison and equality operators have higher precedence
-                    if (outerBinary.OperatorType == ExpressionType.AndAlso)
-                    {
-                        return sqlBinaryExpression.OperatorType == ExpressionType.OrElse;
-                    }
-
-                    if (outerBinary.OperatorType == ExpressionType.OrElse)
-                    {
-                        // Precedence-wise AND is above OR but we still add parenthesis for ease of understanding
-                        return sqlBinaryExpression.OperatorType == ExpressionType.AndAlso;
-                    }
-                }
-
-                return true;
-            }
-        }
-
-        return false;
-    }
-
     /// <summary>
     ///     Generates a TOP construct in the relational command
     /// </summary>
@@ -1361,4 +1306,125 @@ void LiftPredicate(TableExpressionBase joinTable)
     protected override Expression VisitJsonScalar(JsonScalarExpression jsonScalarExpression)
         => throw new InvalidOperationException(
             RelationalStrings.JsonNodeMustBeHandledByProviderSpecificVisitor);
+
+    /// <summary>
+    ///     Returns a bool value indicating if the inner SQL expression required to be put inside parenthesis when generating SQL for outer
+    ///     SQL expression.
+    /// </summary>
+    /// <param name="outerExpression">The outer expression which provides context in which SQL is being generated.</param>
+    /// <param name="innerExpression">The inner expression which may need to be put inside parenthesis.</param>
+    /// <returns>A bool value indicating that parenthesis is required or not. </returns>
+    protected virtual bool RequiresParentheses(SqlExpression outerExpression, SqlExpression innerExpression)
+    {
+        int outerPrecedence, innerPrecedence;
+
+        // Convert is rendered as a function (CAST()) and not as an operator, so we never need to add parentheses around the inner
+        if (outerExpression is SqlUnaryExpression { OperatorType: ExpressionType.Convert })
+        {
+            return false;
+        }
+
+        switch (innerExpression)
+        {
+            case SqlUnaryExpression innerUnaryExpression:
+            {
+                // If the same unary operator is used in both outer and inner (e.g. NOT NOT), no parentheses are needed
+                if (outerExpression is SqlUnaryExpression outerUnary
+                    && innerUnaryExpression.OperatorType == outerUnary.OperatorType)
+                {
+                    // ... except for double negative (--), which is interpreted as a comment in SQL
+                    return innerUnaryExpression.OperatorType == ExpressionType.Negate;
+                }
+
+                // If the provider defined precedence for the two expression, use that
+                if (TryGetOperatorInfo(outerExpression, out outerPrecedence, out _)
+                    && TryGetOperatorInfo(innerExpression, out innerPrecedence, out _))
+                {
+                    return outerPrecedence >= innerPrecedence;
+                }
+
+                // Otherwise, wrap IS (NOT) NULL operation, except if it's in a logical operator
+                if (innerUnaryExpression.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 innerBinaryExpression:
+            {
+                // Precedence-wise AND is above OR but we still add parenthesis for ease of understanding
+                if (innerBinaryExpression.OperatorType is ExpressionType.AndAlso or ExpressionType.And
+                    && outerExpression is SqlBinaryExpression { OperatorType: ExpressionType.OrElse or ExpressionType.Or })
+                {
+                    return true;
+                }
+
+                // If the provider defined precedence for the two expression, use that
+                if (TryGetOperatorInfo(outerExpression, out outerPrecedence, out var isOuterAssociative)
+                    && TryGetOperatorInfo(innerExpression, out innerPrecedence, out _))
+                {
+                    return outerPrecedence.CompareTo(innerPrecedence) 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 != innerBinaryExpression.OperatorType
+                            || !isOuterAssociative
+                            // 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)
+                    };
+                }
+
+                // Otherwise always parenthesize for safety
+                return true;
+            }
+
+            case CollateExpression or LikeExpression or AtTimeZoneExpression:
+                return !TryGetOperatorInfo(outerExpression, out outerPrecedence, out _)
+                    || !TryGetOperatorInfo(innerExpression, out innerPrecedence, out _)
+                    || outerPrecedence >= innerPrecedence;
+
+            default:
+                return false;
+        }
+    }
+
+    /// <summary>
+    ///     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 and associativity.</param>
+    /// <param name="precedence">
+    ///     If the method returned <see langword="true" />, contains the precedence of the provided <paramref name="expression" />.
+    ///     Otherwise, contains default values.
+    /// </param>
+    /// <param name="isAssociative">
+    ///     If the method returned <see langword="true" />, contains the 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="precedence" /> and
+    ///     <paramref name="isAssociative" />. Otherwise, <see langword="false" />.
+    /// </returns>
+    /// <remarks>
+    ///     The default implementation always returns false, so that parentheses almost always get added. Providers can override this method
+    ///     to remove parentheses where they aren't necessary.
+    /// </remarks>
+    protected virtual bool TryGetOperatorInfo(SqlExpression expression, out int precedence, out bool isAssociative)
+    {
+        (precedence, isAssociative) = (default, default);
+        return false;
+    }
 }

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

@@ -296,7 +296,12 @@ when tableExpression.FindAnnotation(SqlServerAnnotationNames.TemporalOperationTy
         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 Expression VisitJsonScalar(JsonScalarExpression jsonScalarExpression)
     {
         if (jsonScalarExpression.Path.Count == 1
@@ -344,6 +349,62 @@ protected override void CheckComposableSqlTrimmed(ReadOnlySpan<char> sql)
         }
     }
 
+    /// <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, out bool isAssociative)
+    {
+        // See https://docs.microsoft.com/sql/t-sql/language-elements/operator-precedence-transact-sql, although that list is very partial
+        (precedence, isAssociative) = 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),
+            AtTimeZoneExpression => (1200, false),
+
+            _ => default,
+        };
+
+        return precedence != default;
+    }
+
     private void GenerateList<T>(
         IReadOnlyList<T> items,
         Action<T> generationAction,

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

@@ -105,4 +105,58 @@ private Expression VisitRegexp(RegexpExpression regexpExpression)
 
         return regexpExpression;
     }
+
+    /// <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, out bool isAssociative)
+    {
+        // See https://sqlite.org/lang_expr.html#operators_and_parse_affecting_attributes
+        (precedence, isAssociative) = 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 precedence != default;
+    }
 }

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

@@ -0,0 +1,62 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using Microsoft.EntityFrameworkCore.TestModels.Northwind;
+
+namespace Microsoft.EntityFrameworkCore.Query;
+
+public abstract class NorthwindOperatorsQueryTestBase<TFixture> : QueryTestBase<TFixture>
+    where TFixture : NorthwindQueryFixtureBase<NoopModelCustomizer>, new()
+{
+    protected NorthwindOperatorsQueryTestBase(TFixture fixture)
+        : base(fixture)
+    {
+    }
+
+    protected static ExpressionType[] BinaryArithmeticOperators { get; } =
+    {
+        ExpressionType.Add,
+        ExpressionType.Subtract,
+        ExpressionType.Multiply,
+        ExpressionType.Divide,
+        // TODO Complete...
+    };
+
+    [ConditionalTheory]
+    [MemberData(nameof(Get_binary_arithmetic_data))]
+    public virtual async Task Binary_arithmetic_operators(ExpressionType outer, ExpressionType inner)
+    {
+        var parameter = Expression.Parameter(typeof(Order), "o");
+        var predicate =
+            Expression.Lambda<Func<Order, int>>(
+                Expression.MakeBinary(
+                    outer,
+                    Expression.MakeBinary(
+                        inner,
+                        Expression.Property(parameter, nameof(Order.OrderID)),
+                        Expression.Constant(8)),
+                    Expression.Constant(9)),
+                parameter);
+
+        await AssertQueryScalar(
+            async: true,
+            ss => ss.Set<Order>().Where(o => o.OrderID == 10248).Select(predicate));
+    }
+
+    public static IEnumerable<object[]> Get_binary_arithmetic_data()
+        => from op1 in BinaryArithmeticOperators from op2 in BinaryArithmeticOperators select new object[] { op1, op2 };
+
+    [ConditionalTheory]
+    [MemberData(nameof(IsAsyncData))]
+    public virtual Task Double_negate_on_column(bool async)
+        => AssertQuery(
+            async,
+            ss => ss.Set<Order>().Where(o => -(-o.OrderID) == o.OrderID),
+            entryCount: 830);
+
+    // TODO: Test associativity (no parentheses on consecutive e.g. add operations)
+    // TODO: Test non-associativity of arithmetic operators on floating points aren't associative (because of rounding errors)
+
+    // TODO: Move operator/precedence related here, e.g. NullSemanticsQueryTestBase.Bool_not_equal_nullable_int_HasValue,
+    // GearsOfWarTestBase.Negate_on_binary_expression...
+}