Fix kwarg func resolution by jmlidbetter · Pull Request #1136 · pythonnet/pythonnet · GitHub

What does this implement/fix? Explain your changes.

A first run at fixing issue #1097. Currently, the method for selecting which method to use is quite simple and just takes the first function it finds to match. The problem is that the current picking algorithm has no knowledge of kwargs and how many have been supplied or used, i.e. MatchesArgumentCount will return true even if we've supplied some kwargs but the current overload does not require them. Ultimately, this means the first matched method might not be the best.

This small patch aims to rectify this issue by collecting together all the methods for which MatchesArgumentCount returns true and then from these selecting the best match based on the following criteria

Maximizing the number of supplied kwargs that have been used
Minimizing the number of default arguments needed

It also has some logic to abort in the case where

The above criteria are met by more than one overload
We need at least one default argument to make the call

This stops users in python making ambiguous function calls - see the tests for an example.

Does this close any currently open issues?

#1097

Any other comments?

Feedback please!

Checklist

Check all those that are applicable and complete.

Make sure to include one or more tests for your change
If an enhancement PR, please create docs and at best an example
Add yourself to AUTHORS
Updated the CHANGELOG

All CLA requirements met.

Just to note that we have been using this fix for some time and it does appear to work. It is a welcome change from needing to specify long list of args you don't care about.

Just FYI, there's a much better way to do method resolution (which I hope we will adopt at some point) but it requires lots of rework: C# Binder. I did a simple experiment here: losttech@605c576

We can still take this change in meanwhile.

Can you make named structs from the tuples? It's hard to read with Item1-like references.

Just FYI, there's a much better way to do method resolution (which I hope we will adopt at some point) but it requires lots of rework: C# Binder. I did a simple experiment here: losttech@605c576

We can still take this change in meanwhile.

Yes this wasn't supposed to change the world, simply just to get something that worked 😄

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

Please use Count property. Count() method might require a run through the entire collection.

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.

Approved, but should wait for post 2.x branch due to the breaking change in behavior around overload resolution ordering.

@jmlidbetter we have released 2.5 and master is now working on 3.x. Can you update this PR, so we could merge it?

@jmlidbetter we have released 2.5 and master is now working on 3.x. Can you update this PR, so we could merge it?

Sure. I assume just rebasing on top of master is sufficient or is there something else you'd want?

@jmlidbetter it should be. I'll re-review just in case.

…st matched on defaults

@lostmsu Ok I have rebased, feel free to check and merge (as appropriate)

@jmlidbetter related tests seem to be failing.

Hmm that is strange - when I ran the tests on my machine they all passed.... I will have a look tomorrow.

Seems like the Linux tests have passed ok but are broken on Windows - perhaps there is some slight difference in behaviour on windows. I have limited ability test on windows but I'll see what I can do.

@lostmsu I have not been able to reproduce the test failures. Building on Windows I get the following output:

Are you able to repro this issue on windows?

I am not sure what causes the discrepancy, but judging by the code, it seems like the CI build messes up with named parameters. I will try to see if that is correct.

However, this also uncovers a bug: if Python passed a named parameter not defined in C#, the overload resolution behavior is incorrect. I suggest to handle that and add a test too.

void NoOverloads(int a = 0) {}

NoOverloads(c=1) # should fail, because c does not match a

I believe I had originally implemented that behaviour (not submitted onto this branch) but it broke some other tests. I may be misremembering though, I will check tomorrow.

@lostmsu here is the breaking test if you fail when a kwarg is supplied that doesn't appear in the method signature:

____________________________________________ test_namespace_and_init _____________________________________________

    def test_namespace_and_init():
        calls = []
        class TestX(System.Object):
            __namespace__ = "test_clr_subclass_with_init_args"
            def __init__(self, *args, **kwargs):
                calls.append((args, kwargs))
>       t = TestX(1,2,3,foo="bar")
E       TypeError: No constructor matches given arguments: (<class 'int'>, <class 'int'>, <class 'int'>)

src/tests/test_subclass.py:229: TypeError

I haven't done any further investigation into this. I guess there are two ways to proceed:
(1) Log this as an issue and proceed as is
(2) Dig in and fix the broken test

Its up to you, but personally I'm in favour of (1) because (a) having working kwargs (except in the case where you supply extra ones ;) ) is quite a big improvement and (b) python is fairly loose anyway with ducktyping etc etc, in a way all C# methods would have an implicit **kwargs in their signature - it might be possible to throw a warning when extraneous kwargs are given, I don't know.

Are we good to merge this change once the tests have passed?

This looks good to me, @lostmsu are you also fine with merging this?

Codecov Report

Merging #1136 (8d0fc2c) into master (7870a9f) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1136   +/-   ##
=======================================
  Coverage   87.97%   87.97%           
=======================================
  Files           1        1           
  Lines         291      291           
=======================================
  Hits          256      256           
  Misses         35       35

