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..e377b5f83 100644 --- a/src/embed_tests/TestMethodBinder.cs +++ b/src/embed_tests/TestMethodBinder.cs @@ -740,6 +740,164 @@ 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, decimal arg3, decimal kwarg1 = 1.1m, bool kwarg2 = false, string kwarg3 = "") + { + return "Method2 Overload 1"; + } + + 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"; + } + + 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=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(); + + 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; } + + 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/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/Converter.cs b/src/runtime/Converter.cs index 2783f0037..047f7a03a 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; @@ -845,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..bef394ba7 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,6 +452,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(methods.Count); + List matchesUsingImplicitConversion = null; + for (var i = 0; i < methods.Count; i++) { var methodInformation = methods[i]; @@ -504,6 +503,7 @@ 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; // Conversion loop for each parameter for (int paramIndex = 0; paramIndex < clrArgCount; paramIndex++) @@ -513,26 +513,34 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe var parameter = pi[paramIndex]; // Clr parameter we are targeting object arg; // Python -> Clr argument - // Check our KWargs for this parameter - bool hasNamedParam = kwArgDict == null ? false : kwArgDict.TryGetValue(paramNames[paramIndex], out tempPyObject); - if (tempPyObject != null) + // Check positional arguments first and then check for named arguments and optional values + if (paramIndex >= pyArgCount) { - op = tempPyObject; - } - - NewReference tempObject = default; + var hasNamedParam = kwArgDict == null ? false : kwArgDict.TryGetValue(paramNames[paramIndex], out tempPyObject); - // Check if we are going to use default - if (paramIndex >= pyArgCount && !(hasNamedParam || (paramsArray && paramIndex == paramsArrayIndex))) - { - if (defaultArgList != null) + // All positional arguments have been used: + // Check our KWargs for this parameter + if (hasNamedParam) { - margs[paramIndex] = defaultArgList[paramIndex - pyArgCount]; + kwargsMatched++; + if (tempPyObject != null) + { + op = tempPyObject; + } } + else if (parameter.IsOptional && !(hasNamedParam || (paramsArray && paramIndex == paramsArrayIndex))) + { + if (defaultArgList != null) + { + margs[paramIndex] = defaultArgList[paramIndex - pyArgCount]; + } - continue; + 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 +609,9 @@ 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) + // 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)) @@ -687,58 +695,65 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe } } - object target = null; - if (!mi.IsStatic && inst != null) + var match = new MatchedMethod(kwargsMatched, margs, outs, mi); + if (usedImplicitConversion) { - //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 (matchesUsingImplicitConversion == null) { - return null; + matchesUsingImplicitConversion = new List(); } - target = co.inst; + matchesUsingImplicitConversion.Add(match); } - - // 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) + else { - mi = ResolveGenericMethod((MethodInfo)mi, margs); - genericBinding = new Binding(mi, target, margs, outs); - continue; + matches.Add(match); + // We don't need the matches using implicit conversion anymore, we can free the memory + matchesUsingImplicitConversion = null; } + } + } - var binding = new Binding(mi, target, margs, outs); - if (usedImplicitConversion) + 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; + + // 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' + + // 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) { - // 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; + target = co.inst; } else { - return binding; + Exceptions.SetError(Exceptions.TypeError, "Invoked a non-static method with an invalid instance"); + return null; } } - } - // if we generated a binding using implicit conversion return it - if (bindingUsingImplicitConversion != null) - { - return bindingUsingImplicitConversion; - } + // 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 we generated a generic binding, return it - if (genericBinding != null) - { - return genericBinding; + return new Binding(mi, target, margs, outs); } return null; @@ -1063,6 +1078,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); 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