Method resolution fix with overloading and variable arity#282
Conversation
| ReflectedCallData callData = new ReflectedCallData(); | ||
| ReflectedArgs match = null; | ||
| // varargs methods should always have lower precedence than a non-varargs | ||
| // method that also applies but as they will always have <= arguments than an |
There was a problem hiding this comment.
I'm not sure this is true but I don't think it matters. eg
foo(1,2)would match both
public void foo(int i, int j) {}
public void foo(int i, int j, int... k) {}The vararg case has more arguments but still shouldn't be used. As (I think) the methods are in arg count order, the vararg method will be later and therefore never checked.
|
For reference (mainly to save me having to look it up again next time), the Java spec reference for overloaded method order. |
|
@tpoliaw I cloned your PR branch and tried it. Unfortunately, it is broken... |
| method = argslist[i].method; | ||
| break; | ||
| } else if (varargMatch == null) { | ||
| varargMatch = argslist[i].method; |
There was a problem hiding this comment.
This still isn't correct. The previous implementation grabs the first method that matches. This one does exactly the same thing for the vararg methods. The real problem is that ReflectedArgs.matches() only has visibility into a single declared method's parameter definition and the args passed. It cannot differentiate between multiple overloaded methods (i.e., multiple vararg methods) that match to determine which one is best. For example, think about this use case:
mymethod(Object... params)
mymethod(String key, Object... params)
mymethod(String key, Throwable error, Object...params)and then calling it like so:
error = Exception("oops")
obj.mymethod("foo", error, "abc", "def")The problem is that ReflectedArgs.matches() will return true for all three of these declared versions of mymethod() yet the code picks the first one encountered (when the right one in this particular scenario is actually the one with three params).
The reason that removing #201 works for my overloaded constructors use case is that it does not call ensureBoxedVarargs() (because isVarArgs is false) so it is able to find the correct constructor.
There was a problem hiding this comment.
It may be that the varargMethod == null check is causing problems. If it always chose the last matching vararg method it would pick the correct one for your example. Are there alternatives where an earlier one matches better than a later one?
Which would java pick out of
Foo(int i, int... rest) {}
Foo(Object i, Object j, int... rest) {}
Foo(Object i, Object j, Object k) {}When called with Foo(1, 2, 3)?
I still don't think this is caused by #201. The same thing can be seen in non-constructors which weren't affected by the change. The reason reverting it fixes this case is that vararg constructors couldn't be called before (without manually passing lists) so they were excluded from the method checks.
There was a problem hiding this comment.
In Java, it will call Foo(Object i, Object j, Object k)...
There was a problem hiding this comment.
@tpoliaw Here is a tweak to your change that I have confirmed resolves the problem for constructors. The "match score" is really just the length of the varargs method parameters list so this accomplishes finding the best match of the varargs constructors if there is no exact match of a non-varargs constructor.
I suspect that the PyReflectedFunction.__call__() matching logic needs a similar change:
} else {
// Just look for a constructor with no keyword args
List<ReflectedArgs> varargMatches = new ArrayList<>();
int n = nargs;
for (int i = 0; i < n; i++) {
rargs = argslist[i];
if (rargs.matches(null, args, Py.NoKeywords, callData)) {
if (!argslist[i].isVarArgs) {
method = argslist[i].method;
break;
} else {
varargMatches.add(argslist[i]);
}
}
}
if (method == null) {
int argsCount = -1;
for (ReflectedArgs varargsMatch : varargMatches) {
if (varargsMatch.args.length > argsCount) {
argsCount = varargsMatch.args.length;
method = varargsMatch.method;
}
}
}
}There was a problem hiding this comment.
And if only the first two options are present it's a compile error so Jython will never have to deal with it.
I think removing the varargMethod == null check achieves the same thing. If there is no matching non-vararg method, the longest (and therefore most specific) vararg method will be used instead.
There was a problem hiding this comment.
And if only the first two options are present it's a compile error so Jython will never have to deal with it.
I think removing the
varargMethod == nullcheck achieves the same thing. If there is no matching non-vararg method, the longest (and therefore most specific) vararg method will be used instead.
I don’t see how removing the varargMethod == null check does the same thing UNLESS the ordering is such that the matching varargs constructor with the longest parameter list is ALWAYS the last one encountered. Is that a safe assumption to make? What handles the sorting to ensure that behavior?
There was a problem hiding this comment.
@tpoliaw Of course, you could collapse the logic to check the length of the ReflectedArgs.args to find the longest matching varargs constructor into the first for loop instead of storing references in a List and then iterating again…
There was a problem hiding this comment.
What handles the sorting to ensure that behavior
It's in ReflectedArgs.compareTo, there are other checks in the method for when the args length matches but I think this should ensure the sort order.
int n = this.args.length;
if (n < oargs.length) {
return -1;
}
if (n > oargs.length) {
return +1;
}|
I'm not surprised, if I thought it was the full fix I'd have submitted it last year. What example were you running to test that, the same one from this issue? |
And https://docs.oracle.com/javase/specs/jls/se17/html/jls-15.html#jls-15.12.2 |
I think the reason (or one of them) for this is that |
|
I'm moving this out of WIP for now. As far as I can make out, this fixes the issues seen with vararg method orders. If the basic premise is ok, I'm happy to look at tidying it up a bit and maybe extracting the common sections. |
|
The test failures on the launcher and spelling are unrelated (and can be fixed by merging from |
|
@jeff5 I must have missed the new one amongst the expected ones. Should be fixed now. Also rebased onto master and squashed a couple of the fix/format changes. |
|
My fix for the Python version in the launcher test has broken again. (Someone is determined to stamp out Python 2: I wonder who?) I'll fix that again. But |
|
I'm getting less confident in this PR with every failure. Locally I'm now only getting |
I would run them individually to see the manner they fail, or cut and paste the list of tests from that output to a command like: If the complaints are about numbers of arguments, we're probably not there yet. If it's networking, they may be spurious (e.g. firewall or ISP interference). |
|
There's nothing that looks related and I'm getting all the same errors when building |
|
Well, it passed here on GitHub, and for me at home, so I'll dive into the code with the questions in my mind being:
If we have to incorporate from |
|
Thanks for looking through it. The main thing I'm not happy about is the duplication and whether it can be extracted to a single place.
Sorry about that, it's habit from the Gerrit workflow where the commit is reviewed unit rather than change as a whole. |
|
The main question, of course, is "what would Java do". The specification is quite long and it prescribes several steps and sub-steps (phases) for finding the right definition. It covers a lot of possibilities even when we ignore things Jython 2 doesn't understand (type variables, lambdas). I'm fairly sure our process is not the one prescribed, but do we get the same result? Since I don't hold all this in my head, I've added to because they match both I've been less successful showing we respect the rule about selecting the most specific definition. If I introduce It makes different choices from Java when the first argument s Combine this with the fact that However, I bet a load of stuff will break if we change that. |
We call the methods in the test object from Java in order to set expectations for the Python call.
|
A couple of thoughts:
Jython may never be perfect but last I checked, this PR fixes a real problem and at least, does as good a job as (in fact, a better job than) Jython 2.7.1 and before. My vote would be to wrap this PR up, merge it, and let others report issues if they find them. Clearly, the current 2.7.2/2.7.3 is badly broken and this change supports our real-world use cases as far as I have been able to test it so while it is maybe not perfect, it at least restores pre-2.7.2 functionality that was broken in 2.7.2/2.7.3. |
That text (as Java, in context) won't compile, to save you from the ambiguity, but it doesn't stop Jython interpreting the similar expression in Python as unambiguously a call to just one of those definitions. Python 3 has only one kind of |
again, the JVM will never allow the call (to the wrongly selected method) to even occur, whether due to a compiler error or a runtime dispatch error.
So where does that leave us? What do we need to do to get this fix merged (and 2.7.4 released)? |
You and Peter @tpoliaw have dug into this fairly thoroughly (thank you) and I've played around with it enough now to follow your discussion. If you're happy this fixes the variable arity call I am too. It doesn't implement the standard process exactly and can't without changes that would almost certainly be problematic elsewhere. (Worth re-thinking for Python 3, though.) So I expect more problems. In fact, I think #276 is one. |
We beef up these tests to show that PR jython#282 also fixes jython#281.
This is very much a WIP and I've not looked at it since I opened #221 last year. I'll try and make sense of what I was doing but open to suggestions of better ways to approach it until then.