'Unused Parameters' make lambda compilation fail

[StevenThuriot]

Hi,

Just discovered this project and I was playing around with it. I found a minor issue and figured I'd report it.
Basically I have a set of scripts that all run on an instance that I pass as parameter. On startup, I create an interpreter and try to simulate context by adding all the properties of said parameter.

static class Scripting {
        static readonly Interpreter _interpreter;
        static readonly Parameter _parameter = new Parameter("value", typeof(GlobalContext));

        static Scripting()
        {
            _interpreter = new Interpreter(InterpreterOptions.DefaultCaseInsensitive);
            _interpreter.Reference(typeof(GlobalContext));

            var knownTypes = new HashSet<Type>(_interpreter.ReferencedTypes.Select(x => x.Type));

            foreach (var property in typeof(GlobalContext).GetProperties(BindingFlags.Instance | BindingFlags.Public)
                                                       .Where(x => x.CanRead))
            {
                var propExpr = Expression.Property(_parameter.Expression, property);
                _interpreter.SetExpression(property.Name, propExpr);

                if (knownTypes.Add(property.PropertyType))
                    _interpreter.Reference(property.PropertyType);
            }
        }
}

Eventually, I will generate a compiled lambda.

var lambda = _interpreter.Parse(condition, typeof(bool), _parameter);
var @delegate = lambda.Compile<Func<GlobalContext, bool>>();

The Parse method will fail, because of the following line in the constructor of Lambda.cs

var lambdaExpression = Expression.Lambda(_expression, _parserArguments.UsedParameters.Select(p => p.Expression).ToArray());

UsedParameters is empty, because I haven't actually used the parameter, I've used properties of said parameter. (meaning I did use it, but the parser didn't notice)

After reading the note that it's not really needed to compile the lambda there if you're going to compile a typed one later, I changed the code there to a Lazy<Delegate> instead, which was enough to make things work out for me. However, when compiling a typed lambda, I noticed you used the following code:

public Expression<TDelegate> LambdaExpression<TDelegate>()
{
	return Expression.Lambda<TDelegate>(_expression, DeclaredParameters.Select(p => p.Expression).ToArray());
}

Here DeclaredParameters is used instead of UsedParameters.
Shouldn't this also be the case for the lambda compilation in the ctor of Lambda.cs ?

[davideicardi]

Hello Steven,

Can you elaborate more the expression: "I did use it, but the parser didn't notice"?
Can you provide an example (unit test would be great) where the parser cannot correctly detect used parameters?

thanks

[StevenThuriot]

Hi David,

Here you go 😄

class GlobalContext
{
    public int NumericProperty { get; set; }
}

[Test]
public void Test_Github_Issue_60()
{
    var parameter = new Parameter("value", typeof(GlobalContext));            
    var interpreter = new Interpreter(InterpreterOptions.DefaultCaseInsensitive);

    //Reference our parameter
    interpreter.Reference(typeof(GlobalContext));


    //Pretend GlobalContext is our current context and reference property types.
    var knownTypes = new HashSet<Type>(interpreter.ReferencedTypes.Select(x => x.Type));

    foreach (var property in typeof(GlobalContext).GetProperties(BindingFlags.Instance | BindingFlags.Public))
    {
        var propExpr = Expression.Property(parameter.Expression, property);
        interpreter.SetExpression(property.Name, propExpr);

        if (knownTypes.Add(property.PropertyType))
            interpreter.Reference(property.PropertyType);
    }

    //this line crashes because of the ctor in Lambda.cs
    //var lambdaExpression = Expression.Lambda(_expression, _parserArguments.UsedParameters.Select(p => p.Expression).ToArray());
    //UsedParameters is empty because our parameter "value" is not used directly.
    //NumericProperty does reference it, though, since it's a property of value.
    //Other than crashing, which is sad, it seems like in this specific example the expression being compiled who's responsible for the crash, is not used since we're using a typed delegate compile below.
    var script = interpreter.Parse("NumericProperty > 3", typeof(bool), parameter);

    //However, this does work as expected since we're actually using the value parameter here.
    //var script = interpreter.Parse("value.NumericProperty > 3", typeof(bool), parameter);

    //This one works because it uses _parserArguments.DeclaredParameters instead of _parserArguments.UsedParameters
    var @delegate = script.Compile<Func<GlobalContext, bool>>(); 

    var context = new GlobalContext
    {
        NumericProperty = 5
    };
            
    var result = @delegate(context);
    Assert.AreEqual(true, result);
}

[davideicardi]

Thanks, now the problem is clear.

Your scenario is interesting but for now not officially supported. It is related to issue #18, and probably your solution is an useful hint on how to solve this scenario.

Unfortunately changing UsedParameters with DeclaredParameters make the test When_lambda_is_invoked_I_can_omit_parameters_not_used fail. Honestly I don't remember why I have written this test, but I suppose there was a reason and use case.
Probably there are some possible solutions, but I must think carefully to not break existing code...

A quick solution is to write expression with an explicit context, like it.NumbericProperty or context.NumbericProperty.

[Test]
public void Test_Github_Issue_60()
{
	var parameter = new Parameter("it", typeof(GlobalContext));
	var interpreter = new Interpreter(InterpreterOptions.DefaultCaseInsensitive);

	foreach (var property in typeof(GlobalContext).GetProperties(BindingFlags.Instance | BindingFlags.Public))
	{
		var propExpr = Expression.Property(parameter.Expression, property);
		interpreter.SetExpression(property.Name, propExpr);
	}

	var script = interpreter.Parse("it.NumericProperty > 3", typeof(bool), parameter);

	var @delegate = script.Compile<Func<GlobalContext, bool>>();

	var context = new GlobalContext
	{
		NumericProperty = 5
	};

	var result = @delegate(context);
	Assert.AreEqual(true, result);
}

Can be a valid option for your scenario? Eventually this can also be done at runtime, not very clean but it should be quite easy to prepend it. on every known parameters...

[StevenThuriot]

Thanks for looking it over. I had considered your solution before, having the users type out "value.NumericProperty" instead, but I'm trying to put this into place into an existing API and trying to break as little as possible. Worst case I will go with that option! Definitely not the worst thing in the world. We are currently using Roslyn (why i'm trying to simulate context in the first place!) and it worked well, but it was a lot slower to eval our little snippets and we had a ton of assemblies loaded into memory in no time. So I'm trying to move away from that.

I also considered manually prepending "value." on all known properties when I receive the input, but that's a path I'd rather not take as I'll never feel 100% sure it will be correct every single time.

For now, I've forked the repo and changed the crashing line to

new Lazy<Delegate>(() =>{
var lambdaExpression =  Expression.Lambda(_expression, _parserArguments.UsedParameters.Select(p => p.Expression).ToArray());

return lambdaExpression.Compile();
);

The delegate isn't ever called in my case, which is good enough for now, since I'm using:

script.Compile<Func<GlobalContext, bool>>();

(In other words, I could easily comment out the whole part in my very specific case and everything would still work just fine)

However, since Compile<> uses DeclaredParameters instead of UsedParameters, wouldn't your test When_lambda_is_invoked_I_can_omit_parameters_not_used fail in case of a typed compile?

Anyway, it's definitely up to you to chose what you do with this. I just figured I'd report it since typed Compile<> and untyped (e.g. Eval) currently have different behavior. It would at least be great if a typed compile (which works) wouldn't crash because the untyped one doesn't compile (which is overhead in a typed scenario in the first place since it's compiled and never used)