Improve NativeJavaList to behave more like NativeArray #830

rPraml · 2021-01-25T10:33:58Z

This PR extends the NativeJavaList to behave more like NativeArray.

Some ideas are extracted from #827

@tuchida maybe you can take a look at this.

tuchida · 2021-01-25T11:24:59Z

testsrc/org/mozilla/javascript/tests/NativeJavaListTest.java

+        listD.add(1.0);
+        listD.add(2.0);
+        listD.add(3.0);
+        assertEquals("1", runScriptAsString("value.indexOf(2)", listD));


Isn't this calling Java's indexOf method?

https://docs.oracle.com/javase/8/docs/api/java/util/List.html#indexOf-java.lang.Object-
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/indexOf

They differ in the following ways.

JS can receive fromIndex as the second argument.

var list = new java.util.ArrayList(); list.indexOf(1, 2); // js: Can't find method java.util.ArrayList.indexOf(number,number).

The equivalence check is different.

list.add(NaN); list.indexOf(NaN); // 0 [NaN].indexOf(NaN); // -1

https://docs.oracle.com/javase/8/docs/api/java/lang/Double.html#equals-java.lang.Object-

~~There may be compatibility issues.~~ Sorry, mistake.

You're right, value.indexOf will call the Java's indexOf method.
Array.indexOf(value,...) will call the JS indexOf method.

The question is, what to do, if a method is defined by Java and also by the JS-Array-prototype. Which one should we prefer?
Logically, the method in NativeJavaList will override the method of the prototype.

The next question is, what shoud happen if you wrap an instance, that declares other methods:

public class MyList extends ArrayList { public String join() { ... } } list1 = new ArrayList(); list2 = new MyList(); runScriptAsString("value.join('-')", list1); // call js_join runScriptAsString("value.join('-')", list2); // will call MyList.join - but arguments will not match

tuchida · 2021-01-25T12:21:50Z

In Rhino, all methods of an instance can be accessed.

// java
public static List getList() {
    return new ArrayList();
}

// js
var list = Foo.getList();
// ArrayList method, not a List method.
list.forEach;
// function forEach() {/*
// void forEach(java.util.function.Consumer)
// */}

When calling Java's methods against the return value of getList, it is safe to use only the List's methods. However, Java methods have priority, so if you use JS's methods, you need to avoid ArrayList's methods. If getList is changed to return a List implementation that is not an ArrayList, then you may not be able to call the JS's methods.

tuchida · 2021-01-25T12:40:17Z

I think it would be better to implement Symbol.iterator and convert it with Array.from to avoid problems. Like NodeList.
https://developer.mozilla.org/en-US/docs/Web/API/NodeList

rPraml · 2021-01-25T13:25:27Z

When calling Java's methods against the return value of getList, it is safe to use only the List's methods.
However, Java methods have priority, so if you use JS's methods, you need to avoid ArrayList's methods.
If getList is changed to return a List implementation that is not an ArrayList, then you may not be able to call the JS's methods.

On the one hand I think, that JS methods should have always priority, maybe we should not inherit from NativeJavaObject at all??
On the other hand, implementing a method in a Java class (e.g. join()) is like overloading in JS.

I think it would be better to implement Symbol.iterator like NodeList and convert it with Array.from to avoid problems.
https://developer.mozilla.org/en-US/docs/Web/API/NodeList

Hmm, you mean:

var javaList = new java.util.ArrayList()
var jsList = Array.from(javaList)

This means, that changes on jsList are not reflected to javaList
I can/will implement iterator in a separate PR

tuchida · 2021-01-25T13:40:40Z

If it don't prioritize Java methods, it may run into problems with code written in the past.
However, whichever it prioritize, it will behave strangely in corner cases as long as the names conflict.

rPraml · 2021-01-25T10:35:41Z

