Fix registration of same method signature via SetFunction (#231)

* Properly return all the possible overloads if no overload is an exact match.
Fixes #159

* Added a way to remove identifiers from the current interpreter.

* SetFunction now only keeps the latest delegate registered with a given signature (ie. it's no longer possible to register two delegates with the exact same signature).

* Fix broken test.

src/DynamicExpresso.Core/Identifier.cs

@@ -1,6 +1,7 @@
 using System;
 using System.Collections.Generic;
 using System.Linq.Expressions;
+using System.Reflection;
 
 namespace DynamicExpresso
 {
@@ -56,9 +57,35 @@ internal MethodGroupExpression(Delegate overload)
 
 		internal void AddOverload(Delegate overload)
 		{
-			// don't register the same overload twice
-			if (_overloads.IndexOf(overload) == -1)
-				_overloads.Add(overload);
+			// remove any existing delegate with the exact same signature
+			RemoveDelegateSignature(overload);
+			_overloads.Add(overload);
+		}
+
+		private void RemoveDelegateSignature(Delegate overload)
+		{
+			_overloads.RemoveAll(del => HasSameSignature(overload.Method, del.Method));
+		}
+
+		private static bool HasSameSignature(MethodInfo method, MethodInfo other)
+		{
+			if (method.ReturnType != other.ReturnType)
+				return false;
+
+			var param = method.GetParameters();
+			var oParam = other.GetParameters();
+			if (param.Length != oParam.Length)
+				return false;
+
+			for (var i = 0; i < param.Length; i++)
+			{
+				var p = param[i];
+				var q = oParam[i];
+				if (p.ParameterType != q.ParameterType || p.HasDefaultValue != q.HasDefaultValue)
+					return false;
+			}
+
+			return true;
 		}
 	}
 }

src/DynamicExpresso.Core/Interpreter.cs

@@ -166,6 +166,8 @@ public Interpreter EnableReflection()
 		#region Register identifiers
 		/// <summary>
 		/// Allow the specified function delegate to be called from a parsed expression.
+		/// Overloads can be added (ie. multiple delegates can be registered with the same name).
+		/// A delegate will replace any delegate with the exact same signature that is already registered.
 		/// </summary>
 		/// <param name="name"></param>
 		/// <param name="value"></param>
@@ -273,6 +275,50 @@ public Interpreter SetIdentifier(Identifier identifier)
 
 			return this;
 		}
+
+		/// <summary>
+		/// Remove <paramref name="name"/> from the list of known identifiers.
+		/// </summary>
+		/// <param name="name"></param>>
+		/// <returns></returns>
+		public Interpreter UnsetFunction(string name)
+		{
+			return UnsetIdentifier(name);
+		}
+
+		/// <summary>
+		/// Remove <paramref name="name"/> from the list of known identifiers.
+		/// </summary>
+		/// <param name="name"></param>>
+		/// <returns></returns>
+		public Interpreter UnsetVariable(string name)
+		{
+			return UnsetIdentifier(name);
+		}
+
+		/// <summary>
+		/// Remove <paramref name="name"/> from the list of known identifiers.
+		/// </summary>
+		/// <param name="name"></param>>
+		/// <returns></returns>
+		public Interpreter UnsetExpression(string name)
+		{
+			return UnsetIdentifier(name);
+		}
+
+		/// <summary>
+		/// Remove <paramref name="name"/> from the list of known identifiers.
+		/// </summary>
+		/// <param name="name"></param>
+		/// <returns></returns>
+		public Interpreter UnsetIdentifier(string name)
+		{
+			if (string.IsNullOrWhiteSpace(name))
+				throw new ArgumentNullException(nameof(name));
+
+			_settings.Identifiers.Remove(name);
+			return this;
+		}
 		#endregion
 
 		#region Register referenced types

src/DynamicExpresso.Core/Parsing/Parser.cs

@@ -1220,12 +1220,12 @@ private Expression ParseMethodGroupInvocation(MethodGroupExpression methodGroup,
 				})
 				.ToList();
 
-			var applicableMethods = FindBestMethod(candidates.Select(_ => _.InvokeMethod), args);
+			var applicableMethods = FindBestMethod(candidates.Select(_ => _.Method), args);
 
 			// no method found: retry with the delegate's method
 			// (the parameters might be different, e.g. params array, default value, etc)
 			if (applicableMethods.Length == 0)
-				applicableMethods = FindBestMethod(candidates.Select(_ => _.Method), args);
+				applicableMethods = FindBestMethod(candidates.Select(_ => _.InvokeMethod), args);
 
 			if (applicableMethods.Length == 0)
 				throw CreateParseException(errorPos, ErrorMessages.ArgsIncompatibleWithDelegate);
