Fix kwarg func resolution (#1136)

jmlidbetter · filmor · web-flow · commit c81c3c3db7fc · 2020-11-18T10:48:18.000-08:00
Fix function resolution when calling overloads with keyword arguments

Co-authored-by: Benedikt Reinartz &lt;filmor@gmail.com&gt;
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -25,6 +25,7 @@ details about the cause of the failure
 
 -    Fix incorrect dereference of wrapper object in `tp_repr`, which may result in a program crash
 -    Fix incorrect dereference in params array handling
+-    Fixes issue with function resolution when calling overloaded function with keyword arguments from python ([#1097][i1097])
 -    Fix `object[]` parameters taking precedence when should not in overload resolution
 -    Fixed a bug where all .NET class instances were considered Iterable
 -    Fix incorrect choice of method to invoke when using keyword arguments.
diff --git a/src/runtime/methodbinder.cs b/src/runtime/methodbinder.cs
@@ -279,6 +279,23 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info)
             return Bind(inst, args, kw, info, null);
         }
 
+        private readonly struct MatchedMethod {
+            public MatchedMethod(int kwargsMatched, int defaultsNeeded, object[] margs, int outs, MethodBase mb)
+            {
+                KwargsMatched = kwargsMatched;
+                DefaultsNeeded = defaultsNeeded;
+                ManagedArgs = margs;
+                Outs = outs;
+                Method = mb;
+            }
+
+            public int KwargsMatched { get; }
+            public int DefaultsNeeded { get; }
+            public object[] ManagedArgs { get; }
+            public int Outs { get; }
+            public MethodBase Method { get; }
+        }
+
         internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info, MethodInfo[] methodinfo)
         {
             // loop to find match, return invoker w/ or /wo error
@@ -311,6 +328,8 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info, Meth
                 _methods = GetMethods();
             }
 
+            var argMatchedMethods = new List<MatchedMethod>(_methods.Length);
+
             // TODO: Clean up
             foreach (MethodBase mi in _methods)
             {
@@ -321,8 +340,10 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info, Meth
                 ParameterInfo[] pi = mi.GetParameters();
                 ArrayList defaultArgList;
                 bool paramsArray;
+                int kwargsMatched;
+                int defaultsNeeded;
 
-                if (!MatchesArgumentCount(pynargs, pi, kwargDict, out paramsArray, out defaultArgList))
+                if (!MatchesArgumentCount(pynargs, pi, kwargDict, out paramsArray, out defaultArgList, out kwargsMatched, out defaultsNeeded))
                 {
                     continue;
                 }
@@ -336,6 +357,47 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info, Meth
                     continue;
                 }
 
