ninia / jep

Embed Python in Java
Other
1.28k stars 145 forks source link

varargs support in overloaded methods #451

Closed andvazqu closed 7 months ago

andvazqu commented 1 year ago

pyjmultimethod won't match the correct method when using overloaded methods with variable arguments.

To reproduce Use a java class with an overloaded method and variable arguments, such as: java_class

Calls to these methods from the jep console return "expected x but received y" or "No such method" errors: jep_console

Expected behavior jep_console_fix

Comments _pyjmultimethodcall checks parameterCount == argsSize, so a call to method(1) is matched to method(String...args) instead of to method(int arg, String...args) . method() finds no compatible match.

Made some quick changes to _pyjmultimethodcall : changes_to_pyjmultimethocall

and _PyJMethodCheckArguments : changes_to_pyjmethodcheckarguments

to take into consideration variable arguments. This worked for my use case (see expected behavior), but haven't tested it thoroughly.

Environment:

bsteffensmeier commented 1 year ago

Thanks for contributing, this capability is a nice improvement. Choosing the right overloaded methods is one of the areas where there is always more improvements to make the python/java transition a little smoother.

I would like to incorporate this change into jep. If you have the ability to create a Pull Request with the changes that would be a little easier to review and merge.

I did notice one potential problem, in pyjmultimethod_call when you are finding candMatch it passes varargs but that would be the varargs for method and not the varargs of cand. The reason we sometimes calculate candMatch there is because we are trying to optimize, if there is only one method with the matching number of args then we don't need to call PyJMethod_CheckArguments at all so we wait until we find a second potential match before checking the args. It should be a minor fix to save or get the varargs for cand.

If you are interested in contributing more to jep then the next step in this direction would be to make sure the extra args are compatible with the varags class. I haven't tried it but I believe if a method was overloaded with varargs of different types then it would pick the first one and not even try to match the varargs type. For example if your java methods are defined like this:

public void overloadedVarArgMethod(String... args)
public void overloadedVarArgMethod(int... args)

Then one of these python calls would choose the wrong method.

v.overloadedVarArgMethod(1,2,3)
v.overloadedVarArgMethod("a","b","c")

That use case is not at all a requirement for merging your existing change, it is just something that comes to mind while reviewing your change.

andvazqu commented 1 year ago

Thanks for the quick reply! I added a type check for the use case you mentioned of a method overloaded with varargs of different types and created a pull request.

bsteffensmeier commented 7 months ago

This PR was merged into the 4.2 release which was officially released today so I am closing this issue.