@@ -1952,9 +1952,14 @@ private static MethodData[] FindBestMethod(IEnumerable<MethodData> methods, Expr
 				ToArray();
 			if (applicable.Length > 1)
 			{
-				return applicable.
-					Where(m => applicable.All(n => m == n || MethodHasPriority(args, m, n))).
-					ToArray();
+				var bestCandidates = applicable
+					.Where(m => applicable.All(n => m == n || MethodHasPriority(args, m, n)))
+					.ToArray();
+
+				// bestCandidates.Length == 0 means that no applicable method has priority
+				// we don't return bestCandidates to prevent callers from thinking no method was found
+				if (bestCandidates.Length > 0)
+					return bestCandidates;
 			}
 
 			return applicable;

test/DynamicExpresso.UnitTest/GithubIssues.cs

@@ -173,6 +173,38 @@ public void GitHub_Issue_148()
 			Assert.AreEqual(2, target.Eval("SubArray(arr1, 1, 1).First()"));
 		}
 
+		[Test]
+		public void GitHub_Issue_159_ambiguous_call()
+		{
+			Func<double?, int> f1 = d => 1;
+			Func<string, int> f2 = o => 2;
+
+			var interpreter = new Interpreter();
+			interpreter.SetFunction("f", f1);
+			interpreter.SetFunction("f", f2);
+
+			// we should properly throw an ambiguous invocation exception (multiple matching overloads found)
+			// and not an Argument list incompatible with delegate expression (no matching overload found)
+			var exc = Assert.Throws<ParseException>(() => interpreter.Eval("f(null)"));
+			StringAssert.StartsWith("Ambiguous invocation of delegate (multiple overloads found)", exc.Message);
+		}
+
+
+		[Test]
+		public void GitHub_Issue_159_unset_identifier()
+		{
+			Func<int> f1 = () => 1;
+
+			var interpreter = new Interpreter();
+			interpreter.SetFunction("f", f1);
+
+			Assert.AreEqual(1, interpreter.Eval("f()"));
+
+			// calls to f should lead to an unknown identifier exception
+			interpreter.UnsetFunction("f");
+			Assert.Throws<UnknownIdentifierException>(() => interpreter.Eval("f()"));
+		}
+
 
 #if NETCOREAPP2_1_OR_GREATER
 
@@ -186,7 +218,6 @@ static bool GetGFunction2(string arg = null)
 			}
 
 			GFunction gFunc2 = GetGFunction2;
-			Assert.False(gFunc2.Method.GetParameters()[0].HasDefaultValue); // should be true!
 
 			var flags = BindingFlags.Public | BindingFlags.DeclaredOnly | BindingFlags.Instance;
 			var invokeMethod2 = (MethodInfo)gFunc2.GetType().FindMembers(MemberTypes.Method, flags, Type.FilterName, "Invoke")[0];
@@ -204,7 +235,7 @@ static bool GetGFunction2(string arg = null)
 		public void GitHub_Issue_144_3()
 		{
 			// GetGFunction2 is defined inside the test function
-			static bool GetGFunction2(string arg = null)
+			static bool GetGFunction2(string arg)
 			{
 				return arg == null;
 			}
@@ -220,7 +251,7 @@ static bool GetGFunction2(string arg = null)
 			// ambiguous call
 			Assert.Throws<ParseException>(() => interpreter.Eval("GFunction(arg)"));
 
-			// there should be an ambiguous call exception, but GFunction1 is used
+			// GFunction1 is used
 			// because gFunc1.Method.GetParameters()[0].HasDefaultValue == true 
 			// and     gFunc2.Method.GetParameters()[0].HasDefaultValue == false
 			Assert.False((bool)interpreter.Eval("GFunction()"));

test/DynamicExpresso.UnitTest/VariablesTest.cs

@@ -151,6 +151,20 @@ public void Keywords_with_same_overload_twice()
 			Assert.AreEqual(9.0, target.Eval("pow(3, 2)"));
 		}
 
+		[Test]
+		public void Replace_same_overload_signature()
+		{
+			Func<double, int> f1 = d => 1;
+			Func<double, int> f2 = d => 2;
+
+			var target = new Interpreter()
+									.SetFunction("f", f1)
+									.SetFunction("f", f2);
+
+			// f2 should override the f1 registration, because both delegates have the same signature
+			Assert.AreEqual(2, target.Eval("f(0d)"));
+		}
+
 		[Test]
 		public void Keywords_with_invalid_delegate_call()
 		{