+                var matchedMethod = new MatchedMethod(kwargsMatched, defaultsNeeded, margs, outs, mi);
+                argMatchedMethods.Add(matchedMethod);
+            }
+            if (argMatchedMethods.Count > 0)
+            {
+                var bestKwargMatchCount = argMatchedMethods.Max(x => x.KwargsMatched);
+                var fewestDefaultsRequired = argMatchedMethods.Where(x => x.KwargsMatched == bestKwargMatchCount).Min(x => x.DefaultsNeeded);
+
+                int bestCount = 0;
+                int bestMatchIndex = -1;
+
+                for (int index = 0; index < argMatchedMethods.Count; index++)
+                {
+                    var testMatch = argMatchedMethods[index];
+                    if (testMatch.DefaultsNeeded == fewestDefaultsRequired && testMatch.KwargsMatched == bestKwargMatchCount)
+                    {
+                        bestCount++;
+                        if (bestMatchIndex == -1)
+                            bestMatchIndex = index;
+                    }
+                }
+
+                if (bestCount > 1 && fewestDefaultsRequired > 0)
+                {
+                    // Best effort for determining method to match on gives multiple possible
+                    // matches and we need at least one default argument - bail from this point
+                    return null;
+                }
+
+                // If we're here either:
+                //      (a) There is only one best match
+                //      (b) There are multiple best matches but none of them require
+                //          default arguments
+                // in the case of (a) we're done by default. For (b) regardless of which
+                // method we choose, all arguments are specified _and_ can be converted
+                // from python to C# so picking any will suffice
+                MatchedMethod bestMatch = argMatchedMethods[bestMatchIndex];
+                var margs = bestMatch.ManagedArgs;
+                var outs = bestMatch.Outs;
+                var mi = bestMatch.Method;
+
                 object target = null;
                 if (!mi.IsStatic && inst != IntPtr.Zero)
                 {
@@ -575,11 +637,16 @@ static Type TryComputeClrArgumentType(Type parameterType, IntPtr argument, bool
         static bool MatchesArgumentCount(int positionalArgumentCount, ParameterInfo[] parameters,
             Dictionary<string, IntPtr> kwargDict,
             out bool paramsArray,
-            out ArrayList defaultArgList)
+            out ArrayList defaultArgList,
+            out int kwargsMatched,
+            out int defaultsNeeded)
         {
             defaultArgList = null;
             var match = false;
             paramsArray = parameters.Length > 0 ? Attribute.IsDefined(parameters[parameters.Length - 1], typeof(ParamArrayAttribute)) : false;
+            var kwargCount = kwargDict.Count;
+            kwargsMatched = 0;
+            defaultsNeeded = 0;
 
             if (positionalArgumentCount == parameters.Length && kwargDict.Count == 0)
             {
@@ -599,6 +666,7 @@ static bool MatchesArgumentCount(int positionalArgumentCount, ParameterInfo[] pa
                         // no need to check for a default parameter, but put a null
                         // placeholder in defaultArgList
                         defaultArgList.Add(null);
+                        kwargsMatched++;
                     }
                     else if (parameters[v].IsOptional)
                     {
@@ -607,6 +675,7 @@ static bool MatchesArgumentCount(int positionalArgumentCount, ParameterInfo[] pa
                         // The GetDefaultValue() extension method will return the value
                         // to be passed in as the parameter value
                         defaultArgList.Add(parameters[v].GetDefaultValue());
+                        defaultsNeeded++;
                     }
                     else if(!paramsArray)
                     {
diff --git a/src/testing/methodtest.cs b/src/testing/methodtest.cs
@@ -684,7 +684,25 @@ public static string OptionalAndDefaultParams2([Optional]int a, [Optional]int b,
             return string.Format("{0}{1}{2}{3}", a, b, c, d);
         }
 
-        
+        public static string DefaultParamsWithOverloading(int a = 2, int b = 1)
+        {
+            return $"{a}{b}";
+        }
+
+        public static string DefaultParamsWithOverloading(string a = "a", string b = "b")
+        {
+            return $"{a}{b}X";
+        }
+
+        public static string DefaultParamsWithOverloading(int a = 0, int b = 1, int c = 2)
+        {
+            return $"{a}{b}{c}XX";
+        }
+
+        public static string DefaultParamsWithOverloading(int a = 5, int b = 6, int c = 7, int d = 8)
+        {
+            return $"{a}{b}{c}{d}XXX";
+        }
     }
 
 
diff --git a/src/tests/test_method.py b/src/tests/test_method.py
@@ -1153,6 +1153,35 @@ def test_optional_and_default_params():
     res = MethodTest.OptionalAndDefaultParams2(b=2, c=3)
     assert res == "0232"
 
+def test_default_params_overloads():
+    res = MethodTest.DefaultParamsWithOverloading(1, 2)
+    assert res == "12"
+
+    res = MethodTest.DefaultParamsWithOverloading(b=5)
+    assert res == "25"
+
+    res = MethodTest.DefaultParamsWithOverloading("d")
+    assert res == "dbX"
+
+    res = MethodTest.DefaultParamsWithOverloading(b="c")
+    assert res == "acX"
+
+    res = MethodTest.DefaultParamsWithOverloading(c=3)
+    assert res == "013XX"
+
+    res = MethodTest.DefaultParamsWithOverloading(5, c=2)
+    assert res == "512XX"
+
+    res = MethodTest.DefaultParamsWithOverloading(c=0, d=1)
+    assert res == "5601XXX"
+
+    res = MethodTest.DefaultParamsWithOverloading(1, d=1)
+    assert res == "1671XXX"
+
+def test_default_params_overloads_ambiguous_call():
+    with pytest.raises(TypeError):
+        MethodTest.DefaultParamsWithOverloading()
+
 def test_keyword_arg_method_resolution():
     from Python.Test import MethodArityTest
 
