Merge pull request QuantConnect#95 from jhonabreul/bug-matching-pyobject-args-overloads-first

Fix matching PyObject arguments overloads first

Author: jhonabreul

src/embed_tests/TestMethodBinder.cs

@@ -4,7 +4,6 @@
 using NUnit.Framework;
 using System.Collections.Generic;
 using System.Diagnostics;
-using static Python.Runtime.Py;
 
 namespace Python.EmbeddingTest
 {
@@ -71,6 +70,7 @@ def TestEnumerable(self):
         public void SetUp()
         {
             PythonEngine.Initialize();
+            using var _ = Py.GIL();
 
             try
             {
@@ -80,10 +80,7 @@ public void SetUp()
             {
             }
 
-            using (Py.GIL())
-            {
-                module = PyModule.FromString("module", testModule).GetAttr("PythonModel").Invoke();
-            }
+            module = PyModule.FromString("module", testModule).GetAttr("PythonModel").Invoke();
         }
 
         [OneTimeTearDown]
@@ -926,6 +923,131 @@ def call_method(instance):
             Assert.IsFalse(Exceptions.ErrorOccurred());
         }
 
+        public class CSharpClass2
+        {
+            public string CalledMethodMessage { get; private set; } = string.Empty;
+
+            public void Clear()
+            {
+                CalledMethodMessage = string.Empty;
+            }
+
+            public void Method()
+            {
+                CalledMethodMessage = "Overload 1";
+            }
+
+            public void Method(CSharpClass csharpClassArgument, decimal decimalArgument = 1.2m, PyObject pyObjectKwArgument = null)
+            {
+                CalledMethodMessage = "Overload 2";
+            }
+
+            public void Method(PyObject pyObjectArgument, decimal decimalArgument = 1.2m, object objectArgument = null)
+            {
+                CalledMethodMessage = "Overload 3";
+            }
+
+            // This must be matched when passing just a single argument and it's a PyObject,
+            // event though the PyObject kwarg in the second overload has more precedence.
+            // But since it will not be passed, this overload must be called.
+            public void Method(PyObject pyObjectArgument, decimal decimalArgument = 1.2m, int intArgument = 0)
+            {
+                CalledMethodMessage = "Overload 4";
+            }
+        }
+
+        [Test]
+        public void PyObjectArgsHavePrecedenceOverOtherTypes()
+        {
+            using var _ = Py.GIL();
+
+            var instance = new CSharpClass2();
+            using var pyInstance = instance.ToPython();
+            using var pyArg = new CSharpClass().ToPython();
+
+            Assert.DoesNotThrow(() =>
+            {
+                // We are passing a PyObject and not using the named arguments,
+                // that overload must be called without converting the PyObject to CSharpClass
+                pyInstance.InvokeMethod("Method", pyArg);
+            });
+
+            Assert.AreEqual("Overload 4", instance.CalledMethodMessage);
+            Assert.IsFalse(Exceptions.ErrorOccurred());
+            instance.Clear();
+
+            // With the first named argument
+            Assert.DoesNotThrow(() =>
+            {
+                using var kwargs = Py.kw("decimalArgument", 1.234m);
+                pyInstance.InvokeMethod("Method", new[] { pyArg }, kwargs);
+            });
+
+            Assert.AreEqual("Overload 4", instance.CalledMethodMessage);
+            Assert.IsFalse(Exceptions.ErrorOccurred());
+            instance.Clear();
+
+            // Snake case version
+            Assert.DoesNotThrow(() =>
+            {
+                using var kwargs = Py.kw("decimal_argument", 1.234m);
+                pyInstance.InvokeMethod("method", new[] { pyArg }, kwargs);
+            });
+
+            Assert.AreEqual("Overload 4", instance.CalledMethodMessage);
+            Assert.IsFalse(Exceptions.ErrorOccurred());
+        }
+
+        [Test]
+        public void OtherTypesHavePrecedenceOverPyObjectArgsIfMoreArgsAreMatched()
+        {
+            using var _ = Py.GIL();
+
+            var instance = new CSharpClass2();
+            using var pyInstance = instance.ToPython();
+            using var pyArg = new CSharpClass().ToPython();
+
+            Assert.DoesNotThrow(() =>
+            {
+                using var kwargs = Py.kw("pyObjectKwArgument", new CSharpClass2());
+                pyInstance.InvokeMethod("Method", new[] { pyArg }, kwargs);
+            });
+
+            Assert.AreEqual("Overload 2", instance.CalledMethodMessage);
+            Assert.IsFalse(Exceptions.ErrorOccurred());
+            instance.Clear();
+
+            Assert.DoesNotThrow(() =>
+            {
+                using var kwargs = Py.kw("py_object_kw_argument", new CSharpClass2());
+                pyInstance.InvokeMethod("method", new[] { pyArg }, kwargs);
+            });
+
+            Assert.AreEqual("Overload 2", instance.CalledMethodMessage);
+            Assert.IsFalse(Exceptions.ErrorOccurred());
+            instance.Clear();
+
+            Assert.DoesNotThrow(() =>
+            {
+                using var kwargs = Py.kw("objectArgument", "somestring");
+                pyInstance.InvokeMethod("Method", new[] { pyArg }, kwargs);
+            });
+
+            Assert.AreEqual("Overload 3", instance.CalledMethodMessage);
+            Assert.IsFalse(Exceptions.ErrorOccurred());
+            instance.Clear();
+
+            Assert.DoesNotThrow(() =>
+            {
+                using var kwargs = Py.kw("object_argument", "somestring");
+                pyInstance.InvokeMethod("method", new[] { pyArg }, kwargs);
+            });
+
+            Assert.AreEqual("Overload 3", instance.CalledMethodMessage);
+            Assert.IsFalse(Exceptions.ErrorOccurred());
+            instance.Clear();
+        }
+
         [Test]
         public void BindsConstructorToSnakeCasedArgumentsVersion([Values] bool useCamelCase, [Values] bool passOptionalArgument)
         {

src/perf_tests/Python.PerformanceTests.csproj

@@ -13,7 +13,7 @@
       <IncludeAssets>runtime; build; native; contentfiles; analyzers; buildtransitive</IncludeAssets>
     </PackageReference>
     <PackageReference Include="Microsoft.NET.Test.Sdk" Version="16.*" />
-    <PackageReference Include="quantconnect.pythonnet" Version="2.0.39" GeneratePathProperty="true">
+    <PackageReference Include="quantconnect.pythonnet" Version="2.0.40" GeneratePathProperty="true">
       <IncludeAssets>compile</IncludeAssets>
     </PackageReference>
   </ItemGroup>
@@ -25,7 +25,7 @@
   </Target>
 
   <Target Name="CopyBaseline" AfterTargets="Build">
-    <Copy SourceFiles="$(NuGetPackageRoot)quantconnect.pythonnet\2.0.39\lib\net6.0\Python.Runtime.dll" DestinationFolder="$(OutDir)baseline" />
+    <Copy SourceFiles="$(NuGetPackageRoot)quantconnect.pythonnet\2.0.40\lib\net6.0\Python.Runtime.dll" DestinationFolder="$(OutDir)baseline" />
   </Target>
 
   <Target Name="CopyNewBuild" AfterTargets="Build">

src/runtime/MethodBinder.cs

@@ -2,7 +2,6 @@
 using System.Collections;
 using System.Collections.Generic;
 using System.Linq;
-using System.Numerics;
 using System.Reflection;
 using System.Text;
 
@@ -320,22 +319,46 @@ internal List<MethodInformation> GetMethods()
         /// See: https://github.com/jythontools/jython/blob/master/src/org/python/core/ReflectedArgs.java#L192
         /// </remarks>
         private static int GetPrecedence(MethodInformation methodInformation)
+        {
+            return GetMatchedArgumentsPrecedence(methodInformation, null, null);
+        }
+
+        /// <summary>
+        /// Gets the precedence of a method's arguments, considering only those arguments that have been matched,
+        /// that is, those that are not default values.
+        /// </summary>
+        private static int GetMatchedArgumentsPrecedence(MethodInformation methodInformation, int? matchedPositionalArgsCount, IEnumerable<string> matchedKwargsNames)
         {
             ParameterInfo[] pi = methodInformation.ParameterInfo;
             var mi = methodInformation.MethodBase;
             int val = mi.IsStatic ? 3000 : 0;
-            int num = pi.Length;
+            var isOperatorMethod = OperatorMethod.IsOperatorMethod(methodInformation.MethodBase);
 
             val += mi.IsGenericMethod ? 1 : 0;
-            for (var i = 0; i < num; i++)
+
+            if (!matchedPositionalArgsCount.HasValue)
+            {
+                for (var i = 0; i < pi.Length; i++)
+                {
+                    val += ArgPrecedence(pi[i].ParameterType, isOperatorMethod);
+                }
+            }
+            else
             {
-                val += ArgPrecedence(pi[i].ParameterType, methodInformation);
+                matchedKwargsNames ??= Array.Empty<string>();
+                for (var i = 0; i < pi.Length; i++)
+                {
+                    if (i < matchedPositionalArgsCount || matchedKwargsNames.Contains(methodInformation.ParameterNames[i]))
+                    {
+                        val += ArgPrecedence(pi[i].ParameterType, isOperatorMethod);
+                    }
+                }
             }
 
             var info = mi as MethodInfo;
             if (info != null)
             {
-                val += ArgPrecedence(info.ReturnType, methodInformation);
+                val += ArgPrecedence(info.ReturnType, isOperatorMethod);
                 if (mi.DeclaringType == mi.ReflectedType)
                 {
                     val += methodInformation.IsOriginal ? 0 : 300000;
@@ -352,15 +375,15 @@ private static int GetPrecedence(MethodInformation methodInformation)
         /// <summary>
         /// Return a precedence value for a particular Type object.
         /// </summary>
-        internal static int ArgPrecedence(Type t, MethodInformation mi)
+        internal static int ArgPrecedence(Type t, bool isOperatorMethod)
         {
             Type objectType = typeof(object);
             if (t == objectType)
             {
                 return 3000;
             }
 
-            if (t.IsAssignableFrom(typeof(PyObject)) && !OperatorMethod.IsOperatorMethod(mi.MethodBase))
+            if (t.IsAssignableFrom(typeof(PyObject)) && !isOperatorMethod)
             {
                 return -1;
             }
@@ -372,7 +395,7 @@ internal static int ArgPrecedence(Type t, MethodInformation mi)
                 {
                     return 2500;
                 }
-                return 100 + ArgPrecedence(e, mi);
+                return 100 + ArgPrecedence(e, isOperatorMethod);
             }
 
             TypeCode tc = Type.GetTypeCode(t);
@@ -452,6 +475,7 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe
             var methods = info == null ? GetMethods()
                 : new List<MethodInformation>(1) { new MethodInformation(info, true) };
 
+            int pyArgCount = (int)Runtime.PyTuple_Size(args);
             var matches = new List<MatchedMethod>(methods.Count);
             List<MatchedMethod> matchesUsingImplicitConversion = null;
 
@@ -463,7 +487,6 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe
                 var pi = methodInformation.ParameterInfo;
                 // Avoid accessing the parameter names property unless necessary
                 var paramNames = hasNamedArgs ? methodInformation.ParameterNames : Array.Empty<string>();
-                int pyArgCount = (int)Runtime.PyTuple_Size(args);
 
                 // Special case for operators
                 bool isOperator = OperatorMethod.IsOperatorMethod(mi);
@@ -695,7 +718,7 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe
                         }
                     }
 
-                    var match = new MatchedMethod(kwargsMatched, margs, outs, mi);
+                    var match = new MatchedMethod(kwargsMatched, margs, outs, methodInformation);
                     if (usedImplicitConversion)
                     {
                         if (matchesUsingImplicitConversion == null)
@@ -718,8 +741,17 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe
                 // We favor matches that do not use implicit conversion
                 var matchesTouse = matches.Count > 0 ? matches : matchesUsingImplicitConversion;
 
-                // The best match would be the one with the most named arguments matched
-                var bestMatch = matchesTouse.MaxBy(x => x.KwargsMatched);
+                // The best match would be the one with the most named arguments matched.
+                // But if multiple matches have the same max number of named arguments matched,
+                // we solve the ambiguity by taking the one with the highest precedence but only
+                // considering the actual arguments passed, ignoring the optional arguments for
+                // which the default values were used
+                var bestMatch = matchesTouse
+                    .GroupBy(x => x.KwargsMatched)
+                    .OrderByDescending(x => x.Key)
+                    .First()
+                    .MinBy(x => GetMatchedArgumentsPrecedence(x.MethodInformation, pyArgCount, kwArgDict?.Keys));
+
                 var margs = bestMatch.ManagedArgs;
                 var outs = bestMatch.Outs;
                 var mi = bestMatch.Method;
@@ -1084,14 +1116,15 @@ private readonly struct MatchedMethod
             public int KwargsMatched { get; }
             public object?[] ManagedArgs { get; }
             public int Outs { get; }
-            public MethodBase Method { get; }
+            public MethodInformation MethodInformation { get; }
+            public MethodBase Method => MethodInformation.MethodBase;
 
-            public MatchedMethod(int kwargsMatched, object?[] margs, int outs, MethodBase mb)
+            public MatchedMethod(int kwargsMatched, object?[] margs, int outs, MethodInformation methodInformation)
             {
                 KwargsMatched = kwargsMatched;
                 ManagedArgs = margs;
                 Outs = outs;
-                Method = mb;
+                MethodInformation = methodInformation;
             }
         }
 

src/runtime/Properties/AssemblyInfo.cs

@@ -4,5 +4,5 @@
 [assembly: InternalsVisibleTo("Python.EmbeddingTest, PublicKey=00240000048000009400000006020000002400005253413100040000110000005ffd8f49fb44ab0641b3fd8d55e749f716e6dd901032295db641eb98ee46063cbe0d4a1d121ef0bc2af95f8a7438d7a80a3531316e6b75c2dae92fb05a99f03bf7e0c03980e1c3cfb74ba690aca2f3339ef329313bcc5dccced125a4ffdc4531dcef914602cd5878dc5fbb4d4c73ddfbc133f840231343e013762884d6143189")]
 [assembly: InternalsVisibleTo("Python.Test, PublicKey=00240000048000009400000006020000002400005253413100040000110000005ffd8f49fb44ab0641b3fd8d55e749f716e6dd901032295db641eb98ee46063cbe0d4a1d121ef0bc2af95f8a7438d7a80a3531316e6b75c2dae92fb05a99f03bf7e0c03980e1c3cfb74ba690aca2f3339ef329313bcc5dccced125a4ffdc4531dcef914602cd5878dc5fbb4d4c73ddfbc133f840231343e013762884d6143189")]
 
-[assembly: AssemblyVersion("2.0.39")]
-[assembly: AssemblyFileVersion("2.0.39")]
+[assembly: AssemblyVersion("2.0.40")]
+[assembly: AssemblyFileVersion("2.0.40")]

src/runtime/Python.Runtime.csproj

@@ -5,7 +5,7 @@
     <RootNamespace>Python.Runtime</RootNamespace>
     <AssemblyName>Python.Runtime</AssemblyName>
     <PackageId>QuantConnect.pythonnet</PackageId>
-    <Version>2.0.39</Version>
+    <Version>2.0.40</Version>
     <GenerateAssemblyInfo>false</GenerateAssemblyInfo>
     <PackageLicenseFile>LICENSE</PackageLicenseFile>
     <RepositoryUrl>https://github.com/pythonnet/pythonnet</RepositoryUrl>