- Notifications
You must be signed in to change notification settings - Fork749
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Merged
Uh oh!
There was an error while loading.Please reload this page.
Merged
Changes fromall commits
Commits
Show all changes
9 commits Select commitHold shift + click to select a range
9fb753a Fix function resolution when calling overloads with keyword arguments
jmlidbetterfee4373 Updates CHANGELOG.md
jmlidbetter8581715 Switches Tuple for struct
jmlidbetter86d21f1 Use LINQ Min and Max methods; don't create second list for methods be…
jmlidbetterd453159 Removes all secondary lists
jmlidbettera1b6c76 Updates CHANGELOG
jmlidbetter1d01c45 Merge branch 'master' into fix_kwarg_func_resolution
filmor980c231 Merge branch 'master' into fix_kwarg_func_resolution
jmlidbetter8d0fc2c Merge branch 'master' into fix_kwarg_func_resolution
filmorFile filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading.Please reload this page.
Diff view
Diff view
There are no files selected for viewing
1 change: 1 addition & 0 deletionsCHANGELOG.md
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
73 changes: 71 additions & 2 deletionssrc/runtime/methodbinder.cs
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, 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) | ||
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. 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 Member 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 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) | ||
| { | ||
20 changes: 19 additions & 1 deletionsrc/testing/methodtest.cs
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
29 changes: 29 additions & 0 deletionssrc/tests/test_method.py
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.