dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
14.9k stars 4.63k forks source link

InvalidProgramException with System.Linq.Expressions and user-defined conditional operators #103197

Open MarkMpn opened 2 months ago

MarkMpn commented 2 months ago

Description

Using System.Linq.Expressions to build expression trees dynamically, the repro code worked as expected in .NET Framework but fails in .NET (tested .NET 6 & .NET 8)

So far I've managed to narrow this down to when I'm using a type that implements custom conditional operators (true, false, & and |) and using a TryCatch expression on the right hand side of the conditional logic operator (AndAlso or OrElse). If I reorder the arguments so the TryCatch expression is on the left hand side of the operator it works as expected.

Reproduction Steps

using System.Data.SqlTypes;
using System.Linq.Expressions;

var expr = Expression.OrElse(
    Expression.Constant(SqlBoolean.True),
    Expression.TryCatch(
        Expression.Constant(SqlBoolean.True),
        Expression.Catch(
            typeof(SqlTypeException),
            Expression.Constant(SqlBoolean.False)
        )
    )
);
var lambda = Expression.Lambda<Func<SqlBoolean>>(expr);
var func = lambda.Compile();
Console.WriteLine(func());

Expected behavior

The expression should compile and True should be printed to the console.

Actual behavior

A System.InvalidProgramException is thrown by lambda.Compile()

Regression?

Yes - this same code runs as expected in .NET Framework 4.6.2 and 4.8

Known Workarounds

No response

Configuration

.NET 6 and .NET 8 Windows 11 22631.3593 x64 Doesn't appear to be specific to this.

Other information

No response

dotnet-policy-service[bot] commented 2 months ago

Tagging subscribers to this area: @cston See info in area-owners.md if you want to be subscribed.

EgorBo commented 2 months ago

I presume in C# it would look like condition || try { .... Checked JIT reports your snippet as an invalid program due to:

Evaluation stack must be empty on entry into a try block

My understanding that it is an expected behavior according to ECMA-335 I.12.4.2.8.1 Fall Through:

Entry to protected blocks can be accomplished by fall-through, at which time the evaluation
stack shall be empty.

so I am not sure why and how it used to work in .NET Framework.

so the TryCatch expression is on the left hand side of the operator it works as expected.

if it's on the left side then the evaluation stack is empty

cc @jkotas

jkotas commented 2 months ago

I am not sure why and how it used to work in .NET Framework.

The IL produced by the expression compiler on .NET Framework has an extra spill to make the stack empty. It looks like a regression in System.Linq.Expressions to me.

jkotas commented 2 months ago

Note that we merged a ton of optimizations in S.L.Expressions in the early .NET Core days: https://github.com/dotnet/corefx/pulls?q=label%3Aarea-System.Linq.Expressions+is%3Aclosed+optimize . It is likely one of those of changes.

imb591 commented 2 months ago

It's the changes in LambdaCompiler.EmitMethodAndAlso and LambdaCompiler.EmitMethodOrElse in this commit: https://github.com/dotnet/runtime/commit/2ecfbbdc3ca2ff7d8c2cd1ad696947e698f42eee

The previous version took the left operand from the stack into a local before emitting the right operand. And StackSpiller never rewrites AndAlso/OrElse nodes when processing in StackSpiller.RewriteLogicalBinaryExpression, seemingly relying on LambdaCompiler to emit both operands on an empty stack.

jkotas commented 2 months ago

It's the changes in LambdaCompiler.EmitMethodAndAlso and LambdaCompiler.EmitMethodOrElse in this commit: https://github.com/dotnet/runtime/commit/2ecfbbdc3ca2ff7d8c2cd1ad696947e698f42eee

cc @VSadov