Skip to content

Commit 6ecae97

Browse files
jhonabreuljhonabreul
committed
Fixes: matching method overloads with named arguments
1 parent 4179ce3 commit 6ecae97

File tree

3 files changed

+175
-40
lines changed

3 files changed

+175
-40
lines changed

‎src/embed_tests/TestMethodBinder.cs‎

Lines changed: 92 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -740,6 +740,98 @@ from Python.EmbeddingTest import *
740740
"));
741741
}
742742

743+
public class OverloadsTestClass
744+
{
745+
746+
public string Method1(string positionalArg, decimal namedArg1 = 1.2m, int namedArg2 = 123)
747+
{
748+
Console.WriteLine("1");
749+
return "Method1 Overload 1";
750+
}
751+
752+
public string Method1(decimal namedArg1 = 1.2m, int namedArg2 = 123)
753+
{
754+
Console.WriteLine("2");
755+
return "Method1 Overload 2";
756+
}
757+
758+
// ----
759+
760+
public string Method2(string arg1, int arg2, float arg3, float kwarg1 = 1.1f, bool kwarg2 = false, string kwarg3 = "")
761+
{
762+
return "Method2 Overload 1";
763+
}
764+
765+
public string Method2(string arg1, int arg2, float kwarg1 = 1.1f, bool kwarg2 = false, string kwarg3 = "")
766+
{
767+
return "Method2 Overload 2";
768+
}
769+
770+
// ----
771+
772+
public string ImplicitConversionSameArgumentCount(string symbol, int quantity, float trailingAmount, bool trailingAsPercentage, string tag = "")
773+
{
774+
return "ImplicitConversionSameArgumentCount 1";
775+
}
776+
777+
public string ImplicitConversionSameArgumentCount(string symbol, decimal quantity, decimal trailingAmount, bool trailingAsPercentage, string tag = "")
778+
{
779+
return "ImplicitConversionSameArgumentCount 2";
780+
}
781+
782+
public string ImplicitConversionSameArgumentCount2(string symbol, int quantity, float trailingAmount, bool trailingAsPercentage, string tag = "")
783+
{
784+
return "ImplicitConversionSameArgumentCount2 1";
785+
}
786+
787+
public string ImplicitConversionSameArgumentCount2(string symbol, float quantity, float trailingAmount, bool trailingAsPercentage, string tag = "")
788+
{
789+
return "ImplicitConversionSameArgumentCount2 2";
790+
}
791+
792+
public string ImplicitConversionSameArgumentCount2(string symbol, decimal quantity, float trailingAmount, bool trailingAsPercentage, string tag = "")
793+
{
794+
return "ImplicitConversionSameArgumentCount2 2";
795+
}
796+
}
797+
798+
[TestCase("Method1('abc', namedArg1=1.234, namedArg2=321)", "Method1 Overload 1")]
799+
[TestCase("Method2(\"SPY\", 10, 123.4, kwarg1=0.0025, kwarg2=True)", "Method2 Overload 1")]
800+
public void SelectsRightOverloadWithNamedParameters(string methodCallCode, string expectedResult)
801+
{
802+
using var _ = Py.GIL();
803+
804+
dynamic module = PyModule.FromString("SelectsRightOverloadWithNamedParameters", @$"
805+
806+
def call_method(instance):
807+
return instance.{methodCallCode}
808+
");
809+
810+
var instance = new OverloadsTestClass();
811+
var result = module.call_method(instance).As<string>();
812+
813+
Assert.AreEqual(expectedResult, result);
814+
}
815+
816+
[TestCase("ImplicitConversionSameArgumentCount", "10", "ImplicitConversionSameArgumentCount 1")]
817+
[TestCase("ImplicitConversionSameArgumentCount", "10.1", "ImplicitConversionSameArgumentCount 2")]
818+
[TestCase("ImplicitConversionSameArgumentCount2", "10", "ImplicitConversionSameArgumentCount2 1")]
819+
[TestCase("ImplicitConversionSameArgumentCount2", "10.1", "ImplicitConversionSameArgumentCount2 2")]
820+
public void DisambiguatesOverloadWithSameArgumentCountAndImplicitConversion(string methodName, string quantity, string expectedResult)
821+
{
822+
using var _ = Py.GIL();
823+
824+
dynamic module = PyModule.FromString("DisambiguatesOverloadWithSameArgumentCountAndImplicitConversion", @$"
825+
def call_method(instance):
826+
return instance.{methodName}(""SPY"", {quantity}, 123.4, trailingAsPercentage=True)
827+
");
828+
829+
var instance = new OverloadsTestClass();
830+
var result = module.call_method(instance).As<string>();
831+
832+
Assert.AreEqual(expectedResult, result);
833+
}
834+
743835
public class CSharpClass
744836
{
745837
public string CalledMethodMessage { get; private set; }

‎src/runtime/Converter.cs‎

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -851,8 +851,21 @@ internal static bool ToPrimitive(BorrowedReference value, Type obType, out objec
851851
}
852852

853853
case TypeCode.Boolean:
854-
result = Runtime.PyObject_IsTrue(value) != 0;
854+
if (value == Runtime.PyTrue)
855+
{
856+
result = true;
857+
return true;
858+
}
859+
if (value == Runtime.PyFalse)
860+
{
861+
result = false;
855862
return true;
863+
}
864+
if (setError)
865+
{
866+
goto type_error;
867+
}
868+
return false;
856869

857870
case TypeCode.Byte:
858871
{

‎src/runtime/MethodBinder.cs‎

Lines changed: 69 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,9 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe
456456
var methods = info == null ? GetMethods()
457457
: new List<MethodInformation>(1) { new MethodInformation(info, true) };
458458

459+
var matches = new List<MatchedMethod>();
460+
var matchesUsingImplicitConversion = new List<MatchedMethod>();
461+
459462
for (var i = 0; i < methods.Count; i++)
460463
{
461464
var methodInformation = methods[i];
@@ -504,6 +507,8 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe
504507

505508
int paramsArrayIndex = paramsArray ? pi.Length - 1 : -1; // -1 indicates no paramsArray
506509
var usedImplicitConversion = false;
510+
var kwargsMatched = 0;
511+
var defaultsNeeded = 0;
507512

508513
// Conversion loop for each parameter
509514
for (int paramIndex = 0; paramIndex < clrArgCount; paramIndex++)
@@ -513,25 +518,34 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe
513518
var parameter = pi[paramIndex]; // Clr parameter we are targeting
514519
object arg; // Python -> Clr argument
515520

521+
var hasNamedParam = kwArgDict == null ? false : kwArgDict.TryGetValue(paramNames[paramIndex], out tempPyObject);
522+
523+
// Check positional arguments first and then check for named arguments and optional values
524+
if (paramIndex >= pyArgCount)
525+
{
526+
// All positional arguments have been used:
516527
// Check our KWargs for this parameter
517-
bool hasNamedParam = kwArgDict == null ? false : kwArgDict.TryGetValue(paramNames[paramIndex], out tempPyObject);
528+
if (hasNamedParam)
529+
{
530+
kwargsMatched++;
518531
if (tempPyObject != null)
519532
{
520533
op = tempPyObject;
521534
}
522-
523-
NewReference tempObject = default;
524-
525-
// Check if we are going to use default
526-
if (paramIndex >= pyArgCount && !(hasNamedParam || (paramsArray && paramIndex == paramsArrayIndex)))
535+
}
536+
else if (parameter.IsOptional && !(hasNamedParam || (paramsArray && paramIndex == paramsArrayIndex)))
527537
{
538+
defaultsNeeded++;
528539
if (defaultArgList != null)
529540
{
530541
margs[paramIndex] = defaultArgList[paramIndex - pyArgCount];
531542
}
532543

533544
continue;
534545
}
546+
}
547+
548+
NewReference tempObject = default;
535549

536550
// At this point, if op is IntPtr.Zero we don't have a KWArg and are not using default
537551
if (op == null)
@@ -601,9 +615,7 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe
601615
typematch = true;
602616
clrtype = parameter.ParameterType;
603617
}
604-
// lets just keep the first binding using implicit conversion
605-
// this is to respect method order/precedence
606-
else if (bindingUsingImplicitConversion == null)
618+
else
607619
{
608620
// accepts non-decimal numbers in decimal parameters
609621
if (underlyingType == typeof(decimal))
@@ -687,58 +699,59 @@ internal Binding Bind(BorrowedReference inst, BorrowedReference args, BorrowedRe
687699
}
688700
}
689701

690-
object target = null;
702+
var match = new MatchedMethod(kwargsMatched, margs, outs, mi);
703+
if (usedImplicitConversion)
704+
{
705+
matchesUsingImplicitConversion.Add(match);
706+
}
707+
else
708+
{
709+
matches.Add(match);
710+
}
711+
}
712+
}
713+
714+
if (matches.Count > 0 || matchesUsingImplicitConversion.Count > 0)
715+
{
716+
// We favor matches that do not use implicit conversion
717+
var matchesTouse = matches.Count > 0 ? matches : matchesUsingImplicitConversion;
718+
719+
// The best match would be the one with the most named arguments matched
720+
var bestMatch = matchesTouse.MaxBy(x => x.KwargsMatched);
721+
var margs = bestMatch.ManagedArgs;
722+
var outs = bestMatch.Outs;
723+
var mi = bestMatch.Method;
724+
725+
object? target = null;
691726
if (!mi.IsStatic && inst != null)
692727
{
693728
//CLRObject co = (CLRObject)ManagedType.GetManagedObject(inst);
694729
// InvalidCastException: Unable to cast object of type
695730
// 'Python.Runtime.ClassObject' to type 'Python.Runtime.CLRObject'
696-
var co = ManagedType.GetManagedObject(inst) as CLRObject;
697731

698732
// Sanity check: this ensures a graceful exit if someone does
699733
// something intentionally wrong like call a non-static method
700734
// on the class rather than on an instance of the class.
701735
// XXX maybe better to do this before all the other rigmarole.
702-
if (co == null)
736+
if (ManagedType.GetManagedObject(inst) is CLRObject co)
737+
{
738+
target = co.inst;
739+
}
740+
else
703741
{
742+
Exceptions.SetError(Exceptions.TypeError, "Invoked a non-static method with an invalid instance");
704743
return null;
705744
}
706-
target = co.inst;
707745
}
708746

709747
// If this match is generic we need to resolve it with our types.
710748
// Store this generic match to be used if no others match
711749
if (mi.IsGenericMethod)
712750
{
713751
mi = ResolveGenericMethod((MethodInfo)mi, margs);
714-
genericBinding = new Binding(mi, target, margs, outs);
715-
continue;
716-
}
717-
718-
var binding = new Binding(mi, target, margs, outs);
719-
if (usedImplicitConversion)
720-
{
721-
// in this case we will not return the binding yet in case there is a match
722-
// which does not use implicit conversions, which will return directly
723-
bindingUsingImplicitConversion = binding;
724-
}
725-
else
726-
{
727-
return binding;
728-
}
729-
}
730-
}
731-
732-
// if we generated a binding using implicit conversion return it
733-
if (bindingUsingImplicitConversion != null)
734-
{
735-
return bindingUsingImplicitConversion;
736752
}
737753

738-
// if we generated a generic binding, return it
739-
if (genericBinding != null)
740-
{
741-
return genericBinding;
754+
return new Binding(mi, target, margs, outs);
742755
}
743756

744757
return null;
@@ -1063,6 +1076,23 @@ public int Compare(MethodInformation x, MethodInformation y)
10631076
return 0;
10641077
}
10651078
}
1079+
1080+
private readonly struct MatchedMethod
1081+
{
1082+
public int KwargsMatched { get; }
1083+
public object?[] ManagedArgs { get; }
1084+
public int Outs { get; }
1085+
public MethodBase Method { get; }
1086+
1087+
public MatchedMethod(int kwargsMatched, object?[] margs, int outs, MethodBase mb)
1088+
{
1089+
KwargsMatched = kwargsMatched;
1090+
ManagedArgs = margs;
1091+
Outs = outs;
1092+
Method = mb;
1093+
}
1094+
}
1095+
10661096
protected static void AppendArgumentTypes(StringBuilder to, BorrowedReference args)
10671097
{
10681098
long argCount = Runtime.PyTuple_Size(args);

0 commit comments

Comments
 (0)