diff --git a/src/embed_tests/TestMethodBinder.cs b/src/embed_tests/TestMethodBinder.cs index 355a96c3f..7f4c58d7e 100644 --- a/src/embed_tests/TestMethodBinder.cs +++ b/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) { diff --git a/src/perf_tests/Python.PerformanceTests.csproj b/src/perf_tests/Python.PerformanceTests.csproj index b437fe532..ba9456e3d 100644 --- a/src/perf_tests/Python.PerformanceTests.csproj +++ b/src/perf_tests/Python.PerformanceTests.csproj @@ -13,7 +13,7 @@ runtime; build; native; contentfiles; analyzers; buildtransitive - + compile @@ -25,7 +25,7 @@ - + diff --git a/src/runtime/MethodBinder.cs b/src/runtime/MethodBinder.cs index f598da499..25dd76621 100644 --- a/src/runtime/MethodBinder.cs +++ b/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 GetMethods() /// See: https://github.com/jythontools/jython/blob/master/src/org/python/core/ReflectedArgs.java#L192 /// private static int GetPrecedence(MethodInformation methodInformation) + { + return GetMatchedArgumentsPrecedence(methodInformation, null, null); + } + + /// + /// Gets the precedence of a method's arguments, considering only those arguments that have been matched, + /// that is, those that are not default values. + /// + private static int GetMatchedArgumentsPrecedence(MethodInformation methodInformation, int? matchedPositionalArgsCount, IEnumerable 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(); + 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,7 +375,7 @@ private static int GetPrecedence(MethodInformation methodInformation) /// /// Return a precedence value for a particular Type object. /// - internal static int ArgPrecedence(Type t, MethodInformation mi) + internal static int ArgPrecedence(Type t, bool isOperatorMethod) { Type objectType = typeof(object); if (t == objectType) @@ -360,7 +383,7 @@ internal static int ArgPrecedence(Type t, MethodInformation mi) 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(1) { new MethodInformation(info, true) }; + int pyArgCount = (int)Runtime.PyTuple_Size(args); var matches = new List(methods.Count); List 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(); - 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; } } diff --git a/src/runtime/Properties/AssemblyInfo.cs b/src/runtime/Properties/AssemblyInfo.cs index ffb1308a4..7ab968e35 100644 --- a/src/runtime/Properties/AssemblyInfo.cs +++ b/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")] diff --git a/src/runtime/Python.Runtime.csproj b/src/runtime/Python.Runtime.csproj index c579abaa5..e0d22a71e 100644 --- a/src/runtime/Python.Runtime.csproj +++ b/src/runtime/Python.Runtime.csproj @@ -5,7 +5,7 @@ Python.Runtime Python.Runtime QuantConnect.pythonnet - 2.0.39 + 2.0.40 false LICENSE https://github.com/pythonnet/pythonnet