fixed: dadhi#255 Nested Lambda Invoke does not work in Block

Author: maximv

src/FastExpressionCompiler/FastExpressionCompiler.cs

@@ -744,7 +744,7 @@ public bool IsTryReturnLabel(int index)
         public static ConstructorInfo ArrayClosureWithNonPassedParamsConstructorWithoutConstants = _nonPassedParamsArrayClosureCtors[1];
         public class ArrayClosure
         {
-            public readonly object[] ConstantsAndNestedLambdas; // todo: @incomplete split into two to reduce copying 
+            public readonly object[] ConstantsAndNestedLambdas; // todo: @feature split into two to reduce copying - it mostly need to set up nested lamdbas and constants externally without closure collecting phase
             public ArrayClosure(object[] constantsAndNestedLambdas) => ConstantsAndNestedLambdas = constantsAndNestedLambdas;
         }
 
@@ -2611,12 +2611,13 @@ private static bool TryEmitNotNullConstant(
                     if (constIndex == -1)
                         return false;
 
+                    // todo: @incomplete - are we still using the usage to decide for the vars?
                     var varIndex = closure.ConstantUsageThenVarIndex.Items[constIndex] - 1;
                     if (varIndex > 0)
                         EmitLoadLocalVariable(il, varIndex);
                     else
                     {
-                        il.Emit(OpCodes.Ldloc_0); // load constants array from variable
+                        il.Emit(OpCodes.Ldloc_0); // load constants array from the 0 variable // todo: @incomplete until we optimize for a single constant case - then we need a check here for number of constants
                         EmitLoadConstantInt(il, constIndex);
                         il.Emit(OpCodes.Ldelem_Ref);
                         if (exprType.IsValueType())
@@ -2657,7 +2658,7 @@ private static bool TryEmitNotNullConstant(
                 return true;
             }
 
-            // todo: can we do something about boxing?
+            // todo: @perf can we do something about boxing?
             private static bool TryEmitNumberConstant(ILGenerator il, object constantValue, Type constValueType)
             {
                 if (constValueType == typeof(int))
@@ -2768,6 +2769,7 @@ internal static bool TryEmitNumberOne(ILGenerator il, Type type)
 
             internal static void EmitLoadConstantsAndNestedLambdasIntoVars(ILGenerator il, ref ClosureInfo closure)
             {
+                // todo: @perf load the field to `var` only if the constants are more than 1
                 // Load constants array field from Closure and store it into the variable
                 il.Emit(OpCodes.Ldarg_0);
                 il.Emit(OpCodes.Ldfld, ArrayClosureArrayField);
@@ -2780,7 +2782,7 @@ internal static void EmitLoadConstantsAndNestedLambdasIntoVars(ILGenerator il, r
                 int varIndex;
                 for (var i = 0; i < constCount; i++)
                 {
-                    if (constUsage[i] > 1)
+                    if (constUsage[i] > 1) // todo: @incomplete should we proceed to do this or simplify an remove the usages for the closure info?
                     {
                         il.Emit(OpCodes.Ldloc_0);// SHOULD BE always at 0 locaton; load array field variable on the stack
                         EmitLoadConstantInt(il, i);
@@ -3241,8 +3243,8 @@ private static bool TryEmitAssign(BinaryExpression expr,
                         // if parameter isn't passed, then it is passed into some outer lambda or it is a local variable,
                         // so it should be loaded from closure or from the locals. Then the closure is null will be an invalid state.
                         // if it's a local variable, then store the right value in it
-                        var localVariableIdx = closure.GetDefinedLocalVarOrDefault(leftParamExpr); // todo: @bug ? does the var index takes into account loading constants into vars?
-                        if (localVariableIdx != -1)
+                        var localVarIndex = closure.GetDefinedLocalVarOrDefault(leftParamExpr);
+                        if (localVarIndex != -1)
                         {
                             if (!TryEmit(right, paramExprs, il, ref closure, flags))
                                 return false;
@@ -3253,7 +3255,7 @@ private static bool TryEmitAssign(BinaryExpression expr,
                             if ((parent & ParentFlags.IgnoreResult) == 0) // if we have to push the result back, duplicate the right value
                                 il.Emit(OpCodes.Dup);
 
-                            EmitStoreLocalVariable(il, localVariableIdx);
+                            EmitStoreLocalVariable(il, localVarIndex);
                             return true;
                         }
 
@@ -3638,10 +3640,6 @@ private static bool TryEmitNestedLambda(LambdaExpression lambdaExpr,
                     EmitLoadLocalVariable(il, nestedLambdaInfo.LambdaVarIndex); // load the variable for the second time
                     il.Emit(OpCodes.Ldfld, NestedLambdaWithConstantsAndNestedLambdas.ConstantsAndNestedLambdasField);
                 }
-                else
-                {
-                    // we are already loaded the nested lambda, because it is not wrapped in NestedLambdaWithConstantsAndNestedLambdas
-                }
 
                 // - create `NonPassedParameters` array
                 EmitLoadConstantInt(il, nestedNonPassedParams.Length); // load the length of array
@@ -3680,10 +3678,12 @@ private static bool TryEmitNestedLambda(LambdaExpression lambdaExpr,
                         if (outerNonPassedParams.Length == 0)
                             return false; // impossible, better to throw?
 
-                        var variableIdx = closure.GetDefinedLocalVarOrDefault(nestedParam); // rename to paramVarIndex
-                        if (variableIdx != -1) // it's a local variable
+                        var outerLocalVarIndex = closure.GetDefinedLocalVarOrDefault(nestedParam);
+                        if (outerLocalVarIndex != -1) // it's a local variable
                         {
-                            EmitLoadLocalVariable(il, variableIdx);
+                            EmitLoadLocalVariable(il, outerLocalVarIndex);
+                            if (nestedParam.Type.IsValueType()) // don't forget to box the value type when we store it into object array, (fixes #255)
+                                il.Emit(OpCodes.Box, nestedParam.Type);
                         }
                         else // it's a parameter from the outer closure
                         {

test/FastExpressionCompiler.TestsRunner/Program.cs

@@ -10,8 +10,6 @@ public class Program
         public static void Main()
         {
             RunAllTests();
-
-            //new FastExpressionCompiler.LightExpression.UnitTests.BlockTests().Run();
         }
 
         public static void RunAllTests()
@@ -85,7 +83,6 @@ void Run(Func<int> run, string name = null)
                 Run(new FastExpressionCompiler.LightExpression.UnitTests.NestedLambdasSharedToExpressionCodeStringTest().Run);
             });
 
-
             var issueTests = Task.Run(() => 
             {
                 Run(new Issue14_String_constant_comparisons_fail().Run);

test/FastExpressionCompiler.UnitTests/BlockTests.cs

@@ -27,16 +27,16 @@ public int Run()
             Block_local_variable_assignment_with_member_init();
             Block_local_variable_assignment_with_member_init_array_closure();
             Block_local_variable_assignment_with_lambda_invoke();
-
-            // todo: @fix #255 Nested Lambda Invoke does not work in Block
-            // Block_local_variable_assignment_returning_the_nested_lambda_which_reassigns_the_variable();
-
-            // Block_local_variable_assignment_with_lambda_invoke_plus_external_assignment();
-            // Block_local_variable_assignment_with_lambda_invoke_array_closure();
-            // Block_local_variable_assignment_with_lambda_invoke_with_param_override();
-            // Block_local_variable_assignment_with_lambda_invoke_with_param_override_array_closure();
-
-            return 13;
+            Block_returning_the_nested_lambda_assigning_the_outer_parameter();
+            Block_calling_non_void_method_returning_the_nested_lambda_assigning_the_outer_parameter();
+            Block_assigning_local_variable_then_returning_the_nested_lambda_which_reassigns_the_variable();
+            Block_assigning_local_ValueType_variable_then_returning_the_nested_lambda_which_reassigns_the_variable();
+            Block_local_variable_assignment_with_lambda_invoke_plus_external_assignment();
+            Block_local_variable_assignment_with_lambda_invoke_array_closure();
+            Block_local_variable_assignment_with_lambda_invoke_with_param_override();
+            Block_local_variable_assignment_with_lambda_invoke_with_param_override_array_closure();
+
+            return 17;
         }
 
         [Test]
@@ -98,43 +98,72 @@ public void Block_local_variable_assignment_with_lambda_invoke()
             Assert.AreEqual(6, f());
         }
 
-        // todo: @fix Invoke CLR crash
+        public static int Inc(int i) => i + 1; 
+
         [Test]
-        public void Block_local_variable_assignment_returning_the_nested_lambda_which_reassigns_the_variable()
+        public void Block_calling_non_void_method_returning_the_nested_lambda_assigning_the_outer_parameter()
         {
-            var variable = Variable(typeof(int));
-
-            var block = Block(new[] { variable },
-                Assign(variable, Constant(35)),
-                Lambda(Assign(variable, Constant(42))));
+            var p = Parameter(typeof(int), "p");
 
-            var lambda = Lambda<Func<Func<int>>>(block);
+            var lambda = Lambda<Func<int, Func<int>>>(
+                Block(
+                    Call(null, GetType().GetTypeInfo().GetDeclaredMethod(nameof(Inc)), p),
+                    Lambda(Assign(p, Constant(42)))
+                ),
+                p);
 
 #if LIGHT_EXPRESSION
             lambda.PrintCSharpString();
 #endif
-
             var s = lambda.CompileSys();
             s.PrintIL("system il");
 
-            var f = lambda.CompileFast<Func<Func<int>>>(true);
+            var f = lambda.CompileFast<Func<int, Func<int>>>(true);
             f.PrintIL("fec il");
 
             Assert.NotNull(f);
-            Assert.AreEqual(6, f()());
+            var ff = f(17);
+            Assert.IsInstanceOf<Func<int>>(ff);
+
+            Assert.AreEqual(42, ff());
         }
 
-        // todo: @fix Invoke CLR crash
         [Test]
-        public void Block_local_variable_assignment_with_lambda_invoke_plus_external_assignment()
+        public void Block_returning_the_nested_lambda_assigning_the_outer_parameter()
         {
-            var variable = Variable(typeof(int));
+            var p = Parameter(typeof(int), "p");
 
-            var block = Block(new[] { variable },
-                Assign(variable, Constant(5)),
-                Invoke(Lambda(Assign(variable, Constant(6)))));
+            var lambda = Lambda<Func<int, Func<int>>>(
+                    Lambda(Assign(p, Constant(42))),
+                p);
 
-            var lambda = Lambda<Func<int>>(block);
+#if LIGHT_EXPRESSION
+            lambda.PrintCSharpString();
+#endif
+            var s = lambda.CompileSys();
+            s.PrintIL("system il");
+
+            var f = lambda.CompileFast<Func<int, Func<int>>>(true);
+            f.PrintIL("fec il");
+
+            Assert.NotNull(f);
+            var ff = f(17);
+            Assert.IsInstanceOf<Func<int>>(ff);
+
+            Assert.AreEqual(42, ff());
+        }
+
+        [Test]
+        public void Block_assigning_local_variable_then_returning_the_nested_lambda_which_reassigns_the_variable()
+        {
+            var variable = Variable(typeof(string));
+
+            var lambda = Lambda<Func<Func<string>>>(
+                Block(new[] { variable },
+                    Assign(variable, Constant("35")),
+                    Lambda(Assign(variable, Constant("42")))
+                )
+            );
 
 #if LIGHT_EXPRESSION
             lambda.PrintCSharpString();
@@ -143,11 +172,48 @@ public void Block_local_variable_assignment_with_lambda_invoke_plus_external_ass
             var s = lambda.CompileSys();
             s.PrintIL("system il");
 
-            var f = lambda.CompileFast<Func<int>>(true);
+            var f = lambda.CompileFast(true);
             f.PrintIL("fec il");
 
-            // Assert.NotNull(fastCompiled);
-            // Assert.AreEqual(6, fastCompiled());
+            Assert.NotNull(f);
+            var ff = f();
+            Assert.IsInstanceOf<Func<string>>(ff);
+            Assert.AreEqual("42", ff());
+        }
+
+        [Test]
+        public void Block_assigning_local_ValueType_variable_then_returning_the_nested_lambda_which_reassigns_the_variable()
+        {
+            var variable = Variable(typeof(int));
+
+            var lambda = Lambda<Func<Func<int>>>(
+                Block(new[] { variable },
+                    Assign(variable, Constant(35)),
+                    Lambda(Assign(variable, Constant(42)))
+                )
+            );
+
+            var f = lambda.CompileFast(true);
+            Assert.NotNull(f);
+
+            var ff = f();
+            Assert.IsInstanceOf<Func<int>>(ff);
+            Assert.AreEqual(42, ff());
+        }
+
+        [Test]
+        public void Block_local_variable_assignment_with_lambda_invoke_plus_external_assignment()
+        {
+            var variable = Variable(typeof(int));
+
+            var block = Block(new[] { variable },
+                Assign(variable, Constant(5)),
+                Invoke(Lambda(Assign(variable, Constant(6)))));
+
+            var lambda = Lambda<Func<int>>(block);
+
+            var f = lambda.CompileFast<Func<int>>(true);
+            Assert.AreEqual(6, f());
         }
 
         [Test]