Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[js] Enable haxe for-in iteration of js.lib Map and Set #8754

Merged
merged 10 commits into from
Sep 4, 2019

Conversation

haxiomic
Copy link
Member

@haxiomic haxiomic commented Sep 3, 2019

This enables using for-in on js.lib.Set, js.lib.Map and js.lib.Iterator;

Impact on Existing Code
None expected

For example, the following will now compile:

var set = new js.lib.Set();
set.add('A');
set.add('B');
set.add('C');

for (value in set) {
    trace(value);
}
var map = new js.lib.Map();
map.set('a', 1);
map.set('b', 2);
map.set('c', 3);

for (key => value in map) {
    trace('$key => $value');
}

For key-value iteration over a Set I've matched the behavior of haxe.ds.List

for (key => value in set) {
    trace('$key => $value');
}
// 0 => A
// 1 => B
// 2 => C

To iterate over a js.lib.Iterator requires using the HaxeIterator class:

using js.lib.HaxeIterator;
...

for (value in set.values()) {
    trace(value);
}

or alternatively

for (value in new js.lib.HaxeIterator(set.values())) {
    trace(value);
}

The generated code gets nicely inlined, for key-value iteration on map it looks something like

var jsIterator = map.entries();
var _this_lastStep = jsIterator.next();
while(!_this_lastStep.done) {
    var v2 = _this_lastStep.value;
    _this_lastStep = jsIterator.next();
    console.log("Main.hx", v2[0] + " => " + v2[1]);
}

@haxiomic haxiomic requested a review from nadako September 3, 2019 13:17
@haxiomic haxiomic added platform-javascript Everything related to JS / JavaScript standard library feature-es6 ES6 labels Sep 3, 2019
@:forward
abstract Iterator<T>(IteratorStructure<T>) from IteratorStructure<T> {

public inline function iterator(): JSIterator<T> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is semantically wrong. It makes js.lib.Iterator a Haxe's Iterable.
The difference is the former could be iterated one time, while the latter could be iterated multiple times.
So js.lib.Iterator should behave like Haxe's Iterator, which means fields next():T and hasNext():Bool should be declared directly on js.lib.Iterator. But I don't think it's possible because ES6 Iterator already has next with a different signature.
Though, that's pretty convenient to have iterator() field on js.lib.Iterable :)

Copy link
Member Author

@haxiomic haxiomic Sep 3, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I agree about the semantic issue, ideally we'd just have next():T and hasNext() on the abstract but I couldn't find a way around the collision with the ES6 next()

Maybe the solution here is to rename abstract Iterator to abstract Iterable and use that in Set and Map instead

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To resolve the semantic issue, I've disabled iterating over js.lib.Iterator (now instead there's a class HaxeIterator and you have to manually convert it via new HaxeIterator(jsIterator))

But iterating over Set and Map will still work and is semantically sound

(I tried lots of approaches to try to resolve the semantic issue with iterating over js.lib.Iterator however I don't think it's possible to be fully correct here)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To enable js.lib.Iterator iteration you can do

using HaxeIterator;

Which avoids any semantic issues because js.lib.Iterator still can't unify with StdTypes.Iterable

@RealyUniqueName RealyUniqueName added this to the Backlog milestone Sep 3, 2019
@skial skial mentioned this pull request Sep 4, 2019
1 task
Copy link
Member

@RealyUniqueName RealyUniqueName left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MapKvIterator and SetKvIterator are not needed. You can have inline function iterator() return new HaxeIterator(this.values() /* or this.entries() */) on both Map and Set

Edit: Sorry, SetKvIterator is actually required, because of key:index++ . But Maps iterator already returns a structure, which is compatible with Haxe's KeyValueIterator. Try it, please

@Aurel300
Copy link
Member

Aurel300 commented Sep 4, 2019

Also the Kv abbreviation seems unnecessary, why not spell it out?

@haxiomic
Copy link
Member Author

haxiomic commented Sep 4, 2019

Thanks for reviewing @RealyUniqueName :), I've removed MapKvIterator (not sure why I thought I needed it in the first place 🙃)

It would be nice to have a @:using(HaxeIterator) on typedef Iterator so this works out of the box:

for (key in map.keys()) {
    ....
}

But that requires resolving #8188 so it's something I'll revisit in the future

@RealyUniqueName RealyUniqueName merged commit 517f0a4 into HaxeFoundation:development Sep 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-es6 ES6 platform-javascript Everything related to JS / JavaScript standard library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants