Throw error when registering multiple times the same delegate

[knoxi]

After upgrading from V2.4.1 to a newer version, I'm receiving an error DynamicExpresso.Exceptions.ParseException: 'Argument list incompatible with delegate expression (at index 0). I have not seen any breaking changes in the release notes.

The code I'm trying to parse:

//var body = "GetClient().GetPromoCodesAsync(skip, top, orderby, filter, page, pagesize, CancellationToken.None)";
//var parameterNames = new[] { "skip", "top", "orderby", "filter", "page", "pagesize" };
//var customFunc = new CustomFunction() { Name = "GetClient", RefType = <type>, CustomFunction = <Delegate> }

var interpreter = new Interpreter();

interpreter.Reference(customFunc.RefType);
interpreter.SetFunction(customFunc.Name, customFunc.CustomFunction);

var compiledFunc = interpreter.ParseAsDelegate<TDelegate>(body, parameterNames);

[davideicardi]

We have changed some internal code on how functions are interpreted, inside #138 or other related PRs.
So I suspect it is some kind of regression.
Are you able to write an unit test to reproduce the bug?
See for example https://github.com/davideicardi/DynamicExpresso/blob/master/test/DynamicExpresso.UnitTest/FunctionTest.cs or https://github.com/davideicardi/DynamicExpresso/blob/master/test/DynamicExpresso.UnitTest/MemberInvocationTest.cs

[knoxi]

Within our framework, we use a lot of generic implementation, so I tried to simplify a repro of our implementation.
Finally, I was able to do a repro which fails with code which worked with V2.4.1 but no longer with newer versions.

I have added the repro project. If you run the tests, you can see two tests, one is failing failing, the failing one is exactly our current use cases. The tests becomes green switching back to V2.4.1.

While reproducing the issue, I noticed that our code sets twice the same types and function.
It looks like this causes the issue, because when I'm avoiding to add the same function a second time, it works also on newer versions (second UT).

DynamicExpressoRepro.zip

[metoule]

@knoxi thanks for the investigation. I suspect the double registration is the issue: in version 2.4.1, calling SetFunction with the same name overrode previous function registration (ie. the last call was the one that was used).
Starting with 2.5.0, we allow the registration of multiple overloads of the same function name. It's possible there's an ambiguity when the exact same function is registered twice.

[knoxi]

yes, with that explanation it makes now absolutely sense why we are running into that issue.
This means, we need to keep care what we register, we can no longer expect that it's overridden.

Thanks!

[metoule]

@knoxi I wouldn't close the issue just yet, I still want to investigate if that's actually the issue :) Also, we should detect if the same function is registered twice to prevent this from happening.

[metoule]

It was possible to register the same delegate multiple times; that has now been fixed, thanks for the report!

In your particular case however, you're registering two delegates that are in fact the same, but are built twice via Expression.Lambda.Compile(). It's not the same delegate object, so we treat as two different overloads, and since they have the same signature, there's an ambiguous match.

We should throw an AmbiguousDelegateInvocation error message instead of a ArgsIncompatibleWithDelegate, but that's not possible at the moment due to a peculiarity in the way overloads are selected.

@davideicardi in FindBestMethod, if there are more than one applicable method, the candidates are filtered by the MethodHasPriority method. If no method has priority (like in this particular case), then FindBestMethod returns an empty array, and therefore the caller can't see that there's in fact an ambiguity:

private static MethodData[] FindBestMethod(IEnumerable<MethodData> methods, Expression[] args)
{
	var applicable = methods.
		Where(m => CheckIfMethodIsApplicableAndPrepareIt(m, args)).
		ToArray();
	if (applicable.Length > 1)
	{
                // can return an empty array; the ambiguity is lost to the caller
		return applicable.
			Where(m => applicable.All(n => m == n || MethodHasPriority(args, m, n))).
			ToArray();
	}

	return applicable;
}

[davideicardi]

Thank you @metoule for the investigation and fix! In my opinion it is not a bit issue for now, it can be solved by the caller.
I will open a dedicated issue for that case.

[davideicardi]

Closed by #158