Implement Array.prototype.at() #1613

nekevss · 2021-10-02T03:48:00Z

Hi!

This Pull Request closes #1596.

I implemented the Array.prototype.at() using the link to the ECMAScript feature provided on the issue. I also added to the method call for at() to the chain.

ECMAScript had Array.prototype.at() just in front of the Array.prototype.concat() method, so I placed the function for at prior to that method to follow the doc order.

Just as a note: this is my first pull request. So if there is anything wrong or something I missed, feel free to let me know!

jedel1043 · 2021-10-02T04:08:53Z

Thank you for your contribution!
The code currently doesn't compile, so I'd advise you to run cargo check and cargo clippy to fix your compilation errors :)
And in general, before committing anything you should always run those two commands to ensure it at least compiles, then you can check if the tests pass or fail in the CI we have implemented for PRs

…ay-prototype-at

nekevss · 2021-10-02T15:33:40Z

Hi! I updated the branch with fixes to some errors I was able to get cargo check gave me.

This fix compiled and successfully ran a test.js file on my computer for the implementation.

jedel1043 · 2021-10-02T16:33:13Z

Test262 conformance changes:

Test result	master count	PR count	difference
Total	80,930	80,930	0
Passed	33,313	33,335	+22
Ignored	15,898	15,898	0
Failed	31,719	31,697	-22
Panics	0	0	0
Conformance	41.16%	41.19%	+0.03%

Fixed tests (22):

test/built-ins/Array/prototype/at/returns-item.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/at/returns-item.js (previously Failed)
test/built-ins/Array/prototype/at/returns-undefined-for-holes-in-sparse-arrays.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/at/returns-undefined-for-holes-in-sparse-arrays.js (previously Failed)
test/built-ins/Array/prototype/at/name.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/at/name.js (previously Failed)
test/built-ins/Array/prototype/at/returns-undefined-for-out-of-range-index.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/at/returns-undefined-for-out-of-range-index.js (previously Failed)
test/built-ins/Array/prototype/at/index-argument-tointeger.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/at/index-argument-tointeger.js (previously Failed)
test/built-ins/Array/prototype/at/returns-item-relative-index.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/at/returns-item-relative-index.js (previously Failed)
test/built-ins/Array/prototype/at/return-abrupt-from-this.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/at/return-abrupt-from-this.js (previously Failed)
test/built-ins/Array/prototype/at/index-non-numeric-argument-tointeger-invalid.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/at/index-non-numeric-argument-tointeger-invalid.js (previously Failed)
test/built-ins/Array/prototype/at/index-non-numeric-argument-tointeger.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/at/index-non-numeric-argument-tointeger.js (previously Failed)
test/built-ins/Array/prototype/at/length.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/at/length.js (previously Failed)
test/built-ins/Array/prototype/at/prop-desc.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/at/prop-desc.js (previously Failed)

jedel1043

Just some recommendations to improve the code. Good job!

jedel1043 · 2021-10-02T16:36:08Z

boa/src/builtins/array/mod.rs

+        //2. let len be ? LengthOfArrayLike(O)
+        let len = obj.length_of_array_like(context)? as i64;
+        //3. let relativeIndex be ? ToIntegerOrInfinity(index)
+        let relative_index = args.get(0).unwrap().to_integer_or_infinity(context)?;


Suggested change

let relative_index = args.get(0).unwrap().to_integer_or_infinity(context)?;

let relative_index = args.get_or_undefined(0).to_integer_or_infinity(context)?;

This won't panic if at is called with no arguments, like so: [1, 2, 3, 4].at()

I noticed this literally like 15-20 minutes ago! Completely unaware that get_or_undefined existed! Definitely making this update.

jedel1043 · 2021-10-02T16:43:20Z

boa/src/builtins/array/mod.rs

+            //handle most likely impossible case of
+            //IntegerOrInfinity::NegativeInfinity || IntegerOrInfinity::PositiveInfinity
+            //by setting k to len which will return undefined below
+            _ => len,


Instead of relying on the condition below, maybe it would be better if we early returned here:

Suggested change

_ => len,

_ => return Ok(JsValue::undefined()),

Makes it easier to identify that only finite numbers are valid.

Definitely makes more sense to identify only finite numbers being valid

jedel1043 · 2021-10-02T16:47:46Z

boa/src/builtins/array/mod.rs

+    /// When at method takes desired integer as index and returns the value at given
+    /// index. Negative integer counts backwards. If index is invalid, the at method
+    /// returns undefined.


Could you clarify this comment? You could instead take the explanation of the function on MDN and paste it here, that should be enough to explain what this does.

Took the MDN description. I thought that one was the best too, but I was a bit hesitant to just grab it.

boa/src/builtins/array/mod.rs

nekevss · 2021-10-02T17:21:25Z

Added all the feedback above.

Let me know if there's any more changes! The match guard one was kind of cool not gonna lie.