Flag	Coverage Δ
setup_linux	65.29% <ø> (ø)
setup_windows	74.22% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7870a9f...8d0fc2c. Read the comment docs.

lostmsu requested changes May 7, 2020

View reviewed changes

lostmsu requested changes May 11, 2020

View reviewed changes

Comment thread src/runtime/methodbinder.cs Outdated

Comment thread src/runtime/methodbinder.cs Outdated

Comment thread src/runtime/methodbinder.cs Outdated

jmlidbetter force-pushed the fix_kwarg_func_resolution branch from ce07727 to 2b98872 Compare May 12, 2020 08:17

jmlidbetter commented May 12, 2020

View reviewed changes

lostmsu requested changes May 12, 2020

View reviewed changes

jmlidbetter force-pushed the fix_kwarg_func_resolution branch from 643f7bc to 1dd8725 Compare May 13, 2020 06:33

lostmsu approved these changes May 13, 2020

View reviewed changes

lostmsu added this to the 3.0.0 milestone May 13, 2020

jmlidbetter added 5 commits June 22, 2020 16:04

Fix function resolution when calling overloads with keyword arguments

9fb753a

Updates CHANGELOG.md

fee4373

Switches Tuple for struct

8581715

Use LINQ Min and Max methods; don't create second list for methods be…

86d21f1

…st matched on defaults

Removes all secondary lists

d453159

jmlidbetter force-pushed the fix_kwarg_func_resolution branch from 7caa7ed to d453159 Compare June 22, 2020 15:21

Updates CHANGELOG

a1b6c76

Merge branch 'master' into fix_kwarg_func_resolution

1d01c45

Merge branch 'master' into fix_kwarg_func_resolution

980c231

Merge branch 'master' into fix_kwarg_func_resolution

8d0fc2c

lostmsu merged commit c81c3c3 into pythonnet:master Nov 18, 2020

+                              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;
+                              }

+                              // Order by the number of defaults required and find the smallest
+                              var fewestDefaultsRequired  = bestKwargMatches.OrderBy((x) => x.Item2).ToArray()[0].Item2;
+                              List<Tuple<int, int, object[], int, MethodBase>> bestDefaultsMatches =

+                                  if (testMatch.DefaultsNeeded == fewestDefaultsRequired)
+                                  {
+                                      bestCount++;
+                                      if (bestMatchIndex == -1)

+                              var matchedMethod = new MatchedMethod(kwargsMatched, defaultsNeeded, margs, outs, mi);
+                              argMatchedMethods.Add(matchedMethod);
+                          }
+                          if (argMatchedMethods.Count() > 0)

Fix kwarg func resolution by jmlidbetter · Pull Request #1136 · pythonnet/pythonnet · GitHub

Uh oh!

Fix kwarg func resolution - #1136

Fix kwarg func resolution#1136lostmsu merged 9 commits intopythonnet:masterpythonnet/pythonnet:masterfrom jmlidbetter:fix_kwarg_func_resolutionjmlidbetter/pythonnet:fix_kwarg_func_resolutionCopy head branch name to clipboard

Conversation

jmlidbetter commented May 7, 2020

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

Uh oh!

dnfclas commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrNickClarke commented May 7, 2020

Uh oh!

lostmsu left a comment

Choose a reason for hiding this comment

Uh oh!

lostmsu May 7, 2020

Choose a reason for hiding this comment

Uh oh!

jmlidbetter commented May 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmlidbetter May 12, 2020

Choose a reason for hiding this comment

Uh oh!

lostmsu May 12, 2020

Choose a reason for hiding this comment

Uh oh!

lostmsu May 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

lostmsu left a comment

Choose a reason for hiding this comment

Uh oh!

lostmsu commented Jun 19, 2020

Uh oh!

jmlidbetter commented Jun 19, 2020

Uh oh!

lostmsu commented Jun 20, 2020

Uh oh!

jmlidbetter commented Jun 22, 2020

Uh oh!

lostmsu commented Jun 22, 2020

Uh oh!

jmlidbetter commented Jun 22, 2020

Uh oh!

jmlidbetter commented Jun 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmlidbetter commented Jun 23, 2020

Uh oh!

lostmsu commented Jun 23, 2020

Uh oh!

jmlidbetter commented Jun 23, 2020

Uh oh!

jmlidbetter commented Jun 25, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jmlidbetter commented Oct 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

filmor commented Nov 18, 2020

Uh oh!

codecov-io commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Reviewers

Assignees

Labels

Fix kwarg func resolution#1136
lostmsu merged 9 commits into
pythonnet:masterfrom
jmlidbetter:fix_kwarg_func_resolution

dnfclas commented May 7, 2020 •
edited
Loading

lostmsu May 12, 2020 •
edited
Loading

jmlidbetter commented Jun 22, 2020 •
edited
Loading

jmlidbetter commented Jun 25, 2020 •
edited
Loading

jmlidbetter commented Oct 21, 2020 •
edited
Loading

codecov-io commented Nov 18, 2020 •
edited
Loading