src/org/mozilla/javascript/NativeJavaList.java

    private List<Object> list;

    @SuppressWarnings("unchecked")
    public NativeJavaList(Scriptable scope, Object list) {
        super(scope, list, list.getClass());
        assert list instanceof List;
        this.list = (List<Object>) list;
+        setPrototype(ScriptableObject.getClassPrototype(scope, "Array"));


This makes it possible to invoke all/most NativeArray mehtods also an NativeJavaList
Is this a good/acceptable change?

rPraml · 2021-01-25T10:38:12Z

src/org/mozilla/javascript/NativeJavaList.java

@@ -39,6 +42,13 @@ public boolean has(int index, Scriptable start) {
        }
        return super.has(index, start);
    }
+
+    public void delete(int index) {
+        if (isWithValidIndex(index)) {


delete value[0] does work, but will return false as has(0,start) will still return true

Should we maintain a boolean[] array to store all undefined values?

IMHO this is a tradeOff, I would not add this complexity to NativeArray to simulate "empty slots"

rPraml · 2021-01-25T10:53:41Z

src/org/mozilla/javascript/ScriptRuntime.java

+        }
+        if (y instanceof Wrapper) {
+            y = ((Wrapper) y).unwrap();
+        }


The unwrap here was neccessary to make 'includes' work.
I can place the unwrap also here: https://github.com/mozilla/rhino/blob/master/src/org/mozilla/javascript/NativeArray.java#L1766

rPraml · 2021-01-25T10:57:35Z

src/org/mozilla/javascript/ScriptRuntime.java

@@ -3372,6 +3378,12 @@ private static boolean eqString(CharSequence x, Object y)
    }
    public static boolean shallowEq(Object x, Object y)
    {
+        if (x instanceof Wrapper) {


This was neccessary to make 'indexOf'/'lastIndexOf' work.
The unwrap can also be placed here:
https://github.com/mozilla/rhino/blob/master/src/org/mozilla/javascript/NativeArray.java#L1671
https://github.com/mozilla/rhino/blob/master/src/org/mozilla/javascript/NativeArray.java#L1727

rPraml · 2021-01-25T11:00:37Z

testsrc/org/mozilla/javascript/tests/NativeJavaListTest.java

+        // FIXME: This will invoke the java.util.List.indexOf method, which
+        // is not yet type aware!
+        // assertEquals("1", runScriptAsString("value.lastIndexOf(2)", listI));
+        // assertEquals("-1", runScriptAsString("value.lastIndexOf(4)", listD));


I tried to write a test for each method defined by the Array prototype.
There are some methods defined by the wrapped object (java.util.List). These methods have precedence.

rPraml · 2021-01-25T12:11:33Z

testsrc/org/mozilla/javascript/tests/NativeJavaListTest.java

+        listD.add(1.0);
+        listD.add(2.0);
+        listD.add(3.0);
+        assertEquals("1", runScriptAsString("value.indexOf(2)", listD));


You're right, value.indexOf will call the Java's indexOf method.
Array.indexOf(value,...) will call the JS indexOf method.

The question is, what to do, if a method is defined by Java and also by the JS-Array-prototype. Which one should we prefer?
Logically, the method in NativeJavaList will override the method of the prototype.

The next question is, what shoud happen if you wrap an instance, that declares other methods:

public class MyList extends ArrayList { public String join() { ... } } list1 = new ArrayList(); list2 = new MyList(); runScriptAsString("value.join('-')", list1); // call js_join runScriptAsString("value.join('-')", list2); // will call MyList.join - but arguments will not match

rPraml · 2021-01-25T13:50:00Z

If it don't prioritize Java methods, it may run into problems with code written in the past.

My bad. Changes in this way should not be done.

However, whichever it prioritize, it will behave strangely in corner cases as long as the names conflict.

In most cases, there should be no conflicts, as in most cases, ArrayLists are used. And if there are conflicts, it "works as designed", respectively, this should be specified as "design"

BTW: Sry. I assume most of my "review" comments were not yet visible for you, because they were "pending"

gbrail · 2021-04-15T18:06:50Z

I'm sorry -- I've lost track of what we collectively want to do with this collection of this PR. @rPraml I am hoping that we can still resolve this but I'm not sure where you left it on the review comments. Do we still want to pursue this?

# Conflicts: # src/org/mozilla/javascript/NativeJavaList.java # src/org/mozilla/javascript/ScriptRuntime.java

rPraml · 2021-09-07T09:05:08Z

@gbrail I updated this PR (I had not much time in the past)
Not much is left, because many smaller changes are already implemented (Thanks to @tuchida and others)
So what's left:

Fixed ScriptRuntime.sameZero und ScriptRuntime.shallowEq when a Wrapper is passed (in my case a wrapped integer was compared with a primitive in Array.indexOf)
NativeJavaList does a setPrototype(ScriptableObject.getClassPrototype(scope, "Array")), so all JavaScript Array methods works also on a java.util.List.
Note: If there is a name clash between Array prototype and java.util.List (e.g. Array.indexOf() and java.util.List.indexOf()) the java method is called. See here
This is required to be backward compatible.
NativeJavaList has methods to set length and to delete entries (delete javaArr[0])

tuchida · 2021-09-07T09:52:20Z

src/org/mozilla/javascript/NativeJavaList.java

@@ -17,6 +18,7 @@ public NativeJavaList(Scriptable scope, Object list) {
        super(scope, list, list.getClass());
        assert list instanceof List;
        this.list = (List<Object>) list;
+        setPrototype(ScriptableObject.getClassPrototype(scope, "Array"));


What happens if you are using instanceof to determine if it is an array?

if (value instanceof Array) { // some code to operate `value` }

How about Array.isArray?

Hello Tuchida, value instanceof Array will return true, while Array.isArray(value) will return false.

I get different results in firefox

class inheritance

class Foo extends Array {}; var y = new Foo(); y instanceof Array; // returns true Array.isArray(y); // returns true

vs. prototype

var Bar = function(){}; Bar.prototype = Array.prototype; var x = new Bar(); x instanceof Array; // returns true Array.isArray(x); // returns false

What do you think, is ok, when we return true for instanceof and false for isArray?

I think javaList instanceof Array should be false.

Java's List and JavaScript's Array are different, so their detailed behaviors are different.(For example, .indexOf)
If the existing code is written as follows, a List will be passed to the code that is expecting an Array.

if (value instanceof Array) { // some code to operate `value` }

I think it may cause compatibility issues.

rPraml · 2021-09-10T13:43:35Z

@tuchida I made JavaList/JavaObject available as prototypes and made some fixes in class inheritance, so all of them is true:

var x= new java.util.ArrayList();
x instanceof JavaList; 
x instanceof JavaObject; 
x instanceof Object;
x instanceof java.util.List

To add the array functions, I can define them now on the JavaList

Object.getOwnPropertyNames(Array.prototype).forEach(function(x) { JavaList.prototype[x] = Array.prototype[x]})

I think this would be the nicest solutuion, when someone wants to handle JavaList as an array.

What do you think about that change?

p-bakker · 2021-09-10T14:00:16Z

Haven't followed along here, but you might want to look at how GraalJS handles Java List interoperability, if only to see how they solved things

tuchida

To what extent do you want Java's List and JS's Array to be the same?
If we make such a change, it would be better to discuss the specification before implementing it.
For example, as p-bakker commented, I think there are issues that should be considered for the specification, such as investigating how GraalJS and Nashorn are doing.

tuchida · 2021-09-11T11:41:22Z

src/org/mozilla/javascript/NativeJavaObject.java

+
+                    public Object call(
+                            Context cx, Scriptable scope, Scriptable thisObj, Object[] args) {
+                        throw new UnsupportedOperationException(


This is not an EcmaError and will cause the process to terminate.

js> new JavaMap() Exception in thread "main" java.lang.UnsupportedOperationException: Function not constructable at org.mozilla.javascript.NativeJavaObject$1.call(NativeJavaObject.java:79) at org.mozilla.javascript.BaseFunction.construct(BaseFunction.java:385) at org.mozilla.javascript.ScriptRuntime.newObject(ScriptRuntime.java:2678) at org.mozilla.javascript.gen._stdin__8._c_script_0(Unknown Source) at org.mozilla.javascript.gen._stdin__8.call(Unknown Source) at org.mozilla.javascript.ContextFactory.doTopCall(ContextFactory.java:380) at org.mozilla.javascript.ScriptRuntime.doTopCall(ScriptRuntime.java:3872) at org.mozilla.javascript.gen._stdin__8.call(Unknown Source) at org.mozilla.javascript.gen._stdin__8.exec(Unknown Source) at org.mozilla.javascript.tools.shell.Main.processSource(Main.java:497) at org.mozilla.javascript.tools.shell.Main.processFiles(Main.java:181) at org.mozilla.javascript.tools.shell.Main$IProxy.run(Main.java:101) at org.mozilla.javascript.Context.call(Context.java:535) at org.mozilla.javascript.ContextFactory.call(ContextFactory.java:472) at org.mozilla.javascript.tools.shell.Main.exec(Main.java:163) at org.mozilla.javascript.tools.shell.Main.main(Main.java:138)

tuchida · 2021-09-11T11:42:33Z

src/org/mozilla/javascript/NativeJavaList.java


    private List<Object> list;

+    static void init(ScriptableObject scope, boolean sealed) {
+        NativeJavaList obj = new NativeJavaList();
+        obj.exportAsJSClass(scope, "JavaObject", sealed);


Is it correct to say that new JavaObject, JavaList, and JavaMap will be added to the global variables?

Why these three? There are many interfaces and classes in Java. For example, if you want to extend a Set or Stream with a prototype, do you have to add more JavaXXX each time? Why List instead of Collection?

If you have a problem you want to solve, it is better to fully discuss what specifications you want to use before implementing them.

My original problem was, that the Wrappers (JavaList / JavaMap) should provide the same functionality as the equivalent JS objects (Array/Object), so that I can use existing javaScript code that can work on the wrappers without modification. This goal has already been achieved very well by other PRs that improve List/Map handling.

I also took a look at GraalJS as suggested by @p-bakker
You can add objects with list[x] = value, but only if x points to the end of the list (x == list.length)

In this PR I think I've lost the focus.
I will investigate in that direction and will add only neccessary features to rhino

tuchida · 2021-09-11T11:45:13Z

src/org/mozilla/javascript/NativeJavaObject.java

@@ -34,6 +34,8 @@

    static void init(ScriptableObject scope, boolean sealed) {
        JavaIterableIterator.init(scope, sealed);
+        NativeJavaObject obj = new NativeJavaObject();
+        obj.exportAsJSClass(scope, "Object", sealed);


Is it correct that JavaObject inherits from JS Object?
JS Object prototype defines valueOf, toLocaleString, etc. Are there likely to be compatibility issues with these?

testsrc/org/mozilla/javascript/tests/NativeJavaListTest.java

tuchida · 2021-09-11T12:11:19Z

I consider Java's List and JS's Array to be different things, so I think mutual conversion is sufficient.

var javaList = java.util.ArrayList([1, 2, 3]);
var jsArray = Array.from(javaList);

In JavaScript, there are Array-Like objects that look like Arrays but are not.
For example, arguments and NodeList.
If you want to use Array methods by these, it is customary to convert them using Array.from or Spread syntax.

rPraml

Haven't followed along here, but you might want to look at how GraalJS handles Java List interoperability, if only to see how they solved things

I made some tests with GraalVM - I think this is the way we should also go in

> var ArrayList = Java.type("java.util.ArrayList");
> var List = Java.type("java.util.List")
> var Map = Java.type("java.util.Map")
> typeof ArrayList
function
> var list = new ArrayList();
> typeof list
object
> var arr = []
> typeof arr
object
> arr instanceof Object
true
> list instanceof Object
false
> list instanceof List
true
> list instanceof Map
false
> arr instanceof List
false

> Object.getPrototypeOf(list)
null

> Object.getOwnPropertyNames(arr)
["length"]
> Object.getOwnPropertyNames(list)
(31)["add", "remove", "get", "clone", "indexOf", "clear", "isEmpty", ...

> list[0] = 'a'
> list[1] = 'b'
> list[4] = 'c' // can only add element to the end
> list
(2)["a", "b"]
Array.prototype.includes.call(list, 'b')
true
Array.prototype.includes.call(list, 'c')
false

I will update the PR and remove unneccesary things

rPraml · 2021-09-13T12:00:37Z

src/org/mozilla/javascript/NativeJavaList.java


    private List<Object> list;

+    static void init(ScriptableObject scope, boolean sealed) {
+        NativeJavaList obj = new NativeJavaList();
+        obj.exportAsJSClass(scope, "JavaObject", sealed);


My original problem was, that the Wrappers (JavaList / JavaMap) should provide the same functionality as the equivalent JS objects (Array/Object), so that I can use existing javaScript code that can work on the wrappers without modification. This goal has already been achieved very well by other PRs that improve List/Map handling.

I also took a look at GraalJS as suggested by @p-bakker
You can add objects with list[x] = value, but only if x points to the end of the list (x == list.length)

In this PR I think I've lost the focus.
I will investigate in that direction and will add only neccessary features to rhino

tonygermano · 2021-09-14T18:00:06Z

src/org/mozilla/javascript/NativeJavaMap.java

@@ -33,7 +37,7 @@ public String getClassName() {
    @Override
    public boolean has(String name, Scriptable start) {
        Context cx = Context.getCurrentContext();
-        if (cx != null && cx.hasFeature(Context.FEATURE_ENABLE_JAVA_MAP_ACCESS)) {
+        if (map != null && cx != null && cx.hasFeature(Context.FEATURE_ENABLE_JAVA_MAP_ACCESS)) {


I don't think we should be introducing the possibility of map being null.

rPraml · 2021-09-17T13:42:05Z

I would close this PR and create a new one

…ist-length-support # Conflicts: # src/org/mozilla/javascript/NativeJavaList.java

# Conflicts: # testsrc/org/mozilla/javascript/tests/NativeJavaListTest.java

rPraml · 2021-11-08T16:04:42Z

closing this PR - all changes are in other PRs

rPraml added 2 commits January 25, 2021 11:20

Improve NativeJavaList to behave more like NativeArray

7f55b2f

Better error handling on setLength

fab481f

tuchida reviewed Jan 25, 2021

View reviewed changes

rPraml commented Jan 25, 2021

View reviewed changes

rPraml mentioned this pull request May 10, 2021

Add native iterators for Map/List/Collections #890

Closed

p-bakker added the Java Interop Issues related to the interaction between Java and JavaScript label Jun 30, 2021

rPraml added 5 commits September 7, 2021 08:17

Merge branch 'tmp' into list-length-support

080bbd4

# Conflicts: # src/org/mozilla/javascript/NativeJavaList.java # src/org/mozilla/javascript/ScriptRuntime.java

Update submodule

12e2576

spotlessApply

719b673

reverted unexplainable change.

aa0afca

FIX: Wrapper handling in "sameZero" and "shallowEq"

1a3fd27

tuchida reviewed Sep 7, 2021

View reviewed changes

rPraml added 3 commits September 8, 2021 12:05

ADD: Test for "instanceOf"

5a0ec05

spotless

a2b3b3f

Merge remote-tracking branch 'upstream/master' into list-length-support

62835f9

rPraml mentioned this pull request Sep 10, 2021

Do not init standard properties twice #1025

Merged

JavaList inherits from JavaObject inherits from Object now

cf107f2

tuchida reviewed Sep 11, 2021

View reviewed changes

rPraml commented Sep 14, 2021

View reviewed changes

removed unnecessary changes

6233e33

tonygermano reviewed Sep 14, 2021

View reviewed changes

rPraml added 2 commits September 16, 2021 17:10

Merge remote-tracking branch 'upstream/master' into list-length-support

7fb6e38

ADD: delete method and set length property to NativeJavaList

9931a30

rPraml mentioned this pull request Sep 17, 2021

ADD: delete method and set length property to NativeJavaList #1031

Merged

rPraml mentioned this pull request Sep 21, 2021

Java List and array access improvements #561

Closed

rPraml added 2 commits November 8, 2021 16:25

Merge branch 'list-length-support' of github.com:FOCONIS/rhino into l…

40592ce

…ist-length-support # Conflicts: # src/org/mozilla/javascript/NativeJavaList.java

Merge remote-tracking branch 'upstream/master' into list-length-support

f292b0c

# Conflicts: # testsrc/org/mozilla/javascript/tests/NativeJavaListTest.java

rPraml mentioned this pull request Nov 8, 2021

Improve NativeJavaList to behave more like NativeArray 2 #1083

Draft

rPraml closed this Nov 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve NativeJavaList to behave more like NativeArray #830

Improve NativeJavaList to behave more like NativeArray #830

rPraml commented Jan 25, 2021

tuchida Jan 25, 2021 •

edited

Loading

rPraml Jan 25, 2021

tuchida commented Jan 25, 2021

tuchida commented Jan 25, 2021 •

edited

Loading

rPraml commented Jan 25, 2021 •

edited

Loading

tuchida commented Jan 25, 2021

rPraml Jan 25, 2021

rPraml Jan 25, 2021

rPraml Sep 7, 2021

rPraml Jan 25, 2021

rPraml Jan 25, 2021

rPraml Jan 25, 2021

rPraml Jan 25, 2021

rPraml commented Jan 25, 2021

gbrail commented Apr 15, 2021

rPraml commented Sep 7, 2021

tuchida Sep 7, 2021

rPraml Sep 9, 2021

tuchida Sep 9, 2021

rPraml commented Sep 10, 2021

p-bakker commented Sep 10, 2021

tuchida left a comment

tuchida Sep 11, 2021

tuchida Sep 11, 2021

tuchida Sep 12, 2021

rPraml Sep 13, 2021

tuchida Sep 11, 2021

tuchida commented Sep 11, 2021 •

edited

Loading

rPraml left a comment

rPraml Sep 13, 2021

tonygermano Sep 14, 2021

rPraml commented Sep 17, 2021

rPraml commented Nov 8, 2021

Improve NativeJavaList to behave more like NativeArray #830

Improve NativeJavaList to behave more like NativeArray #830

Conversation

rPraml commented Jan 25, 2021

tuchida Jan 25, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuchida commented Jan 25, 2021

tuchida commented Jan 25, 2021 • edited Loading

rPraml commented Jan 25, 2021 • edited Loading

tuchida commented Jan 25, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rPraml commented Jan 25, 2021

gbrail commented Apr 15, 2021

rPraml commented Sep 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rPraml commented Sep 10, 2021

p-bakker commented Sep 10, 2021

tuchida left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tuchida commented Sep 11, 2021 • edited Loading

rPraml left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rPraml commented Sep 17, 2021

rPraml commented Nov 8, 2021

tuchida Jan 25, 2021 •

edited

Loading

tuchida commented Jan 25, 2021 •

edited

Loading

rPraml commented Jan 25, 2021 •

edited

Loading

tuchida commented Sep 11, 2021 •

edited

Loading