Thanks for all the help and feedback! 😄

jedel1043

Looks good to me! Just a small change that you can ignore if you consider it less readable :)

jedel1043 · 2021-10-02T21:30:52Z

boa/src/builtins/array/mod.rs

+            IntegerOrInfinity::Integer(i) if i >= 0 && i < len => i,
+            //5. Else, let k be len + relativeIndex
+            //integer should be negative, so abs() and check if less than or equal to length of array
+            IntegerOrInfinity::Integer(i) if i.abs() <= len && i != len => len + i,


Suggested change

IntegerOrInfinity::Integer(i) if i.abs() <= len && i != len => len + i,

IntegerOrInfinity::Integer(i) if i < 0 && i.abs() <= len => len + i,

I think this describes the check a lot better.

Totally agree! pushed the change to the second arm of the match. Again, thanks for all the feedback!

jedel1043 · 2021-10-02T21:43:20Z

We usually wait for two approved reviews until a PR is merged into master, so you'll just have to wait a bit of time and your contribution should be merged soon 😄

nekevss added 7 commits October 1, 2021 22:08

Initial commit of comments and function

f8b1997

Adding ECMAScript step comment outlines

b2a835f

code written for all steps - comments reformatted

270508d

Added extra | to comment

7464179

Cleaning up comments on match _ arm

be10c9f

Updated top level comment block

db5725a

Added at method to constructor's prototype at line 89

07aebb9

jedel1043 changed the title ~~Implementation of Array.prototype.at() #1596~~ Implement Array.prototype.at() Oct 2, 2021

jedel1043 added this to the v0.14.0 milestone Oct 2, 2021

jedel1043 added builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request labels Oct 2, 2021

nekevss added 3 commits October 2, 2021 10:43

Merge branch 'master' of https://github.com/boa-dev/boa into impl-arr…

0096fd5

…ay-prototype-at

Fixing some errors involving len being usize and return type

138a755

changed relativeIndex to relative_index for rust

f9bb0ac

jedel1043 added the Hacktoberfest Hacktoberfest 2021 - https://hacktoberfest.digitalocean.com label Oct 2, 2021

return 0 index by default in the case of no args given

57b51ce

jedel1043 requested changes Oct 2, 2021

View reviewed changes

nekevss added 3 commits October 2, 2021 12:49

fixed comment on change for better readability

1a9c4a9

made changes according to review feedback

18e26e3

updated comment block lines for readability

224dc93

jedel1043 reviewed Oct 2, 2021

View reviewed changes

boa/src/builtins/array/mod.rs Outdated Show resolved Hide resolved

nekevss added 2 commits October 2, 2021 13:16

adding match guard update - removes need of step 6

e11bfc4

clean up comments on match guards

753f517

fixing potential match bug involving arg being len

1acc870

jedel1043 approved these changes Oct 2, 2021

View reviewed changes

logic update to match guard on second arm for readability

0a6d4eb

HalidOdat approved these changes Oct 3, 2021

View reviewed changes

HalidOdat merged commit 94992a4 into boa-dev:master Oct 3, 2021

nekevss deleted the impl-array-prototype-at branch October 5, 2021 22:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Array.prototype.at() #1613

Implement Array.prototype.at() #1613

nekevss commented Oct 2, 2021 •

edited by jedel1043

Loading

jedel1043 commented Oct 2, 2021 •

edited

Loading

nekevss commented Oct 2, 2021

jedel1043 commented Oct 2, 2021

jedel1043 left a comment

jedel1043 Oct 2, 2021

nekevss Oct 2, 2021

jedel1043 Oct 2, 2021

nekevss Oct 2, 2021

jedel1043 Oct 2, 2021

nekevss Oct 2, 2021

nekevss commented Oct 2, 2021

jedel1043 left a comment

jedel1043 Oct 2, 2021

nekevss Oct 3, 2021

jedel1043 commented Oct 2, 2021

	let relative_index = args.get(0).unwrap().to_integer_or_infinity(context)?;
	let relative_index = args.get_or_undefined(0).to_integer_or_infinity(context)?;

	IntegerOrInfinity::Integer(i) if i.abs() <= len && i != len => len + i,
	IntegerOrInfinity::Integer(i) if i < 0 && i.abs() <= len => len + i,

Implement Array.prototype.at() #1613

Implement Array.prototype.at() #1613

Conversation

nekevss commented Oct 2, 2021 • edited by jedel1043 Loading

jedel1043 commented Oct 2, 2021 • edited Loading

nekevss commented Oct 2, 2021

jedel1043 commented Oct 2, 2021

Test262 conformance changes:

jedel1043 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

Choose a reason for hiding this comment

nekevss commented Oct 2, 2021

jedel1043 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 commented Oct 2, 2021

nekevss commented Oct 2, 2021 •

edited by jedel1043

Loading

jedel1043 commented Oct 2, 2021 •

edited

Loading