Non deterministic behaviour with vararg methods

[rPraml]

Summary
If there are two methods, one with varargs and an other without, for example

• String args(String arg1, String... args)
• String args(String arg1)
  rhino considers these two methods as PREFERENCE_EQUAL when it is invoked with args('foo') from javascript. This may result in a non deterministic behaviour as one of the two was taken.

Details
In the case described above, all methods are compared and it is tried to find the best matchin one. So after preferSignaturewe will run into this code part in
NativeJavaMethod

                            if (preference != PREFERENCE_EQUAL) Kit.codeBug();
                            // This should not happen in theory
                            // but on some JVMs, Class.getMethods will return all
                            // static methods of the class hierarchy, even if
                            // a derived class's parameters match exactly.
                            // We want to call the derived class's method.
                            if (bestFit.isStatic()
                                    && bestFit.getDeclaringClass()
                                            .isAssignableFrom(member.getDeclaringClass())) {
                                // ... 
                            } else {
                                if (debug)
                                    printDebug("Ignoring same signature member ", member, args); // CASE 2
                            }

In our application, where the bug occured, we run in the Ignoring same signature member code path and one (not really deterministic - because of HashMap) method was taken. So the code sometimes works and sometimes not.

It was also not easy to provide a deterministic failing test, that's why (and maybe to add more determinism) I've changed the map in discoverAccessibleMethods to a LinkedHashMap (and yet the test depends heavily on the order of the JVM's getDeclaredMethods implementation)

Fix
When there are two candidates, the no-vararg method is taken. This is the same behaviour, as it is in Java.

[rPraml]

same test, but different order. If using HashMap instead of LinkedHashMap, I cannot provide stable failing tests

[rPraml]

To discuss: Should we make this change? It would help to add determinism (as long as the JVM's getDeclaredMethods returns them in the same order as in source code)

[gbrail]

I think that this change makes sense and fixes a bug, and I'm fine with using LinkedHashMap here so that it's deterministic. However, I don't currently work on anything that uses all of the Java reflection stuff in Rhino, and I'd love an opinion by another regular user who works with Rhino this way to make sure that we're not missing anything.

Thanks!

[rPraml]

@gbrail I've resolved merge commits here

[p-bakker]

Never ran into this, but LGTM

@andreabergia @tonygermano care to comment on this one as well, given that both of you seem to use Java Interop in your Rhino integrations

[andreabergia]

Looks good to me!

[tonygermano]

I haven't reviewed the code, but the stated fix of making the behavior match what java does seems correct to me.

[tonygermano]

Here's some information about how java resolves overloads. https://stackoverflow.com/a/27880923

[tonygermano]

Looking at the java spec, I think it should be the very first check in the preferSignature method where if vararg1 != vararg2 to automatically prefer the one that does not have variable arity.

This does 2 things,

1. It short circuits the function and skips a lot of other tests that end up not being required.
2. Without putting too much thought into it, I suspect there are cases that could result in PREFERENCE_AMBIGUOUS where one has variable arity and the other does not. This should still prefer the method which does not have variable arity.

Edit: I think the current algorithm could even end up preferring the vararg method over the other, depending on the types, which would be also be wrong. In section 15.12.2, the note following phase 2 states,

This ensures that a method is never chosen through variable arity method invocation if it is applicable through fixed arity method invocation.

[gbrail]

Can you take a look at the comments by @tonygermano and let us know what you think? Thanks!

[rPraml]

@tonygermano thanks for feedback

I think it should be the very first check in the preferSignature method where if vararg1 != vararg2

I'm unsure. If I do the check at top of the method, I will always prefer arg(String, String) before arg(String, int...).
in rhino myObj.arg('foo', 1) will call the wrong method.

Thanks for the link to java spec. I've extended the test cases, but I think there is still one failing case. when an array is passed

[p-bakker]

Converted to draft, as it currently doesn't pass the CI build