Skip to content

Fix kwarg func resolution #1136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Nov 18, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ details about the cause of the failure

- Fix incorrect dereference of wrapper object in `tp_repr`, which may result in a program crash
- Fix incorrect dereference in params array handling
- Fixes issue with function resolution when calling overloaded function with keyword arguments from python ([#1097][i1097])
- Fix `object[]` parameters taking precedence when should not in overload resolution
- Fixed a bug where all .NET class instances were considered Iterable
- Fix incorrect choice of method to invoke when using keyword arguments.
Expand Down
73 changes: 71 additions & 2 deletions src/runtime/methodbinder.cs
Original file line number Diff line number Diff line change
Expand Up @@ -279,6 +279,23 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info)
return Bind(inst, args, kw, info, null);
}

private readonly struct MatchedMethod {
public MatchedMethod(int kwargsMatched, int defaultsNeeded, object[] margs, int outs, MethodBase mb)
{
KwargsMatched = kwargsMatched;
DefaultsNeeded = defaultsNeeded;
ManagedArgs = margs;
Outs = outs;
Method = mb;
}

public int KwargsMatched { get; }
public int DefaultsNeeded { get; }
public object[] ManagedArgs { get; }
public int Outs { get; }
public MethodBase Method { get; }
}

internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info, MethodInfo[] methodinfo)
{
// loop to find match, return invoker w/ or /wo error
Expand Down Expand Up @@ -311,6 +328,8 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info, Meth
_methods = GetMethods();
}

var argMatchedMethods = new List<MatchedMethod>(_methods.Length);

