Improve boxing of arguments to vararg methods#101
Conversation
If a Java method that accepts varargs is called from Jython, the
arguments are boxed into a PyList first. This should be skipped only if
there are already the correct number of arguments and the final
argument is a sequence type. If only the final argument is checked for
being a sequence it can block valid calls from matching the underlying
method.
eg in Java
void foo(Object... args) {}
should be callable from Jython as
foo(1, 2, [3, 4])
where the list and the numbers are all objects and passed as a single
array to the varargs method.
Similarly, if there are too few arguments, a java method such as
void bar(int a, int[] b, int... c) {}
should be callable from Jython as
bar(1, [2, 3])
where the vararg argument is empty and [2, 3] is passed as b.
The only remaining ambiguity is where a vararg parameter can take a
sequence type as an element, eg
void fizz(Object... args) {}
calling from Jython as
fizz([1,2,3])
would be valid as either args being [1,2,3] or [[1, 2, 3]]. This
ambiguity is also present in Java but behaves as if it was the former.
|
Any chance of getting some feedback on this? It would be good to get this fixed without having to maintain the patched version locally. |
|
@tpoliaw Sorry, I've been having too much fun with Jython 3, but I'm having a push on Jython 2 issues now. I hope to accept one soon. It looks well-reasoned, and to have Java tests is reassuring. Single-stepping through some of those will improve my understanding of argument matching. Then my opinion will be worth something! I don't understand the "force push" on this PR. When I pull it to a branch on my repo I only see one change set ( |
Experiment. Intend squash commit.
|
That's what I guessed. This looks good functionally, thanks. I'm merging in the tip of master for test, and will push that to your PR branch. Then there are some non-code changes (e.g. line length, and making your helpful commit message a comment) but I'll make and push those rather than ask for them. I'll squash-merge at the end which should make a single change set in which all the extra change will have evaporated. At least, I think that will work. |
Tweak code to match intent expressed in comments and examine the argument array actually formed.
|
@tpoliaw : I set out to give each test a Javadoc comment based on the comment in the body, but on close inspection, a couple of the the tests didn't actually do what they claim, so I adjusted the code to match the comment. That led me to want stronger tests of what actually comes back from So the test is a lot longer now, and I understand what the code actually does. :) Have I got this right and do they now cover the range of your addition to |
|
And in a further confirmation, if |
tpoliaw
left a comment
There was a problem hiding this comment.
Thanks for looking at this and improving the docs for the tests. I think everything looks ok to me.
A couple of nitpicking comments on the javadoc but nothing that should block the PR.
|
Sigh ... it fails irrelevantly (test_select), then succeeds on the rebound. |
The natural sequel to #101 supporting the same pattern in constructors.
Potential fix for #100.
Only check final argument for a sequence if the correct number of arguments have been passed.