NotificationsYou must be signed in to change notification settings
Fork748
Star5.1k

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

Jump to bottom

Merged

lostmsu merged 9 commits intopythonnet:masterfromjmlidbetter:fix_kwarg_func_resolution

Nov 18, 2020

Merged

Fix kwarg func resolution#1136

lostmsu merged 9 commits intopythonnet:masterfromjmlidbetter:fix_kwarg_func_resolution

Nov 18, 2020

Conversation

Copy link

Contributor

jmlidbetter commentedMay 7, 2020

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 toAUTHORS
Updated theCHANGELOG

Copy link

dnfclas commentedMay 7, 2020•
edited
Loading

All CLA requirements met.

Copy link

DrNickClarke commentedMay 7, 2020

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.

lostmsu requested changes

May 7, 2020

View reviewed changes

Copy link

Member

lostmsu left a comment

Choose a reason for hiding this comment

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

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.

src/runtime/methodbinder.cs Outdated

		// 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 =

Copy link

Member

lostmsuMay 7, 2020

Choose a reason for hiding this comment

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

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

Copy link

ContributorAuthor

jmlidbetter commentedMay 11, 2020

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 😄

lostmsu requested changes

May 11, 2020

View reviewed changes

src/runtime/methodbinder.cs OutdatedShow resolvedHide resolved

jmlidbetter force-pushed thefix_kwarg_func_resolution branch fromce07727 to2b98872Compare

May 12, 2020 08:17

jmlidbetter commented

May 12, 2020

View reviewed changes

src/runtime/methodbinder.cs

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

Copy link

ContributorAuthor

jmlidbetterMay 12, 2020

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

lostmsu requested changes

May 12, 2020

View reviewed changes

src/runtime/methodbinder.cs Outdated

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

Copy link

Member

lostmsuMay 12, 2020

Choose a reason for hiding this comment

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

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

src/runtime/methodbinder.cs

Comment on lines +380 to +386

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

Copy link

Member

lostmsuMay 12, 2020•
edited
Loading

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.

src/runtime/methodbinder.cs OutdatedShow resolvedHide resolved

jmlidbetter force-pushed thefix_kwarg_func_resolution branch from643f7bc to1dd8725Compare

May 13, 2020 06:33

lostmsu approved these changes

May 13, 2020

View reviewed changes

Copy link

Member

lostmsu left a comment

Choose a reason for hiding this comment

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

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

lostmsu added this to the3.0.0 milestone

May 13, 2020

Copy link

Member

lostmsu commentedJun 19, 2020

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

Copy link

ContributorAuthor

jmlidbetter commentedJun 19, 2020

@jmlidbetter we have released 2.5 andmaster 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?

Copy link

Member

lostmsu commentedJun 20, 2020

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

jmlidbetter added5 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 thefix_kwarg_func_resolution branch from7caa7ed tod453159Compare

June 22, 2020 15:21

Updates CHANGELOG

a1b6c76

Copy link

ContributorAuthor

jmlidbetter commentedJun 22, 2020

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

Copy link

Member

lostmsu commentedJun 22, 2020

@jmlidbetter related tests seem to be failing.

Copy link

ContributorAuthor

jmlidbetter commentedJun 22, 2020

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

Copy link

ContributorAuthor

jmlidbetter commentedJun 22, 2020•
edited
Loading

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.

Copy link

ContributorAuthor

jmlidbetter commentedJun 23, 2020

@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?

Copy link

Member

lostmsu commentedJun 23, 2020

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.

voidNoOverloads(inta=0){}

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

Copy link

ContributorAuthor

jmlidbetter commentedJun 23, 2020

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.

Copy link

ContributorAuthor

jmlidbetter commentedJun 25, 2020•
edited
Loading

@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.

Merge branch 'master' into fix_kwarg_func_resolution

1d01c45

Merge branch 'master' into fix_kwarg_func_resolution

980c231

Copy link

ContributorAuthor

jmlidbetter commentedOct 21, 2020•
edited
Loading

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

Merge branch 'master' into fix_kwarg_func_resolution

8d0fc2c

Copy link

Member

filmor commentedNov 18, 2020

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

Copy link

codecov-io commentedNov 18, 2020•
edited
Loading

Codecov Report

Merging#1136 (8d0fc2c) intomaster (7870a9f) willnot change coverage.
The diff coverage isn/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 byCodecov. Last update7870a9f...8d0fc2c. Read thecomment docs.

lostmsu merged commitc81c3c3 intopythonnet:master

Nov 18, 2020

Labels

None yet

6 participants

Movatterモバイル変換

Fix kwarg func resolution#1136

Fix kwarg func resolution#1136

Uh oh!

Conversation

jmlidbetter commentedMay 7, 2020

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

Any other comments?

Checklist

Uh oh!

dnfclas commentedMay 7, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

DrNickClarke commentedMay 7, 2020

Uh oh!

lostmsu left a comment

Choose a reason for hiding this comment

Uh oh!

lostmsuMay 7, 2020

Choose a reason for hiding this comment

Uh oh!

jmlidbetter commentedMay 11, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jmlidbetterMay 12, 2020

Choose a reason for hiding this comment

Uh oh!

lostmsuMay 12, 2020

Choose a reason for hiding this comment

Uh oh!

lostmsuMay 12, 2020• editedLoading 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 commentedJun 19, 2020

Uh oh!

jmlidbetter commentedJun 19, 2020

Uh oh!

lostmsu commentedJun 20, 2020

Uh oh!

jmlidbetter commentedJun 22, 2020

Uh oh!

lostmsu commentedJun 22, 2020

Uh oh!

jmlidbetter commentedJun 22, 2020

Uh oh!

jmlidbetter commentedJun 22, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

jmlidbetter commentedJun 23, 2020

Uh oh!

lostmsu commentedJun 23, 2020

Uh oh!

jmlidbetter commentedJun 23, 2020

Uh oh!

jmlidbetter commentedJun 25, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

jmlidbetter commentedOct 21, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Uh oh!

filmor commentedNov 18, 2020

Uh oh!

codecov-io commentedNov 18, 2020• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

dnfclas commentedMay 7, 2020•
edited
Loading

lostmsuMay 12, 2020•
edited
Loading

jmlidbetter commentedJun 22, 2020•
edited
Loading

jmlidbetter commentedJun 25, 2020•
edited
Loading

jmlidbetter commentedOct 21, 2020•
edited
Loading

codecov-io commentedNov 18, 2020•
edited
Loading