// TODO: Clean up
foreach (MethodBase mi in _methods)
{
Expand All @@ -321,8 +340,10 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info, Meth
ParameterInfo[] pi = mi.GetParameters();
ArrayList defaultArgList;
bool paramsArray;
int kwargsMatched;
int defaultsNeeded;

if (!MatchesArgumentCount(pynargs, pi, kwargDict, out paramsArray, out defaultArgList))
if (!MatchesArgumentCount(pynargs, pi, kwargDict, out paramsArray, out defaultArgList, out kwargsMatched, out defaultsNeeded))
{
continue;
}
Expand All @@ -336,6 +357,47 @@ internal Binding Bind(IntPtr inst, IntPtr args, IntPtr kw, MethodBase info, Meth
continue;
}

var matchedMethod = new MatchedMethod(kwargsMatched, defaultsNeeded, margs, outs, mi);
argMatchedMethods.Add(matchedMethod);
}
if (argMatchedMethods.Count > 0)
{
var bestKwargMatchCount = argMatchedMethods.Max(x => x.KwargsMatched);
var fewestDefaultsRequired = argMatchedMethods.Where(x => x.KwargsMatched == bestKwargMatchCount).Min(x => x.DefaultsNeeded);

int bestCount = 0;
int bestMatchIndex = -1;

for (int index = 0; index < argMatchedMethods.Count; index++)
{
var testMatch = argMatchedMethods[index];
if (testMatch.DefaultsNeeded == fewestDefaultsRequired && testMatch.KwargsMatched == bestKwargMatchCount)
{
bestCount++;
if (bestMatchIndex == -1)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not having this check will cause tests to fail because a different overload is picked- e.g. MethodTest.TestOverloadedObjectTwo(5, 5) will get the (Object, Object) overload rather than the (Int, Int) overload

bestMatchIndex = index;
}
}

if (bestCount > 1 && fewestDefaultsRequired > 0)
{
// Best effort for determining method to match on gives multiple possible
// matches and we need at least one default argument - bail from this point
return null;
}
Comment on lines +382 to +387
Copy link
Member

@lostmsu lostmsu May 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this makes the change breaking for Python.NET 2.x branch. Currently the first match is returned irrespective if there are other matches. No need to fix it, just means might have to be merged later.


// If we're here either:
// (a) There is only one best match
// (b) There are multiple best matches but none of them require
// default arguments
// in the case of (a) we're done by default. For (b) regardless of which
// method we choose, all arguments are specified _and_ can be converted
// from python to C# so picking any will suffice
MatchedMethod bestMatch = argMatchedMethods[bestMatchIndex];
var margs = bestMatch.ManagedArgs;
var outs = bestMatch.Outs;
var mi = bestMatch.Method;

object target = null;
if (!mi.IsStatic && inst != IntPtr.Zero)
{
Expand Down Expand Up @@ -575,11 +637,16 @@ static Type TryComputeClrArgumentType(Type parameterType, IntPtr argument, bool
static bool MatchesArgumentCount(int positionalArgumentCount, ParameterInfo[] parameters,
Dictionary<string, IntPtr> kwargDict,
out bool paramsArray,
out ArrayList defaultArgList)
out ArrayList defaultArgList,
out int kwargsMatched,
out int defaultsNeeded)
{
defaultArgList = null;
var match = false;
paramsArray = parameters.Length > 0 ? Attribute.IsDefined(parameters[parameters.Length - 1], typeof(ParamArrayAttribute)) : false;
var kwargCount = kwargDict.Count;
kwargsMatched = 0;
defaultsNeeded = 0;

if (positionalArgumentCount == parameters.Length && kwargDict.Count == 0)
{
Expand All @@ -599,6 +666,7 @@ static bool MatchesArgumentCount(int positionalArgumentCount, ParameterInfo[] pa
// no need to check for a default parameter, but put a null
// placeholder in defaultArgList
defaultArgList.Add(null);
kwargsMatched++;
}
else if (parameters[v].IsOptional)
{
Expand All @@ -607,6 +675,7 @@ static bool MatchesArgumentCount(int positionalArgumentCount, ParameterInfo[] pa
// The GetDefaultValue() extension method will return the value
// to be passed in as the parameter value
defaultArgList.Add(parameters[v].GetDefaultValue());
defaultsNeeded++;
}
else if(!paramsArray)
{
Expand Down
20 changes: 19 additions & 1 deletion src/testing/methodtest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -684,7 +684,25 @@ public static string OptionalAndDefaultParams2([Optional]int a, [Optional]int b,
return string.Format("{0}{1}{2}{3}", a, b, c, d);
}


public static string DefaultParamsWithOverloading(int a = 2, int b = 1)
{
return $"{a}{b}";
}

public static string DefaultParamsWithOverloading(string a = "a", string b = "b")
{
return $"{a}{b}X";
}

public static string DefaultParamsWithOverloading(int a = 0, int b = 1, int c = 2)
{
return $"{a}{b}{c}XX";
}

public static string DefaultParamsWithOverloading(int a = 5, int b = 6, int c = 7, int d = 8)
{
return $"{a}{b}{c}{d}XXX";
}
}


Expand Down
29 changes: 29 additions & 0 deletions src/tests/test_method.py
Original file line number Diff line number Diff line change
Expand Up @@ -1153,6 +1153,35 @@ def test_optional_and_default_params():
res = MethodTest.OptionalAndDefaultParams2(b=2, c=3)
assert res == "0232"

def test_default_params_overloads():
res = MethodTest.DefaultParamsWithOverloading(1, 2)
assert res == "12"

res = MethodTest.DefaultParamsWithOverloading(b=5)
assert res == "25"

res = MethodTest.DefaultParamsWithOverloading("d")
assert res == "dbX"

res = MethodTest.DefaultParamsWithOverloading(b="c")
assert res == "acX"

res = MethodTest.DefaultParamsWithOverloading(c=3)
assert res == "013XX"

res = MethodTest.DefaultParamsWithOverloading(5, c=2)
assert res == "512XX"

res = MethodTest.DefaultParamsWithOverloading(c=0, d=1)
assert res == "5601XXX"

res = MethodTest.DefaultParamsWithOverloading(1, d=1)
assert res == "1671XXX"

def test_default_params_overloads_ambiguous_call():
with pytest.raises(TypeError):
MethodTest.DefaultParamsWithOverloading()

def test_keyword_arg_method_resolution():
from Python.Test import MethodArityTest

Expand Down