Add native iterators for Map/List/Collections

[rPraml]

This is a roll up of #827 and #830

I want to discuss, if it makes sense to implement __iterator__ on Java Map/Iterable objects.

This will allow to iterate with for each over any iterable java object. (e.g. HashSets)

It will also improve performance on NativeJavaMap and NativeJavaList, as a call to getIds() (https://github.com/mozilla/rhino/blob/master/src/org/mozilla/javascript/NativeJavaMap.java#L117 and https://github.com/mozilla/rhino/blob/master/src/org/mozilla/javascript/NativeJavaList.java#L90) is not neccessary.

On the other hand, it is no longer possible, to modify the list in the for loop (you may get a concurrentModificationException)

So, we should decide, if this PR is worth to merge, maybe I can also add a contextFlag for this feature.

[gbrail]

Thanks! I agree that it makes sense to do this. I think we should talk about what we want to enable, however.

It looks like this PR enables the pre-ES6 iterator behavior, so that one can do a "for each" over a collection.

I also think that we should support the "Symbol.iterator" property on imported collections so that ES6 users can iterate in other ways and so that Java collections can work in other places that work with ES6 iterators, like Array.from for example.

We do this now using a Symbol property that matches SymbolKey.ITERATOR (this works even if the user is not in ES6 mode and doesn't have the Symbol global property defined). It also looks like we already support that in NativeJavaMap and NativeJavaObject for Iterable objects. Could you look and see whether we could make that approach more generic instead?

[tuchida]

Do you want to make Nashorn's code work with Rhino?
https://docs.oracle.com/javase/9/nashorn/nashorn-java-api.htm#GUID-D1C84FB4-33D6-4584-843A-FD3356C4D11F

I don't really want to use syntax that are outside the ECMA262 specification, and would like to deprecate in the future if possible.

[rPraml]

Thanks for the feedback. First I have to admit that I didn't know that there were differences between ES6 and pre-ES6. I also did not know that the "for each" style is nashorn specific. I agreee that this should be deprecated. (At least I'll try to avoid it in our code)

It also looks like we already support that in NativeJavaMap and NativeJavaObject for Iterable objects. Could you look and see whether we could make that approach more generic instead?

Yes, you're right, there is already support for ES6 Symbol.iterator in NativeJavaObject https://github.com/mozilla/rhino/blob/master/src/org/mozilla/javascript/NativeJavaObject.java#L105 and NativeJavaMap.

The NativeJavaMap iterator will work well with for .. of - it will iterate over entries, which will also cover a lot of our use cases.
Unfortunately, for .. of does not work on javascript-objects - we have some use cases, where we assume, that a javascript object behaves like a java.util.Map.
Especially, we need something to iterare over maps and javascript objects the same way. That's the background behind this pr.

var js = {};
var java = new java.util.Map();

for (var key in XX) // currently works for js and java the same way (if keys are Strings) - it calls 'getIds'
for each (var valuein XX) // currently works for js and java the same way (but is not ECMA specific)
for (var entry of XX) // works well for java-maps. Iterates over entries, but doesn't work on javascript objects, because they defne no iterator (which can be fixed in JS)
Object.values(XX) // this would also help us in many use cases, but this is currently not implemented.

[tonygermano]

Just to clear things up, for each is in Rhino because it is part of the e4x specification, not because it came from Nashorn. Originally used for xml, I think it was extended to work on other objects in a similar way to for...of in that it returns values instead of keys, but it iterates in the same way as for...in, making it not suitable for arrays. For example,

js> var a = [1,2,3]
js> a.property = 'prop'
prop
js> for each (var value in a) print(value)
1
2
3
prop

https://developer.mozilla.org/en-US/docs/Archive/Web/JavaScript/for_each...in

There is a lot of e4x code that would break if for each were removed. That said, ES6 iterators with for...of should be preferred when not dealing with xml.

This is how NativeIterator is meant to be used:

js> // this works because NativeArray implements java.util.Collection
js> var arrayList = new java.util.ArrayList(['foo', 'bar', 'baz']);
js> // Iterator() can wrap any java.lang.Iterable or java.util.Iterator
js> for (var element in Iterator(arrayList)) print(element);
foo
bar
baz

Would it be easier to just use a js function for your use case? Something like

function getValues(objOrMap) {
    if (objOrMap instanceof java.util.Map) {
        return Array.from(objOrMap.values());
    }
    else {
        return Object.keys(objOrMap).map(key => objOrMap[key]);
    }
}
js> o = {a: 'one', b: 'two'}
[object Object]
js> m = new java.util.HashMap(o)
{a=one, b=two}
js> getValues(o)
one,two
js> getValues(m)
one,two

[tonygermano]

I read your last comment again, and it seems like using for (var entry of XX) is the way to go after defining your own iterator function for js Objects. One thing that I ran into when we were working on the stringify changes, is that it's really complicated to try to treat a java.util.Map as a javascript object when it has keys which are not Strings. This is especially true when multiple keys have equal toString() values. When you iterate over the entrySet, it doesn't matter that some of the keys may be objects that are not compatible with a js object.

This seems to work:

Object.prototype[Symbol.iterator] = function* () {
    var entries = Object.keys(this).map(k => [k, this[k]]);
    for (var i = 0; i < entries.length; i++) {
        yield entries[i];
    }
}

[rPraml]

@tonygermano Yes, handling java.util.Map is a bit tricky, especially if keys are not string. I am experimenting with a keyTranslation as you can see in this commit: FOCONIS@d02ac0b#diff-fab16163e35411a29842a0afdd4592dbf45391d96ede85a2ac737744d10ef51b

The current implementation will detect ambigous keys: FOCONIS@d02ac0b#diff-46400a21b05dae3795ff41f2ba08f98e64bcf0633a8b0d19b7c038f6e0ecb7e7R38

I still need some time to investigate what is the best solution, but using for..of looks promising.

I have also other PRs open (e.g. Object.values) which may cover some of my use cases. So ignore this PR for a while (you can also close it, if you want, I can create a new one, after my other PRs are merged)

[tuchida]

There is no iterator in the ES5 specification.
Rhino is a JavaScript engine. Please check the JavaScript specification in ECMA262 or MDN before making any changes.

[rPraml]

Thanks for the hint and I ask for your indulgence when I try to submit one or the other nonsensical change ;)

I have to admit that I don't know the specification in detail and have only even rudimentarily read it so far.
In particular, I have trouble telling the difference between value[Symbol.iterator] and value["__iterator__"]. Especially finding the correct documentation

As far I understand now, the __iterator__ is a very old thing (https://web.archive.org/web/20061203042508/http://developer.mozilla.org:80/en/docs/New_in_JavaScript_1.7%23Iterators) and is used when looping with for...in or for each ... in (for each in is also deprecated/not recommended to use, as I remember from other comments)

The Symbol.iterator was introduced with ES5 and works completely different than the above one. It is used in for..of loops.

Rhino is a JavaScript engine.
... and here we are trying to use java objects in javascript (which isn't specified yet, AFAIK)

So, what this PR enables is to iterate over any java.util.Iterable with for...in / for each ... in.

and now, while writing these lines, the penny dropped...

1. It doesn't make much sense to use for...in with a java.util.HashSet as you might get only the IDs, but you cannot access them by hashSet[id]
2. The for each ... in is deprecated and Rhino specific. It can be easily replaced by for ... of

So the advantage of this PR is, that you can use the (deprecated) for each on iterable objects.

Unfortunately, we have used "for each" in our code and also the patch in this PR, so the clean way is to migrate our js code to for...of.

Thanks again
Roland

[rPraml]

Closing this PR. Use for...of instead of for each ... in