-
Notifications
You must be signed in to change notification settings - Fork 752
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
lostmsu
merged 9 commits into
pythonnet:master
from
jmlidbetter:fix_kwarg_func_resolution
Nov 18, 2020
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
9fb753a
Fix function resolution when calling overloads with keyword arguments
jmlidbetter fee4373
Updates CHANGELOG.md
jmlidbetter 8581715
Switches Tuple for struct
jmlidbetter 86d21f1
Use LINQ Min and Max methods; don't create second list for methods be…
jmlidbetter d453159
Removes all secondary lists
jmlidbetter a1b6c76
Updates CHANGELOG
jmlidbetter 1d01c45
Merge branch 'master' into fix_kwarg_func_resolution
filmor 980c231
Merge branch 'master' into fix_kwarg_func_resolution
jmlidbetter 8d0fc2c
Merge branch 'master' into fix_kwarg_func_resolution
filmor File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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) | ||
{ | ||
|
@@ -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; | ||
} | ||
|
@@ -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) | ||
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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) | ||
{ | ||
|
@@ -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) | ||
{ | ||
|
@@ -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) | ||
{ | ||
|
@@ -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) | ||
{ | ||
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
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