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

Implementation of for...of loops #704

Merged
merged 39 commits into from
Oct 2, 2020
Merged

Conversation

joshwd36
Copy link
Contributor

This Pull Request fixes/closes #576.

It changes the following:

  • Adds the array iterator object and values() function to arrays.
  • Sets the Symbol.iterator parameter to array pointing to the values() function. Also adds the get_well_known_symbol() function to Context.
  • Adds support for a limited subsection of for...of loops.

Currently for...of only works if the iterating variable is declared before the loop and is referenced without another declaration. e.g.

var i;
for (i of [1, 2, 3]) {
    console.log(i)
}

I'm unsure of the approach to take for implementing the rest of the loop. The main problem is that the initial part of a for statement (for (...) is treated differently depending on whether it is an ordinary for loop or a for...of/in loop. My current idea is to loop through each token until either a semicolon or in/of is found, collecting the tokens into a vec and using that as a cursor to parse the initial element depending on what was found. This does seem a bit hacky though, so I'd be open to other ideas.

A slightly more minor note is that I'm unsure of whether my implementation for get_well_known_symbol is correct as there seems to be a lot of indirection required to get the symbol object.

@joshwd36 joshwd36 marked this pull request as draft September 17, 2020 23:20
@codecov
Copy link

codecov bot commented Sep 17, 2020

Codecov Report

Merging #704 into master will increase coverage by 0.77%.
The diff coverage is 70.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #704      +/-   ##
==========================================
+ Coverage   58.51%   59.28%   +0.77%     
==========================================
  Files         152      156       +4     
  Lines        9476     9820     +344     
==========================================
+ Hits         5545     5822     +277     
- Misses       3931     3998      +67     
Impacted Files Coverage Δ
boa/src/builtins/mod.rs 22.58% <0.00%> (ø)
boa/src/object/gcobject.rs 73.10% <ø> (ø)
boa/src/object/mod.rs 27.42% <25.00%> (-0.25%) ⬇️
boa/src/value/mod.rs 70.19% <38.46%> (-1.10%) ⬇️
boa/src/syntax/ast/keyword.rs 47.36% <50.00%> (+0.05%) ⬆️
...syntax/ast/node/declaration/const_decl_list/mod.rs 58.97% <50.00%> (-5.74%) ⬇️
boa/src/syntax/ast/node/mod.rs 43.80% <50.00%> (-0.74%) ⬇️
...a/src/syntax/ast/node/iteration/for_of_loop/mod.rs 53.60% <53.60%> (ø)
boa/src/syntax/parser/statement/declaration/mod.rs 71.42% <60.00%> (-10.39%) ⬇️
...src/syntax/parser/statement/declaration/lexical.rs 71.23% <75.00%> (+1.39%) ⬆️
... and 21 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3cb8c94...875a383. Read the comment docs.

@HalidOdat HalidOdat added builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request labels Sep 18, 2020
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

This looks good! I just skimmed through the code I'll do a proper review later :)

boa/src/builtins/array/array_iterator.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/array_iterator.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/array_iterator.rs Outdated Show resolved Hide resolved
boa/src/builtins/array/array_iterator.rs Outdated Show resolved Hide resolved
boa/src/context.rs Outdated Show resolved Hide resolved
@joshwd36 joshwd36 changed the base branch from master to concurrent_parser September 23, 2020 21:37
@joshwd36 joshwd36 changed the base branch from concurrent_parser to master September 23, 2020 21:37
@joshwd36 joshwd36 changed the title Initial implementation of for...of loop (help wanted) Implementation of for...of loops Sep 23, 2020
@joshwd36 joshwd36 marked this pull request as ready for review September 23, 2020 22:46
@joshwd36
Copy link
Contributor Author

This now works on most types of for...of loop. The one thing I'm unsure of is changing the behaviour of const declarations, as it feels a bit less neat, but I couldn't think of a different way to do it while still having the lhs of the for...of loop be parsed in the same way as an ordinary for loop.

…out into their own file to allow for repeated use.
# Conflicts:
#	boa/src/builtins/array/array_iterator.rs
#	boa/src/builtins/array/mod.rs
#	boa/src/exec/iteration/mod.rs
@HalidOdat HalidOdat requested review from Razican and Lan2u September 25, 2020 19:18
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

This is almost ready to merge :)

boa/src/builtins/array/array_iterator.rs Outdated Show resolved Hide resolved
boa/src/builtins/iterable/mod.rs Outdated Show resolved Hide resolved
boa/src/builtins/string/string_iterator.rs Outdated Show resolved Hide resolved
@joshwd36
Copy link
Contributor Author

joshwd36 commented Oct 2, 2020

This is almost ready to merge :)

Great! In terms of implementing iterators for objects, there is still map to do. Should I implement this now (shouldn't take too long) or just leave it. Also, what should I do about the array index in the array iterator?

# Conflicts:
#	boa/src/exec/declaration/mod.rs
#	boa/src/exec/iteration/mod.rs
#	boa/src/exec/mod.rs
#	boa/src/syntax/ast/node/declaration.rs
#	boa/src/syntax/ast/node/iteration.rs
@HalidOdat
Copy link
Member

HalidOdat commented Oct 2, 2020

Should I implement this now (shouldn't take too long) or just leave it.

Sure. if you want we can implement it in this :)

Also, what should I do about the array index in the array iterator?

I think changing it to u32 is the best option, the array index can only be +0 ≤ i < 2^32 - 1 so it can fit. but i32 will overflow into negative

Edit: On second thought maybe we should merge this, and work on Map iterator on a different PR. This should unblock for..in #750

@HalidOdat HalidOdat requested a review from RageKnify October 2, 2020 20:44
@HalidOdat HalidOdat added execution Issues or PRs related to code execution parser Issues surrounding the parser labels Oct 2, 2020
@HalidOdat HalidOdat added this to the v0.11.0 milestone Oct 2, 2020
@joshwd36
Copy link
Contributor Author

joshwd36 commented Oct 2, 2020

I think changing it to u32 is the best option, the array index can only be +0 ≤ i < 2^32 - 1 so it can fit. but i32 will overflow into negative

Currently the only issue with that is that it sometimes has to be placed back into a value. Should that be done as the Number type rather than Integer then?

@HalidOdat
Copy link
Member

Currently the only issue with that is that it sometimes has to be placed back into a value. Should that be done as the Number type rather than Integer then?

If value can fit in an Value::Integer(i32) it will be placed but if not it will be converted to f64 (which it can fit) Value::Rational(f64), so we don't really have to worry how its stored. https://github.com/boa-dev/boa/blob/master/boa/src/value/conversions.rs#L76-L85

@RageKnify RageKnify merged commit 87d9e9c into boa-dev:master Oct 2, 2020
@joshwd36 joshwd36 deleted the Iterables branch October 2, 2020 22:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request execution Issues or PRs related to code execution parser Issues surrounding the parser
Projects
None yet
3 participants