Fix non-Boolean comparison operator results (#561)

src/FastExpressionCompiler/FastExpressionCompiler.cs

@@ -2011,6 +2011,9 @@ public static bool TryEmit(Expression expr, ILGenerator il, ref CompilerContext
                         case ExpressionType.Equal:
                         case ExpressionType.NotEqual:
                             {
+                                var comparison = (BinaryExpression)expr;
+                                if (comparison.Method != null && !comparison.IsLifted)
+                                    return TryEmitComparisonMethod(comparison, il, ref context, parent);
                                 if (exprType.IsPrimitive && Interpreter.TryInterpretBool(out var boolResult, expr, context.CompilerFlags))
                                 {
                                     if ((parent & ParentFlags.IgnoreResult) == 0)
@@ -5521,6 +5524,47 @@ private static MethodInfo FindBinaryOperandMethod(
                 return null;
             }
 
+            private static bool TryEmitComparisonMethod(BinaryExpression expr, ILGenerator il, ref CompilerContext context, ParentFlags parent)
+            {
+                var method = expr.Method;
+                var parameters = method.GetParameters();
+                if (!TryEmit(expr.Left, il, ref context, ParentFlags.Call, parameters[0].ParameterType.IsByRef ? 0 : -1))
+                    return false;
+
+                // Keep the evaluation stack empty while a nontrivial right operand
+                // may emit exception regions or jumps, just as for call arguments.
+                var right = expr.Right;
+                var spillLeft = !right.IsParamOrConstantOrDefault();
+#if LIGHT_EXPRESSION
+                // A conversion chain may contain a custom IL emitter.
+                for (var operand = right; !spillLeft;)
+                {
+                    if (operand.IsIntrinsic)
+                    {
+                        spillLeft = true;
+                        break;
+                    }
+                    if (operand is not UnaryExpression convert || convert.NodeType != ExpressionType.Convert)
+                        break;
+                    operand = convert.Operand;
+                }
+#endif
+                var leftVar = spillLeft ? EmitStoreLocalVariable(il, parameters[0].ParameterType) : -1;
+
+                if (!TryEmit(expr.Right, il, ref context, ParentFlags.Call, parameters[1].ParameterType.IsByRef ? 1 : -1))
+                    return false;
+                if (leftVar != -1)
+                {
+                    var rightVar = EmitStoreLocalVariable(il, parameters[1].ParameterType);
+                    EmitLoadLocalVariable(il, leftVar);
+                    EmitLoadLocalVariable(il, rightVar);
+                }
+                if (!EmitMethodCall(il, method))
+                    return false;
+                context.LastEmitIsAddress = false;
+                return il.EmitPopIfIgnoreResult(parent);
+            }
+
             private static bool TryEmitComparison(
                 Expression left, Expression right, Type exprType, ExpressionType nodeType,
                 ILGenerator il, ref CompilerContext context, ParentFlags parent)
@@ -7310,6 +7354,8 @@ public static bool TryFindSwitchBranch(SwitchExpression switchExpr, CompilerFlag
             /// <summary>Tries to interpret the expression of the Primitive type of Constant, Convert, Logical, Comparison, Arithmetic.</summary>
             internal static bool TryInterpretBool(ref bool resultBool, Expression expr, ExpressionType nodeType)
             {
+                if (expr is BinaryExpression binary && binary.Method != null)
+                    return false;
                 // what operations are supported, to get the boolean result?
                 // yes: not, logical, comparison, default
                 // not: negate, arithmetic, convert to bool

test/FastExpressionCompiler.IssueTests/Issue561_NonBooleanEqualityOperator.cs

@@ -0,0 +1,281 @@
+// The safety guard uses the dynamic-method decoder supported on modern .NET.
+// The fatal repro itself is also exercised in a separate child-process harness.
+#if NET8_0_OR_GREATER
+using System;
+using System.Linq;
+using System.Linq.Expressions;
+using System.Reflection;
+
+#if LIGHT_EXPRESSION
+using FastExpressionCompiler.LightExpression.ILDecoder;
+using FastExpressionCompiler.LightExpression.ImTools;
+using static FastExpressionCompiler.LightExpression.ExpressionCompiler;
+namespace FastExpressionCompiler.LightExpression.IssueTests;
+#else
+using FastExpressionCompiler.ILDecoder;
+namespace FastExpressionCompiler.IssueTests;
+#endif
+
+public struct Issue561_NonBooleanEqualityOperator : ITestX
+{
+    public void Run(TestRun t)
+    {
+        Equal_returns_the_interface_from_the_selected_operator(t);
+        NotEqual_returns_the_interface_from_the_selected_operator(t);
+        Reversed_equal_returns_the_interface_from_the_selected_operator(t);
+        Reversed_not_equal_returns_the_interface_from_the_selected_operator(t);
+        Ignored_result_still_calls_the_operator(t);
+        Operands_execute_once_in_order_with_nested_try(t);
+        Non_lifted_nullable_parameters_reach_the_selected_method(t);
+        Selected_methods_keep_non_boolean_value_results(t);
+        Boolean_method_is_not_replaced_by_constant_comparison(t);
+        Converted_simple_right_operands_reach_the_selected_method(t);
+#if LIGHT_EXPRESSION
+        Converted_intrinsic_right_operand_can_emit_exception_regions(t);
+#endif
+    }
+
+    public interface ICustomObject { void Ping(); }
+
+    private static string Trace;
+
+    public sealed class CustomObject : ICustomObject
+    {
+        public static int OperatorCalls;
+        public static int PingCalls;
+
+        public static ICustomObject operator ==(double left, CustomObject right) => Result();
+        public static ICustomObject operator !=(double left, CustomObject right) => Result();
+        public static ICustomObject operator ==(CustomObject left, double right) => Result();
+        public static ICustomObject operator !=(CustomObject left, double right) => Result();
+        private static ICustomObject Result() { ++OperatorCalls; Trace += "O"; return new CustomObject(); }
+        public void Ping() { ++PingCalls; }
+        public override bool Equals(object obj) => ReferenceEquals(this, obj);
+        public override int GetHashCode() => base.GetHashCode();
+    }
+
+    public void Equal_returns_the_interface_from_the_selected_operator(TestContext t) =>
+        Check(t, () => 1 == new CustomObject());
+
+    public void NotEqual_returns_the_interface_from_the_selected_operator(TestContext t) =>
+        Check(t, () => 1 != new CustomObject());
+
+    public void Reversed_equal_returns_the_interface_from_the_selected_operator(TestContext t) =>
+        Check(t, () => new CustomObject() == 1);
+
+    public void Reversed_not_equal_returns_the_interface_from_the_selected_operator(TestContext t) =>
+        Check(t, () => new CustomObject() != 1);
+
+    private static void Check(TestContext t, System.Linq.Expressions.Expression<Func<ICustomObject>> expression)
+    {
+        var binary = (System.Linq.Expressions.BinaryExpression)expression.Body;
+        t.AreEqual(typeof(ICustomObject), binary.Type);
+        t.IsNotNull(binary.Method);
+        t.IsTrue(!binary.IsLifted);
+
+        CustomObject.OperatorCalls = CustomObject.PingCalls = 0;
+        var normal = expression.Compile()();
+        t.AreEqual(1, CustomObject.OperatorCalls);
+        normal.Ping();
+        t.AreEqual(1, CustomObject.PingCalls);
+
+        foreach (var flags in new[] { CompilerFlags.Default, CompilerFlags.NoInvocationLambdaInlining,
+            CompilerFlags.EnableDelegateDebugInfo })
+        {
+            var fast = Compile(expression, flags);
+            if (!t.IsNotNull(fast)) return;
+            if (!CheckSelectedCall(t, fast, binary.Method)) return;
+
+            CustomObject.OperatorCalls = CustomObject.PingCalls = 0;
+            var result = fast();
+            t.AreEqual(1, CustomObject.OperatorCalls);
+            if (!t.IsNotNull(result)) return;
+            t.IsTrue(result is CustomObject);
+            result.Ping();
+            t.AreEqual(1, CustomObject.PingCalls);
+        }
+    }
+
+    private static TDelegate Compile<TDelegate>(System.Linq.Expressions.Expression<TDelegate> expression,
+        CompilerFlags flags = CompilerFlags.Default) where TDelegate : Delegate =>
+#if LIGHT_EXPRESSION
+        expression.ToLightExpression().CompileFast(true, flags);
+#else
+        expression.CompileFast(true, flags);
+#endif
+
+    private static bool CheckSelectedCall(TestContext t, Delegate fast, MethodInfo method)
+    {
+        // On affected compilers the returned reference is corrupt. Check the
+        // selected call before invocation so this regression fails without
+        // taking down the whole runner. This does not assert an IL layout.
+        var instructions = fast.TryGetDebugInfo()?.ILInstructions ?? fast.Method.ReadAllInstructions();
+        if (instructions.OfType<InlineMethodInstruction>().Any(i => i.Method.Equals(method))) return true;
+        t.Fail("Issue561: selected operator call is missing; corrupt-reference delegate was not invoked.");
+        return false;
+    }
+
+    public void Ignored_result_still_calls_the_operator(TestContext t)
+    {
+        System.Linq.Expressions.Expression<Func<ICustomObject>> original = () => 1 == new CustomObject();
+        var method = ((System.Linq.Expressions.BinaryExpression)original.Body).Method;
+        var expression = System.Linq.Expressions.Expression.Lambda<Action>(
+            System.Linq.Expressions.Expression.Block(original.Body, System.Linq.Expressions.Expression.Empty()));
+        var fast = Compile(expression);
+        if (!t.IsNotNull(fast) || !CheckSelectedCall(t, fast, method)) return;
+        CustomObject.OperatorCalls = 0;
+        fast();
+        t.AreEqual(1, CustomObject.OperatorCalls);
+    }
+
+    public static double ReadLeft() { Trace += "L"; return 1; }
+    public static int ReadArgument() { Trace += "A"; return 2; }
+    public static void FinishArgument() { Trace += "F"; }
+    public static CustomObject ReadRight(int value) { Trace += "R"; return new CustomObject(); }
+
+    public void Operands_execute_once_in_order_with_nested_try(TestContext t)
+    {
+        var owner = typeof(Issue561_NonBooleanEqualityOperator);
+        var left = System.Linq.Expressions.Expression.Call(owner.GetMethod(nameof(ReadLeft)));
+        var right = System.Linq.Expressions.Expression.Call(owner.GetMethod(nameof(ReadRight)),
+            System.Linq.Expressions.Expression.TryFinally(
+                System.Linq.Expressions.Expression.Call(owner.GetMethod(nameof(ReadArgument))),
+                System.Linq.Expressions.Expression.Call(owner.GetMethod(nameof(FinishArgument)))));
+        var body = System.Linq.Expressions.Expression.Equal(left, right);
+        var expression = System.Linq.Expressions.Expression.Lambda<Func<ICustomObject>>(body);
+        var system = expression.Compile();
+        Trace = ""; system().Ping();
+        var expected = Trace;
+        t.AreEqual("LAFRO", expected);
+        var fast = Compile(expression);
+        if (!t.IsNotNull(fast) || !CheckSelectedCall(t, fast, body.Method)) return;
+        Trace = ""; CustomObject.OperatorCalls = CustomObject.PingCalls = 0;
+        fast().Ping();
+        t.AreEqual(expected, Trace);
+        t.AreEqual(1, CustomObject.OperatorCalls);
+        t.AreEqual(1, CustomObject.PingCalls);
+    }
+
+    public static ICustomObject CompareNullable(double? left, CustomObject right)
+    {
+        Trace += left.HasValue ? "value" : "null";
+        ++CustomObject.OperatorCalls;
+        return new CustomObject();
+    }
+
+    public void Non_lifted_nullable_parameters_reach_the_selected_method(TestContext t)
+    {
+        var left = System.Linq.Expressions.Expression.Parameter(typeof(double?));
+        var body = System.Linq.Expressions.Expression.Equal(left,
+            System.Linq.Expressions.Expression.New(typeof(CustomObject)), false,
+            typeof(Issue561_NonBooleanEqualityOperator).GetMethod(nameof(CompareNullable)));
+        t.IsTrue(!body.IsLifted);
+        var expression = System.Linq.Expressions.Expression.Lambda<Func<double?, ICustomObject>>(body, left);
+        var system = expression.Compile();
+        var fast = Compile(expression);
+        if (!t.IsNotNull(fast) || !CheckSelectedCall(t, fast, body.Method)) return;
+        foreach (var value in new double?[] { null, 2 })
+        {
+            Trace = ""; system(value).Ping(); var expected = Trace;
+            Trace = ""; CustomObject.OperatorCalls = CustomObject.PingCalls = 0;
+            fast(value).Ping();
+            t.AreEqual(expected, Trace);
+            t.AreEqual(1, CustomObject.OperatorCalls);
+            t.AreEqual(1, CustomObject.PingCalls);
+        }
+    }
+
+    public static int CompareValue(int left, int right) { ++CustomObject.OperatorCalls; return left * 10 + right; }
+    public static bool CompareBoolean(int left, int right) { ++CustomObject.OperatorCalls; return left != right; }
+
+    public void Selected_methods_keep_non_boolean_value_results(TestContext t)
+    {
+        var left = System.Linq.Expressions.Expression.Parameter(typeof(int));
+        var method = typeof(Issue561_NonBooleanEqualityOperator).GetMethod(nameof(CompareValue));
+        foreach (var kind in new[] { ExpressionType.Equal, ExpressionType.NotEqual, ExpressionType.LessThan,
+            ExpressionType.LessThanOrEqual, ExpressionType.GreaterThan, ExpressionType.GreaterThanOrEqual })
+        {
+            var body = System.Linq.Expressions.Expression.MakeBinary(kind, left,
+                System.Linq.Expressions.Expression.Constant(2), false, method);
+            var expression = System.Linq.Expressions.Expression.Lambda<Func<int, int>>(body, left);
+            var fast = Compile(expression);
+            if (!t.IsNotNull(fast) || !CheckSelectedCall(t, fast, method)) return;
+            var expected = expression.Compile()(3);
+            CustomObject.OperatorCalls = 0;
+            t.AreEqual(expected, fast(3));
+            t.AreEqual(1, CustomObject.OperatorCalls);
+        }
+    }
+
+    public void Boolean_method_is_not_replaced_by_constant_comparison(TestContext t)
+    {
+        var method = typeof(Issue561_NonBooleanEqualityOperator).GetMethod(nameof(CompareBoolean));
+        var body = System.Linq.Expressions.Expression.Equal(System.Linq.Expressions.Expression.Constant(1),
+            System.Linq.Expressions.Expression.Constant(2), false, method);
+        var expression = System.Linq.Expressions.Expression.Lambda<Func<bool>>(body);
+        var fast = Compile(expression);
+        if (!t.IsNotNull(fast) || !CheckSelectedCall(t, fast, method)) return;
+        var expected = expression.Compile()();
+        CustomObject.OperatorCalls = 0;
+        t.AreEqual(expected, fast());
+        t.AreEqual(1, CustomObject.OperatorCalls);
+    }
+    public void Converted_simple_right_operands_reach_the_selected_method(TestContext t)
+    {
+        var parameter = System.Linq.Expressions.Expression.Parameter(typeof(int));
+        var method = typeof(Issue561_NonBooleanEqualityOperator).GetMethod(nameof(CompareValue));
+        foreach (var right in new System.Linq.Expressions.Expression[] { parameter,
+            System.Linq.Expressions.Expression.Constant(2), System.Linq.Expressions.Expression.Default(typeof(int)) })
+        {
+            var converted = System.Linq.Expressions.Expression.Convert(
+                System.Linq.Expressions.Expression.Convert(right, typeof(long)), typeof(int));
+            var body = System.Linq.Expressions.Expression.Equal(
+                System.Linq.Expressions.Expression.Constant(3), converted, false, method);
+            var expression = System.Linq.Expressions.Expression.Lambda<Func<int, int>>(body, parameter);
+            var fast = Compile(expression);
+            if (!t.IsNotNull(fast) || !CheckSelectedCall(t, fast, method)) return;
+            var expected = expression.Compile()(4);
+            CustomObject.OperatorCalls = 0;
+            t.AreEqual(expected, fast(4));
+            t.AreEqual(1, CustomObject.OperatorCalls);
+        }
+    }
+
+#if LIGHT_EXPRESSION
+    private sealed class TryFinallyConversion : UnaryExpression
+    {
+        private readonly Expression _body;
+        public TryFinallyConversion(Expression operand, Expression body) : base(operand) => _body = body;
+        public override ExpressionType NodeType => ExpressionType.Convert;
+        public override bool IsIntrinsic => true;
+        public override Result TryCollectInfo(ref CompilerContext context, NestedLambdaInfo nestedLambda,
+            ref SmallList<NestedLambdaInfo> rootNestedLambdas) =>
+            ExpressionCompiler.TryCollectInfo(ref context, _body, nestedLambda, ref rootNestedLambdas);
+        public override bool TryEmit(ref CompilerContext context, System.Reflection.Emit.ILGenerator il,
+            ParentFlags parent, int byRefIndex = -1) =>
+            EmittingVisitor.TryEmit(_body, il, ref context, parent, byRefIndex);
+    }
+
+    public void Converted_intrinsic_right_operand_can_emit_exception_regions(TestContext t)
+    {
+        var owner = typeof(Issue561_NonBooleanEqualityOperator);
+        var parameter = Expression.Parameter(typeof(int));
+        var read = Expression.Call(owner.GetMethod(nameof(ReadArgument)));
+        var intrinsic = new TryFinallyConversion(parameter,
+            Expression.TryFinally(read, Expression.Call(owner.GetMethod(nameof(FinishArgument)))));
+        // The intrinsic is hidden inside two ordinary conversion wrappers.
+        var right = Expression.Convert(Expression.Convert(intrinsic, typeof(long)), typeof(int));
+        var method = owner.GetMethod(nameof(CompareValue));
+        var body = Expression.Equal(read, right, false, method);
+        var fast = Expression.Lambda<Func<int, int>>(body, parameter).CompileFast(true);
+        if (!t.IsNotNull(fast) || !CheckSelectedCall(t, fast, method)) return;
+        Trace = "";
+        CustomObject.OperatorCalls = 0;
+        t.AreEqual(22, fast(4));
+        t.AreEqual("AAF", Trace);
+        t.AreEqual(1, CustomObject.OperatorCalls);
+    }
+#endif
+
+}
+#endif

test/FastExpressionCompiler.TestsRunner/Program.cs

@@ -53,6 +53,7 @@ public static void Main()
             var st = new TestRun(TestFlags.RethrowException);
 
             st.Run(new Issue499_InvalidProgramException_for_Sorting_and_comparison_function());
+            st.Run(new Issue561_NonBooleanEqualityOperator());
             st.Run(new Issue498_InvalidProgramException_when_using_loop());
             st.Run(new Issue495_Incomplete_pattern_detection_for_NotSupported_1007_Return_goto_from_TryCatch_with_Assign_generates_invalid_IL());
             st.Run(new Issue480_CLR_detected_an_invalid_program_exception());
@@ -75,6 +76,7 @@ public static void Main()
             lt.Run(new LightExpression.IssueTests.Issue473_InvalidProgramException_when_using_Expression_Condition_with_converted_decimal_expression());
             lt.Run(new LightExpression.IssueTests.Issue476_System_ExecutionEngineException_with_nullables_on_repeated_calls_to_ConcurrentDictionary());
             lt.Run(new LightExpression.IssueTests.Issue499_InvalidProgramException_for_Sorting_and_comparison_function());
+            lt.Run(new LightExpression.IssueTests.Issue561_NonBooleanEqualityOperator());
 
             RunAllTests();
         }