Rhino always prefers non-varargs java method over varargs one

RESOLVED FIXED

Status

Rhino
Core
--
major
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Clovis, Assigned: Hannes Wallnoefer)

Tracking

Details

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686 (x86_64); en-US; rv:1.9) Gecko/2008052912 Firefox/3.0
Build Identifier: Rhino 1.7R1

If i try to run follow code:

var layRegE00 =  java.lang.reflect.Array.newInstance(java.lang.Object, [17,4]);

i get the exception:

Cannot convert 17,4 to java.lang.Integer.

this code works in version 1.6R1 of Rhino.



Reproducible: Always

Steps to Reproduce:
1.
2.
3.
(Assignee)

Comment 1

9 years ago
I guess this might have to do with the patch from bug 382340. I just tried and it is broken with 1.6R6, which is the first release containing that patch. FWIW, invoking that method varargs style works:

java.lang.reflect.Array.newInstance(java.lang.Object, 17, 4);
(Assignee)

Comment 2

9 years ago
I found the problem. The java.lang.reflect.Array.newInstance() method taking an int array as second argument has changed to varargs signature in Java 6, and NativeJavaMethod.preferSignature() currently always prefers non-varargs methods over varargs methods. Thus, the method with int as second argument is called on Java 6, while on Java 5 the right method in chosen.

I guess we shouldn't blindly prefer non-varargs methods over varargs ones. Removing that code results in the varargs method being called with an array object, and the non-varargs one with a number argument. 

Apart from that, I wonder if it is right that NativeJavaObject.canConvert() returns true for Scriptable -> java.lang.Number conversion. Is there a scenario where this conversion actually works - and makes sense?
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 3

9 years ago
Created attachment 351536 [details] [diff] [review]
patch and test case

This patch removes the code that always prefers non-varargs methods over varargs ones. From the original comment, my feeling is that the author added that code without a stringent cause such as actual code requiring it, and the testcase shows the new code works in this case. The comment is updated to point to this bug.
Assignee: nobody → hannes
(Assignee)

Comment 4

9 years ago
Committed to HEAD:

Checking in src/org/mozilla/javascript/NativeJavaMethod.java;
/cvsroot/mozilla/js/rhino/src/org/mozilla/javascript/NativeJavaMethod.java,v  <--  NativeJavaMethod.java
new revision: 1.64; previous revision: 1.63
done
RCS file: /cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/Attic/Bug467396Test.java,v
done
Checking in testsrc/org/mozilla/javascript/tests/Bug467396Test.java;
/cvsroot/mozilla/js/rhino/testsrc/org/mozilla/javascript/tests/Attic/Bug467396Test.java,v  <--  Bug467396Test.java
new revision: 1.1.2.1; previous revision: 1.1
done

Probably should go also to Rhino1_7R2_BRANCH, although there's a slight chance something else might break.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Summary: Rhino misinterpret array elements as wrong integers. → Rhino always prefers non-varargs java method over varargs one

Comment 5

8 years ago
This change caused a regression (see bug 496585). I'm working on a fix that will preserve this case as well.

Comment 6

8 years ago
I committed my fix to bug 496585. That fix includes a new regression test for this bug: testsrc/doctests/467396.doctest.
You need to log in before you can comment on or make changes to this bug.