From 5a3980c307988f07d237a307e346dc5a9e560946 Mon Sep 17 00:00:00 2001 From: jogibear9988 Date: Tue, 10 Nov 2026 23:05:06 +0100 Subject: [PATCH] fix: preserve operand storage for ref conversion methods Explicit conversion methods taking a ref parameter can receive a temporary instead of the original storage. Normalize the formal parameter type and distinguish direct ref passing from lifted conversions, which must use an unwrapped value copy. Validation: Release solution build; full Standard and LightExpression .NET 8, .NET 10 and Framework runners pass. Measurements: master 094d0678 versus this change, compilation and invocation, both APIs, .NET 8/10, two opposite-order process rounds with seven samples each. No measured allocation increase above 0.1 B/op in the selected working controls. Timings and emitted IL are recorded in the PR description; these controls do not establish universal performance neutrality. --- .../FastExpressionCompiler.cs | 37 +++--- .../Program.cs | 1 + .../Program.cs | 1 + .../RefConversionStorageTests.cs | 106 ++++++++++++++++++ 4 files changed, 124 insertions(+), 21 deletions(-) create mode 100644 test/FastExpressionCompiler.UnitTests/RefConversionStorageTests.cs diff --git a/src/FastExpressionCompiler/FastExpressionCompiler.cs b/src/FastExpressionCompiler/FastExpressionCompiler.cs index e1e0b979..899dd95b 100644 --- a/src/FastExpressionCompiler/FastExpressionCompiler.cs +++ b/src/FastExpressionCompiler/FastExpressionCompiler.cs @@ -3947,7 +3947,7 @@ private static bool TryEmitConvert(UnaryExpression expr, ILGenerator il, ref Com // see #73, #451 for examples Type methodReturnType = null; Type methodParamType = null; - var liftedByRefMethod = false; + var methodParameterIsByRef = false; if (convertMethod != null) { methodReturnType = convertMethod.ReturnType; @@ -3955,26 +3955,19 @@ private static bool TryEmitConvert(UnaryExpression expr, ILGenerator il, ref Com var mParams = convertMethod.GetParameters(); Debug.Assert(mParams.Length == 1, $"Expected for the conversion operator to have a single param, but found {mParams.Length}"); methodParamType = mParams[0].ParameterType; - if (methodParamType.IsByRef && sourceType.IsNullable() && methodParamType.GetElementType() != sourceType) - { - liftedByRefMethod = true; + methodParameterIsByRef = methodParamType.IsByRef; + if (methodParameterIsByRef) methodParamType = methodParamType.GetElementType(); - } - - // todo: @wip check if we need to add the ParentFlags.Call here? - var isByRefParam = methodParamType.IsByRef; - var operandFlags = liftedByRefMethod - ? (parent & ~CallArgumentFlagsToClear) | ParentFlags.AssignmentRightValue - : isByRefParam - ? parent & ~ParentFlags.IgnoreResult & ~ParentFlags.InstanceAccess - : (parent & ~ValueOperandFlagsToClear) | ParentFlags.AssignmentRightValue; - if (!TryEmit(opExpr, il, ref context, operandFlags, - isByRefParam && !(opExpr is ParameterExpression { IsByRef: true } && sourceType.IsNullable()) ? 0 : -1)) + var passSourceByRef = methodParameterIsByRef && methodParamType == sourceType; + var operandFlags = passSourceByRef + ? (parent & ~(ParentFlags.IgnoreResult | ParentFlags.InstanceAccess | ParentFlags.MemberAccess | + ParentFlags.AssignmentRightValue | ParentFlags.Arithmetic | ParentFlags.LambdaCall | ParentFlags.ReturnByRef)) | ParentFlags.Call + : (parent & ~(ParentFlags.IgnoreResult | ParentFlags.InstanceAccess | ParentFlags.MemberAccess | + ParentFlags.AssignmentLeftValue | ParentFlags.AssignmentByRef | ParentFlags.DupIt | + ParentFlags.LambdaCall | ParentFlags.ReturnByRef)) | ParentFlags.AssignmentRightValue; + if (!TryEmit(opExpr, il, ref context, operandFlags, passSourceByRef ? 0 : -1)) return false; - // The by-ref param of the method needs the address of the operand, the nullable by-ref parameter is already the address - if (isByRefParam && !context.LastEmitIsAddress && !(opExpr is ParameterExpression { IsByRef: true })) - EmitStoreAndLoadLocalVariableAddress(il, sourceType); } else { @@ -4053,9 +4046,9 @@ private static bool TryEmitConvert(UnaryExpression expr, ILGenerator il, ref Com il.DmarkLabel(labelSourceHasValue); EmitLoadLocalVariableAddress(il, sourceVarIndex); il.Demit(OpCodes.Ldfld, sourceType.GetNullableValueUnsafeAkaGetValueOrDefaultMethod()); - if (liftedByRefMethod) - EmitStoreAndLoadLocalVariableAddress(il, methodParamType); + if (methodParameterIsByRef) + EmitStoreAndLoadLocalVariableAddress(il, methodParamType); EmitMethodCallOrVirtualCall(il, convertMethod); // Wrap the conversion result into the target type only if the conversion method return the underlying target type, @@ -4064,6 +4057,7 @@ private static bool TryEmitConvert(UnaryExpression expr, ILGenerator il, ref Com il.Demit(OpCodes.Newobj, targetType.GetNullableConstructor()); il.DmarkLabel(labelConversionDone); + context.LastEmitIsAddress = false; return il.EmitPopIfIgnoreResult(parent); } @@ -4090,9 +4084,10 @@ private static bool TryEmitConvert(UnaryExpression expr, ILGenerator il, ref Com $"Expected the conversion operator parameter type `{methodParamType}` to be either Nullable<{sourceType}> or a base type of `{sourceType}`"); } - if (liftedByRefMethod) + if (methodParameterIsByRef && methodParamType != sourceType) EmitStoreAndLoadLocalVariableAddress(il, methodParamType); EmitMethodCallOrVirtualCall(il, convertMethod); + context.LastEmitIsAddress = false; if (methodReturnType == underlyingNullableTargetType) il.Demit(OpCodes.Newobj, targetType.GetNullableConstructor()); diff --git a/test/FastExpressionCompiler.TestsRunner.Net472/Program.cs b/test/FastExpressionCompiler.TestsRunner.Net472/Program.cs index 0d472ad6..a7312c7e 100644 --- a/test/FastExpressionCompiler.TestsRunner.Net472/Program.cs +++ b/test/FastExpressionCompiler.TestsRunner.Net472/Program.cs @@ -105,6 +105,7 @@ static void RunLightExpressionTests(object state) fecTests.Run(new ArrayBoundsExceptionRegressionTests()); fecTests.Run(new TryResultValueRegressionTests()); fecTests.Run(new NonVectorArrayRegressionTests()); + fecTests.Run(new RefConversionStorageTests()); fecTests.Run(new UnboxStorageRegressionTests()); fecTests.Run(new ArrayInitializerExceptionRegressionTests()); fecTests.Run(new ReadonlyRefFieldRegressionTests()); diff --git a/test/FastExpressionCompiler.TestsRunner/Program.cs b/test/FastExpressionCompiler.TestsRunner/Program.cs index f0b4a4c2..0057179f 100644 --- a/test/FastExpressionCompiler.TestsRunner/Program.cs +++ b/test/FastExpressionCompiler.TestsRunner/Program.cs @@ -131,6 +131,7 @@ public static void Main(string[] args) st.Run(new ArrayBoundsExceptionRegressionTests()); st.Run(new TryResultValueRegressionTests()); st.Run(new NonVectorArrayRegressionTests()); + st.Run(new RefConversionStorageTests()); st.Run(new UnboxStorageRegressionTests()); st.Run(new ArrayInitializerExceptionRegressionTests()); st.Run(new ReadonlyRefFieldRegressionTests()); diff --git a/test/FastExpressionCompiler.UnitTests/RefConversionStorageTests.cs b/test/FastExpressionCompiler.UnitTests/RefConversionStorageTests.cs new file mode 100644 index 00000000..c3669295 --- /dev/null +++ b/test/FastExpressionCompiler.UnitTests/RefConversionStorageTests.cs @@ -0,0 +1,106 @@ +#if !LIGHT_EXPRESSION +using System; +using System.Linq.Expressions; +using System.Reflection; +namespace FastExpressionCompiler.UnitTests; +public struct RefConversionStorageTests : ITestX +{ + public void Run(TestRun t) + { + Ref_method_mutates_ref_operand(t); + Lifted_ref_method_uses_copy_and_throws_on_null(t); + Ref_method_handles_other_operand_locations(t); + Nonref_method_still_dereferences_ref_operand(t); + Lifted_ref_method_returns_nullable_struct(t); + Ignored_ref_conversion_keeps_mutation(t); + } + public void Ref_method_mutates_ref_operand(TestContext t) => Check(t, false, 0, nameof(ConvertRef)); + public void Lifted_ref_method_uses_copy_and_throws_on_null(TestContext t) => Check(t, true, 0, nameof(ConvertRef)); + public void Ref_method_handles_other_operand_locations(TestContext t) + { for (var shape = 1; shape != 6; ++shape) { Check(t, false, shape, nameof(ConvertRef)); Check(t, true, shape, nameof(ConvertRef)); } } + public void Nonref_method_still_dereferences_ref_operand(TestContext t) + { Check(t, false, 0, nameof(ConvertValue)); Check(t, true, 0, nameof(ConvertValue)); } + public void Lifted_ref_method_returns_nullable_struct(TestContext t) => Check(t, true, 0, nameof(ConvertStructRef)); + public void Ignored_ref_conversion_keeps_mutation(TestContext t) + { + var p = Expression.Parameter(typeof(int).MakeByRefType()); + var expression = Expression.Lambda>(Expression.Block( + Expression.Convert(p, typeof(Converted), typeof(RefConversionStorageTests).GetMethod(nameof(ConvertRef))), Expression.Constant(7)), p); + var fast = expression.CompileFast(true); + if (!t.IsNotNull(fast)) return; + var value = 5; + t.AreEqual(7, fast(ref value)); + t.AreEqual(6, value); + } + private static void Check(TestContext t, bool nullable, int shape, string methodName) + { + var type = nullable ? typeof(int?) : typeof(int); + var p = Expression.Parameter(shape == 0 ? type.MakeByRefType() : type); + var local = Expression.Variable(type); + var cell = Expression.Variable(typeof(Cell<>).MakeGenericType(type)); + Expression location = shape == 2 ? local : shape == 3 ? Expression.Field(cell, nameof(Cell.Value)) + : shape == 4 ? Expression.Constant(nullable ? (object)(int?)5 : 5, type) + : shape == 5 ? Expression.Call(typeof(RefConversionStorageTests).GetMethod(nullable ? nameof(ReadNullableOperand) : nameof(ReadOperand))) : p; + var method = typeof(RefConversionStorageTests).GetMethod(methodName); + var targetType = methodName == nameof(ConvertStructRef) ? typeof(ConvertedStruct?) : typeof(Converted); + var result = Expression.NewArrayInit(typeof(object), Expression.Convert(Expression.Convert(location, targetType, method), typeof(object)), Expression.Convert(shape >= 4 ? p : location, typeof(object))); + var body = shape == 2 ? Expression.Block(new[] { local }, Expression.Assign(local, p), result) + : shape == 3 ? Expression.Block(new[] { cell }, Expression.Assign(cell, Expression.New(cell.Type)), Expression.Assign(location, p), result) : (Expression)result; + var expression = shape == 0 ? Expression.Lambda(typeof(Read<>).MakeGenericType(type), body, p) : Expression.Lambda(body, p); + var fast = expression.CompileFast(true); + if (!t.IsNotNull(fast)) return; + foreach (var value in nullable ? new object[] { null, 5 } : new object[] { 5 }) + { + var expectedArgs = new[] { value }; + var actualArgs = new[] { value }; + OperandCalls = 0; +#if NETFRAMEWORK + // The Framework expression compiler rejects conversion methods with ref parameters. + // Execute the same conversion directly to retain the value, mutation and operand-effect oracle. + var expected = EvaluateExpectedConversion(value, nullable, shape, methodName, expectedArgs); +#else + var expected = Invoke(expression.Compile(), expectedArgs); +#endif + var expectedCalls = OperandCalls; + OperandCalls = 0; + var actual = Invoke(fast, actualArgs); + if (expected is object[] e && actual is object[] a) + { + if (e[0] is Converted ec && a[0] is Converted ac) t.AreEqual(ec.Value, ac.Value); + else t.AreEqual(e[0], a[0]); + t.AreEqual(e[1], a[1]); + } + else t.AreEqual(expected, actual); + t.AreEqual(expectedArgs[0], actualArgs[0]); + t.AreEqual(expectedCalls, OperandCalls); + } + } +#if NETFRAMEWORK + private static object EvaluateExpectedConversion(object value, bool nullable, int shape, string methodName, object[] arguments) + { + var operand = shape == 4 ? 5 : shape == 5 ? nullable ? (object)ReadNullableOperand() : ReadOperand() : value; + if (operand == null) + return methodName == nameof(ConvertStructRef) ? (object)new object[] { null, value } : typeof(InvalidOperationException); + var temporary = (int)operand; + object converted = methodName == nameof(ConvertValue) ? (object)ConvertValue(temporary) + : methodName == nameof(ConvertStructRef) ? ConvertStructRef(ref temporary) : (object)ConvertRef(ref temporary); + var writesBack = !nullable && methodName != nameof(ConvertValue) && shape < 4; + if (writesBack && shape == 0) arguments[0] = temporary; + return new object[] { converted, writesBack ? temporary : value }; + } +#endif + private static object Invoke(Delegate function, object[] arguments) + { try { return function.DynamicInvoke(arguments); } catch (TargetInvocationException e) { return e.InnerException.GetType(); } } + public delegate object[] Read(ref T value); + public delegate int Mutation(ref T value); + public static int OperandCalls; + public static int ReadOperand() { ++OperandCalls; return 5; } + public static int? ReadNullableOperand() { ++OperandCalls; return 5; } + public static Converted ConvertRef(ref int value) => new Converted { Value = value++ }; + public static Converted ConvertValue(int value) => new Converted { Value = value }; + public static ConvertedStruct ConvertStructRef(ref int value) => new ConvertedStruct { Value = value++ }; + public class Converted { public int Value; } + public struct ConvertedStruct { public int Value; } + public class Cell { public T Value; } +} +#endif