From 3db752a04d10bc3b18f94670d3b4c718308e8fb8 Mon Sep 17 00:00:00 2001 From: Jhonathan Abreu Date: Fri, 10 May 2024 17:36:16 -0400 Subject: [PATCH 1/7] Fix type instance conversion --- src/embed_tests/TestConverter.cs | 25 ++++++++++++++ src/embed_tests/TestMethodBinder.cs | 51 +++++++++++++++++++++++++++++ src/runtime/Converter.cs | 8 ++++- 3 files changed, 83 insertions(+), 1 deletion(-) diff --git a/src/embed_tests/TestConverter.cs b/src/embed_tests/TestConverter.cs index 40ed9ff48..88809e7f7 100644 --- a/src/embed_tests/TestConverter.cs +++ b/src/embed_tests/TestConverter.cs @@ -450,6 +450,27 @@ public void PrimitiveIntConversion() var testInt = pyValue.As(); Assert.AreEqual(testInt , 10); } + + [TestCase(typeof(Type), true)] + [TestCase(typeof(string), false)] + [TestCase(typeof(TestCSharpModel), false)] + public void NoErrorSetWhenFailingToConvertClassType(Type type, bool shouldConvert) + { + using var _ = Py.GIL(); + + var module = PyModule.FromString("CallsCorrectOverloadWithoutErrors", @" +from clr import AddReference +AddReference(""System"") +AddReference(""Python.EmbeddingTest"") +from Python.EmbeddingTest import * + +class TestPythonModel(TestCSharpModel): + pass +"); + var testPythonModelClass = module.GetAttr("TestPythonModel"); + Assert.AreEqual(shouldConvert, Converter.ToManaged(testPythonModelClass, type, out var result, setError: false)); + Assert.IsFalse(Exceptions.ErrorOccurred()); + } } public interface IGetList @@ -461,4 +482,8 @@ public class GetListImpl : IGetList { public List GetList() => new() { "testing" }; } + + public class TestCSharpModel + { + } } diff --git a/src/embed_tests/TestMethodBinder.cs b/src/embed_tests/TestMethodBinder.cs index f3a65b477..e8154d1a3 100644 --- a/src/embed_tests/TestMethodBinder.cs +++ b/src/embed_tests/TestMethodBinder.cs @@ -740,6 +740,57 @@ from Python.EmbeddingTest import * ")); } + public class CSharpClass + { + public string CalledMethodMessage { get; private set; } + + public void Method() + { + CalledMethodMessage = "Overload 1"; + } + + public void Method(string stringArgument, decimal decimalArgument = 1.2m) + { + CalledMethodMessage = "Overload 2"; + } + + public void Method(PyObject typeArgument, decimal decimalArgument = 1.2m) + { + CalledMethodMessage = "Overload 3"; + } + } + + [Test] + public void CallsCorrectOverloadWithoutErrors() + { + using var _ = Py.GIL(); + + var module = PyModule.FromString("CallsCorrectOverloadWithoutErrors", @" +from clr import AddReference +AddReference(""System"") +AddReference(""Python.EmbeddingTest"") +from Python.EmbeddingTest import * + +class PythonModel(TestMethodBinder.CSharpModel): + pass + +def call_method(instance): + instance.Method(PythonModel, decimalArgument=1.234) +"); + + var instance = new CSharpClass(); + using var pyInstance = instance.ToPython(); + + Assert.DoesNotThrow(() => + { + module.GetAttr("call_method").Invoke(pyInstance); + }); + + Assert.AreEqual("Overload 3", instance.CalledMethodMessage); + + Assert.IsFalse(Exceptions.ErrorOccurred()); + } + // Used to test that we match this function with Py DateTime & Date Objects public static int GetMonth(DateTime test) diff --git a/src/runtime/Converter.cs b/src/runtime/Converter.cs index 2783f0037..fd028df0c 100644 --- a/src/runtime/Converter.cs +++ b/src/runtime/Converter.cs @@ -473,7 +473,13 @@ internal static bool ToManagedValue(BorrowedReference value, Type obType, } if (mt is ClassBase cb) { - if (!cb.type.Valid || !obType.IsInstanceOfType(cb.type.Value)) + // The value being converted is a class type, so it will only succeed if it's being converted into a Type + if (obType != typeof(Type)) + { + return false; + } + + if (!cb.type.Valid) { Exceptions.SetError(Exceptions.TypeError, cb.type.DeletedMessage); return false; From 4179ce3faa2ca6b297e3a469d5149ddc40ffbb04 Mon Sep 17 00:00:00 2001 From: Jhonathan Abreu Date: Fri, 10 May 2024 17:37:15 -0400 Subject: [PATCH 2/7] Update version to 2.0.37 --- src/perf_tests/Python.PerformanceTests.csproj | 4 ++-- src/runtime/Properties/AssemblyInfo.cs | 4 ++-- src/runtime/Python.Runtime.csproj | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/perf_tests/Python.PerformanceTests.csproj b/src/perf_tests/Python.PerformanceTests.csproj index 1c427fd6f..c0990cf9b 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/Properties/AssemblyInfo.cs b/src/runtime/Properties/AssemblyInfo.cs index 0714dc6e2..0c15263c9 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.36")] -[assembly: AssemblyFileVersion("2.0.36")] +[assembly: AssemblyVersion("2.0.37")] +[assembly: AssemblyFileVersion("2.0.37")] diff --git a/src/runtime/Python.Runtime.csproj b/src/runtime/Python.Runtime.csproj index a83f17199..5b3276dbe 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.36 + 2.0.37 false LICENSE https://github.com/pythonnet/pythonnet From 6ecae97fe8cf109e389d9c43da2090252e01b0b4 Mon Sep 17 00:00:00 2001 From: Jhonathan Abreu Date: Mon, 13 May 2024 18:07:53 -0400 Subject: [PATCH 3/7] Fixes: matching method overloads with named arguments --- src/embed_tests/TestMethodBinder.cs | 92 ++++++++++++++++++++++++ src/runtime/Converter.cs | 15 +++- src/runtime/MethodBinder.cs | 108 ++++++++++++++++++---------- 3 files changed, 175 insertions(+), 40 deletions(-) diff --git a/src/embed_tests/TestMethodBinder.cs b/src/embed_tests/TestMethodBinder.cs index e8154d1a3..99b6f4dd7 100644 --- a/src/embed_tests/TestMethodBinder.cs +++ b/src/embed_tests/TestMethodBinder.cs @@ -740,6 +740,98 @@ from Python.EmbeddingTest import * ")); } + public class OverloadsTestClass + { + + public string Method1(string positionalArg, decimal namedArg1 = 1.2m, int namedArg2 = 123) + { + Console.WriteLine("1"); + return "Method1 Overload 1"; + } + + public string Method1(decimal namedArg1 = 1.2m, int namedArg2 = 123) + { + Console.WriteLine("2"); + return "Method1 Overload 2"; + } + + // ---- + + public string Method2(string arg1, int arg2, float arg3, float kwarg1 = 1.1f, bool kwarg2 = false, string kwarg3 = "") + { + return "Method2 Overload 1"; + } + + public string Method2(string arg1, int arg2, float kwarg1 = 1.1f, bool kwarg2 = false, string kwarg3 = "") + { + return "Method2 Overload 2"; + } + + // ---- + + public string ImplicitConversionSameArgumentCount(string symbol, int quantity, float trailingAmount, bool trailingAsPercentage, string tag = "") + { + return "ImplicitConversionSameArgumentCount 1"; + } + + public string ImplicitConversionSameArgumentCount(string symbol, decimal quantity, decimal trailingAmount, bool trailingAsPercentage, string tag = "") + { + return "ImplicitConversionSameArgumentCount 2"; + } + + public string ImplicitConversionSameArgumentCount2(string symbol, int quantity, float trailingAmount, bool trailingAsPercentage, string tag = "") + { + return "ImplicitConversionSameArgumentCount2 1"; + } + + public string ImplicitConversionSameArgumentCount2(string symbol, float quantity, float trailingAmount, bool trailingAsPercentage, string tag = "") + { + return "ImplicitConversionSameArgumentCount2 2"; + } + + public string ImplicitConversionSameArgumentCount2(string symbol, decimal quantity, float trailingAmount, bool trailingAsPercentage, string tag = "") + { + return "ImplicitConversionSameArgumentCount2 2"; + } + } + + [TestCase("Method1('abc', namedArg1=1.234, namedArg2=321)", "Method1 Overload 1")] + [TestCase("Method2(\"SPY\", 10, 123.4, kwarg1=0.0025, kwarg2=True)", "Method2 Overload 1")] + public void SelectsRightOverloadWithNamedParameters(string methodCallCode, string expectedResult) + { + using var _ = Py.GIL(); + + dynamic module = PyModule.FromString("SelectsRightOverloadWithNamedParameters", @$" + +def call_method(instance): + return instance.{methodCallCode} +"); + + var instance = new OverloadsTestClass(); + var result = module.call_method(instance).As(); + + Assert.AreEqual(expectedResult, result); + } + + [TestCase("ImplicitConversionSameArgumentCount", "10", "ImplicitConversionSameArgumentCount 1")] + [TestCase("ImplicitConversionSameArgumentCount", "10.1", "ImplicitConversionSameArgumentCount 2")] + [TestCase("ImplicitConversionSameArgumentCount2", "10", "ImplicitConversionSameArgumentCount2 1")] + [TestCase("ImplicitConversionSameArgumentCount2", "10.1", "ImplicitConversionSameArgumentCount2 2")] + public void DisambiguatesOverloadWithSameArgumentCountAndImplicitConversion(string methodName, string quantity, string expectedResult) + { + using var _ = Py.GIL(); + + dynamic module = PyModule.FromString("DisambiguatesOverloadWithSameArgumentCountAndImplicitConversion", @$" +def call_method(instance): + return instance.{methodName}(""SPY"", {quantity}, 123.4, trailingAsPercentage=True) +"); + + var instance = new OverloadsTestClass(); + var result = module.call_method(instance).As(); + + Assert.AreEqual(expectedResult, result); + } + public class CSharpClass { public string CalledMethodMessage { get; private set; } diff --git a/src/runtime/Converter.cs b/src/runtime/Converter.cs index fd028df0c..047f7a03a 100644 --- a/src/runtime/Converter.cs +++ b/src/runtime/Converter.cs @@ -851,8 +851,21 @@ internal static bool ToPrimitive(BorrowedReference value, Type obType, out objec } case TypeCode.Boolean: - result = Runtime.PyObject_IsTrue(value) != 0; + if (value == Runtime.PyTrue) + { + result = true; + return true; + } + if (value == Runtime.PyFalse) + { + result = false; return true; + } + if (setError) + { + goto type_error; + } + return false; case TypeCode.Byte: { diff --git a/src/runtime/MethodBinder.cs b/src/runtime/MethodBinder.cs index e951724f2..af2796649 100644 --- a/src/runtime/MethodBinder.cs +++ b/src/runtime/MethodBinder.cs @@ -456,6 +456,9 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe var methods = info == null ? GetMethods() : new List(1) { new MethodInformation(info, true) }; + var matches = new List(); + var matchesUsingImplicitConversion = new List(); + for (var i = 0; i < methods.Count; i++) { var methodInformation = methods[i]; @@ -504,6 +507,8 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe int paramsArrayIndex = paramsArray ? pi.Length - 1 : -1; // -1 indicates no paramsArray var usedImplicitConversion = false; + var kwargsMatched = 0; + var defaultsNeeded = 0; // Conversion loop for each parameter for (int paramIndex = 0; paramIndex < clrArgCount; paramIndex++) @@ -513,18 +518,24 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe var parameter = pi[paramIndex]; // Clr parameter we are targeting object arg; // Python -> Clr argument + var hasNamedParam = kwArgDict == null ? false : kwArgDict.TryGetValue(paramNames[paramIndex], out tempPyObject); + + // Check positional arguments first and then check for named arguments and optional values + if (paramIndex >= pyArgCount) + { + // All positional arguments have been used: // Check our KWargs for this parameter - bool hasNamedParam = kwArgDict == null ? false : kwArgDict.TryGetValue(paramNames[paramIndex], out tempPyObject); + if (hasNamedParam) + { + kwargsMatched++; if (tempPyObject != null) { op = tempPyObject; } - - NewReference tempObject = default; - - // Check if we are going to use default - if (paramIndex >= pyArgCount && !(hasNamedParam || (paramsArray && paramIndex == paramsArrayIndex))) + } + else if (parameter.IsOptional && !(hasNamedParam || (paramsArray && paramIndex == paramsArrayIndex))) { + defaultsNeeded++; if (defaultArgList != null) { margs[paramIndex] = defaultArgList[paramIndex - pyArgCount]; @@ -532,6 +543,9 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe continue; } + } + + NewReference tempObject = default; // At this point, if op is IntPtr.Zero we don't have a KWArg and are not using default if (op == null) @@ -601,9 +615,7 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe typematch = true; clrtype = parameter.ParameterType; } - // lets just keep the first binding using implicit conversion - // this is to respect method order/precedence - else if (bindingUsingImplicitConversion == null) + else { // accepts non-decimal numbers in decimal parameters if (underlyingType == typeof(decimal)) @@ -687,23 +699,49 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe } } - object target = null; + var match = new MatchedMethod(kwargsMatched, margs, outs, mi); + if (usedImplicitConversion) + { + matchesUsingImplicitConversion.Add(match); + } + else + { + matches.Add(match); + } + } + } + + if (matches.Count > 0 || matchesUsingImplicitConversion.Count > 0) + { + // 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); + var margs = bestMatch.ManagedArgs; + var outs = bestMatch.Outs; + var mi = bestMatch.Method; + + object? target = null; if (!mi.IsStatic && inst != null) { //CLRObject co = (CLRObject)ManagedType.GetManagedObject(inst); // InvalidCastException: Unable to cast object of type // 'Python.Runtime.ClassObject' to type 'Python.Runtime.CLRObject' - var co = ManagedType.GetManagedObject(inst) as CLRObject; // Sanity check: this ensures a graceful exit if someone does // something intentionally wrong like call a non-static method // on the class rather than on an instance of the class. // XXX maybe better to do this before all the other rigmarole. - if (co == null) + if (ManagedType.GetManagedObject(inst) is CLRObject co) + { + target = co.inst; + } + else { + Exceptions.SetError(Exceptions.TypeError, "Invoked a non-static method with an invalid instance"); return null; } - target = co.inst; } // If this match is generic we need to resolve it with our types. @@ -711,34 +749,9 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe if (mi.IsGenericMethod) { mi = ResolveGenericMethod((MethodInfo)mi, margs); - genericBinding = new Binding(mi, target, margs, outs); - continue; - } - - var binding = new Binding(mi, target, margs, outs); - if (usedImplicitConversion) - { - // in this case we will not return the binding yet in case there is a match - // which does not use implicit conversions, which will return directly - bindingUsingImplicitConversion = binding; - } - else - { - return binding; - } - } - } - - // if we generated a binding using implicit conversion return it - if (bindingUsingImplicitConversion != null) - { - return bindingUsingImplicitConversion; } - // if we generated a generic binding, return it - if (genericBinding != null) - { - return genericBinding; + return new Binding(mi, target, margs, outs); } return null; @@ -1063,6 +1076,23 @@ public int Compare(MethodInformation x, MethodInformation y) return 0; } } + + private readonly struct MatchedMethod + { + public int KwargsMatched { get; } + public object?[] ManagedArgs { get; } + public int Outs { get; } + public MethodBase Method { get; } + + public MatchedMethod(int kwargsMatched, object?[] margs, int outs, MethodBase mb) + { + KwargsMatched = kwargsMatched; + ManagedArgs = margs; + Outs = outs; + Method = mb; + } + } + protected static void AppendArgumentTypes(StringBuilder to, BorrowedReference args) { long argCount = Runtime.PyTuple_Size(args); From 4b9d7f4b4864c6a1ca82ad68cf3504dbde8d980e Mon Sep 17 00:00:00 2001 From: Jhonathan Abreu Date: Tue, 14 May 2024 09:58:27 -0400 Subject: [PATCH 4/7] Cleanup --- src/runtime/MethodBinder.cs | 62 ++++++++++++++++++------------------- 1 file changed, 30 insertions(+), 32 deletions(-) diff --git a/src/runtime/MethodBinder.cs b/src/runtime/MethodBinder.cs index af2796649..c995bae93 100644 --- a/src/runtime/MethodBinder.cs +++ b/src/runtime/MethodBinder.cs @@ -508,7 +508,6 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe int paramsArrayIndex = paramsArray ? pi.Length - 1 : -1; // -1 indicates no paramsArray var usedImplicitConversion = false; var kwargsMatched = 0; - var defaultsNeeded = 0; // Conversion loop for each parameter for (int paramIndex = 0; paramIndex < clrArgCount; paramIndex++) @@ -524,25 +523,24 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe if (paramIndex >= pyArgCount) { // All positional arguments have been used: - // Check our KWargs for this parameter + // Check our KWargs for this parameter if (hasNamedParam) { kwargsMatched++; - if (tempPyObject != null) - { - op = tempPyObject; - } + if (tempPyObject != null) + { + op = tempPyObject; + } } else if (parameter.IsOptional && !(hasNamedParam || (paramsArray && paramIndex == paramsArrayIndex))) - { - defaultsNeeded++; - if (defaultArgList != null) { - margs[paramIndex] = defaultArgList[paramIndex - pyArgCount]; - } + if (defaultArgList != null) + { + margs[paramIndex] = defaultArgList[paramIndex - pyArgCount]; + } - continue; - } + continue; + } } NewReference tempObject = default; @@ -723,33 +721,33 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe var mi = bestMatch.Method; object? target = null; - if (!mi.IsStatic && inst != null) - { - //CLRObject co = (CLRObject)ManagedType.GetManagedObject(inst); - // InvalidCastException: Unable to cast object of type - // 'Python.Runtime.ClassObject' to type 'Python.Runtime.CLRObject' - - // Sanity check: this ensures a graceful exit if someone does - // something intentionally wrong like call a non-static method - // on the class rather than on an instance of the class. - // XXX maybe better to do this before all the other rigmarole. + if (!mi.IsStatic && inst != null) + { + //CLRObject co = (CLRObject)ManagedType.GetManagedObject(inst); + // InvalidCastException: Unable to cast object of type + // 'Python.Runtime.ClassObject' to type 'Python.Runtime.CLRObject' + + // Sanity check: this ensures a graceful exit if someone does + // something intentionally wrong like call a non-static method + // on the class rather than on an instance of the class. + // XXX maybe better to do this before all the other rigmarole. if (ManagedType.GetManagedObject(inst) is CLRObject co) { target = co.inst; } else - { + { Exceptions.SetError(Exceptions.TypeError, "Invoked a non-static method with an invalid instance"); - return null; - } + return null; } + } - // If this match is generic we need to resolve it with our types. - // Store this generic match to be used if no others match - if (mi.IsGenericMethod) - { - mi = ResolveGenericMethod((MethodInfo)mi, margs); - } + // If this match is generic we need to resolve it with our types. + // Store this generic match to be used if no others match + if (mi.IsGenericMethod) + { + mi = ResolveGenericMethod((MethodInfo)mi, margs); + } return new Binding(mi, target, margs, outs); } From 24a43082fc169a276453fd4cc56d55683d4d46f8 Mon Sep 17 00:00:00 2001 From: Jhonathan Abreu Date: Tue, 14 May 2024 11:40:17 -0400 Subject: [PATCH 5/7] Cleanup --- src/embed_tests/TestMethodBinder.cs | 23 +++++++++++++++++++---- src/runtime/MethodBinder.cs | 24 ++++++++++++++---------- 2 files changed, 33 insertions(+), 14 deletions(-) diff --git a/src/embed_tests/TestMethodBinder.cs b/src/embed_tests/TestMethodBinder.cs index 99b6f4dd7..e377b5f83 100644 --- a/src/embed_tests/TestMethodBinder.cs +++ b/src/embed_tests/TestMethodBinder.cs @@ -757,18 +757,30 @@ public string Method1(decimal namedArg1 = 1.2m, int namedArg2 = 123) // ---- - public string Method2(string arg1, int arg2, float arg3, float kwarg1 = 1.1f, bool kwarg2 = false, string kwarg3 = "") + public string Method2(string arg1, int arg2, decimal arg3, decimal kwarg1 = 1.1m, bool kwarg2 = false, string kwarg3 = "") { return "Method2 Overload 1"; } - public string Method2(string arg1, int arg2, float kwarg1 = 1.1f, bool kwarg2 = false, string kwarg3 = "") + public string Method2(string arg1, int arg2, decimal kwarg1 = 1.1m, bool kwarg2 = false, string kwarg3 = "") { return "Method2 Overload 2"; } // ---- + public string Method3(string arg1, int arg2, float arg3, float kwarg1 = 1.1f, bool kwarg2 = false, string kwarg3 = "") + { + return "Method3 Overload 1"; + } + + public string Method3(string arg1, int arg2, float kwarg1 = 1.1f, bool kwarg2 = false, string kwarg3 = "") + { + return "Method3 Overload 2"; + } + + // ---- + public string ImplicitConversionSameArgumentCount(string symbol, int quantity, float trailingAmount, bool trailingAsPercentage, string tag = "") { return "ImplicitConversionSameArgumentCount 1"; @@ -795,8 +807,11 @@ public string ImplicitConversionSameArgumentCount2(string symbol, decimal quanti } } - [TestCase("Method1('abc', namedArg1=1.234, namedArg2=321)", "Method1 Overload 1")] - [TestCase("Method2(\"SPY\", 10, 123.4, kwarg1=0.0025, kwarg2=True)", "Method2 Overload 1")] + [TestCase("Method1('abc', namedArg1=10, namedArg2=321)", "Method1 Overload 1")] + [TestCase("Method1('abc', namedArg1=12.34, namedArg2=321)", "Method1 Overload 1")] + [TestCase("Method2(\"SPY\", 10, 123, kwarg1=1, kwarg2=True)", "Method2 Overload 1")] + [TestCase("Method2(\"SPY\", 10, 123.34, kwarg1=1.23, kwarg2=True)", "Method2 Overload 1")] + [TestCase("Method3(\"SPY\", 10, 123.34, kwarg1=1.23, kwarg2=True)", "Method3 Overload 1")] public void SelectsRightOverloadWithNamedParameters(string methodCallCode, string expectedResult) { using var _ = Py.GIL(); diff --git a/src/runtime/MethodBinder.cs b/src/runtime/MethodBinder.cs index c995bae93..c81ef35e4 100644 --- a/src/runtime/MethodBinder.cs +++ b/src/runtime/MethodBinder.cs @@ -431,10 +431,6 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedReference kw, MethodBase info) { - // Relevant function variables used post conversion - Binding bindingUsingImplicitConversion = null; - Binding genericBinding = null; - // If we have KWArgs create dictionary and collect them Dictionary kwArgDict = null; if (kw != null) @@ -456,8 +452,8 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe var methods = info == null ? GetMethods() : new List(1) { new MethodInformation(info, true) }; - var matches = new List(); - var matchesUsingImplicitConversion = new List(); + var matches = new List(methods.Count); + List matchesUsingImplicitConversion = null; for (var i = 0; i < methods.Count; i++) { @@ -517,11 +513,11 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe var parameter = pi[paramIndex]; // Clr parameter we are targeting object arg; // Python -> Clr argument - var hasNamedParam = kwArgDict == null ? false : kwArgDict.TryGetValue(paramNames[paramIndex], out tempPyObject); - // Check positional arguments first and then check for named arguments and optional values if (paramIndex >= pyArgCount) { + var hasNamedParam = kwArgDict == null ? false : kwArgDict.TryGetValue(paramNames[paramIndex], out tempPyObject); + // All positional arguments have been used: // Check our KWargs for this parameter if (hasNamedParam) @@ -698,18 +694,26 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe } var match = new MatchedMethod(kwargsMatched, margs, outs, mi); - if (usedImplicitConversion) + // Only add matches using implicit conversion if no other regular matches were found, + // since we favor regular matches over matches using implicit conversion + if (usedImplicitConversion && matches.Count == 0) { + if (matchesUsingImplicitConversion == null) + { + matchesUsingImplicitConversion = new List(); + } matchesUsingImplicitConversion.Add(match); } else { matches.Add(match); + // We don't need the matches using implicit conversion anymore + matchesUsingImplicitConversion = null; } } } - if (matches.Count > 0 || matchesUsingImplicitConversion.Count > 0) + if (matches.Count > 0 || (matchesUsingImplicitConversion != null && matchesUsingImplicitConversion.Count > 0)) { // We favor matches that do not use implicit conversion var matchesTouse = matches.Count > 0 ? matches : matchesUsingImplicitConversion; From 5736d515224881be79fc8c9289137adbdc08a952 Mon Sep 17 00:00:00 2001 From: Jhonathan Abreu Date: Tue, 14 May 2024 13:57:17 -0400 Subject: [PATCH 6/7] Minor improvement --- src/runtime/MethodBinder.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/runtime/MethodBinder.cs b/src/runtime/MethodBinder.cs index c81ef35e4..6ed522fb4 100644 --- a/src/runtime/MethodBinder.cs +++ b/src/runtime/MethodBinder.cs @@ -609,7 +609,9 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe typematch = true; clrtype = parameter.ParameterType; } - else + // we won't take matches using implicit conversions if there is already a match + // not using implicit conversions + else if (matches.Count == 0) { // accepts non-decimal numbers in decimal parameters if (underlyingType == typeof(decimal)) From 65ad1b9e4a9cdcac6c3e3890e022871929240e49 Mon Sep 17 00:00:00 2001 From: Jhonathan Abreu Date: Tue, 14 May 2024 15:57:21 -0400 Subject: [PATCH 7/7] Minor change --- src/runtime/MethodBinder.cs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/runtime/MethodBinder.cs b/src/runtime/MethodBinder.cs index 6ed522fb4..bef394ba7 100644 --- a/src/runtime/MethodBinder.cs +++ b/src/runtime/MethodBinder.cs @@ -696,9 +696,7 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe } var match = new MatchedMethod(kwargsMatched, margs, outs, mi); - // Only add matches using implicit conversion if no other regular matches were found, - // since we favor regular matches over matches using implicit conversion - if (usedImplicitConversion && matches.Count == 0) + if (usedImplicitConversion) { if (matchesUsingImplicitConversion == null) { @@ -709,7 +707,7 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe else { matches.Add(match); - // We don't need the matches using implicit conversion anymore + // We don't need the matches using implicit conversion anymore, we can free the memory matchesUsingImplicitConversion = null; } }