911147 - Implement ES6 Array.prototype.fill

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Brandon Benvie [:benvie]]

Two new Array and TypedArray methods have been added in the latest ES6 spec draft (August 2013 revision).

* Array.prototype.fill(value, start = 0, end = this.length) - 15.4.3.29
  Fill all the indices of an array between `start` and `end` with `value. Returns `ToObject(this)`.

* Array.prototype.copyWithin(target, start, end = this.length) - 15.4.3.30
  This method copies values from one part of an array to another. `target` is where to start copying to. `start` is where to start copying from. `end` is where copying ends at.

Nearly identical methods are to be added to TypedArrays, with the spec noting: "%TypedArray%.prototype.fill is a distinct function that implements the same algorithm as Array.prototype.fill as defined in 15.4.3.23. However, the implementation of the algorithm may be optimized to assume that the this value is an object that has a fixed length and whose integer indexed properties are not sparse.". See 15.13.6.3.26 and 15.13.6.3.27.

Comment 1

[Till Schneidereit [:till]]

Attachment: Part 1: Implement Array.prototype.fill

Straight-forward self-hosted implementation. I hope I've covered all corner cases with the tests.

Comment 2

[Brandon Benvie [:benvie]]

Comment on attachment 8351463 [details] [diff] [review]
Part 1: Implement Array.prototype.fill

Review of attachment 8351463 [details] [diff] [review]:
-----------------------------------------------------------------

Looks mostly good.

::: js/src/builtin/Array.js
@@ +494,5 @@
> +    // Steps 1-2.
> +    var O = ToObject(this);
> +
> +    // Steps 3-5.
> +    var len = ToInteger(O.length);

Should probably add a FIXME pointing to bug 924058 ("Array operations should use ToLength").

Comment 3

[Jason Orendorff [:jorendorff]]

Note bug 934423. There is a massive comment in there by me listing things that could be tested... I'm afraid I may have killed the patch with that.

But we do want fairly thorough test coverage for this, because we want to optimize it.

Comment 4

[Till Schneidereit [:till]]

Oh, I didn't see bug 934423. Luckily, that deals with the method I didn't work on. :)

Jason, I'll look at which of the tests you propose apply to .fill and add them.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8351463 [details] [diff] [review]
Part 1: Implement Array.prototype.fill

Review of attachment 8351463 [details] [diff] [review]:
-----------------------------------------------------------------

Oh no, I had completely forgotten about this. Please don't let me leave reviews idle this long!

::: js/src/builtin/Array.js
@@ +489,5 @@
>      return -1;
>  }
>  
> +// ES6 draft 2013-11-08 22.1.3.6
> +function ArrayFill(value/*, start = 0, end = this.length*/) {

I think you can actually say `value, start = 0, end = undefined` here now, since the .length of the resulting function will be 1.

@@ +499,5 @@
> +
> +    // Steps 6-7.
> +    var relativeStart = arguments.length > 1 && arguments[1] !== undefined
> +                        ? ToInteger(arguments[1])
> +                        : 0;

var relativeStart = ToInteger(start);

@@ +509,5 @@
> +
> +    // Steps 9-10.
> +    var relativeEnd = arguments.length > 2 && arguments[2] !== undefined
> +                      ? ToInteger(arguments[2])
> +                      : len;

var relativeEnd = end === undefined ? len : ToInteger(end);

@@ +518,5 @@
> +                : std_Math_min(relativeEnd, len);
> +
> +    // Step 12.
> +    for (; k < final; k++) {
> +        O[k] = value;

Should be a strict-mode assignment (and this needs a test).

Is it possible for self-hosting code to do stuff like this?

    function strictAssign(O, p, v) {
        "use strict";
        O[p] = v;
    }

I ask in part because the Array.from implementation in bug 904723 uses a trick like that; if it doesn't work, please comment there too!

::: js/src/tests/ecma_6/Array/fill.js
@@ +29,5 @@
> +assertDeepEq([1,1,1].fill(2, undefined, 1), [2,1,1]);
> +assertDeepEq([1,1,1].fill(2, 2, 1), [1,1,1]);
> +assertDeepEq([1,1,1].fill(2, -1, 1), [1,1,1]);
> +assertDeepEq([1,1,1].fill(2, -2, 1), [1,1,1]);
> +assertDeepEq([1,1,1].fill(2, 1, -2), [1,1,1]);

Test these stupid cases:
- an argument is a non-integer number, -0, NaN, infinity, a string, an object with a .valueOf method
  (It's ugly that the spec for ToInteger says it can return infinity. That's not an integer. Neither is -0. Grr.)
- this is a string primitive
- this is null or undefined
- this is a frozen Array
- this is a Proxy with a suitable "set" handler
- this is a typed array and the value to be assigned is an object with a .valueOf method (it should be called multiple times).
- this is a Uint8ClampedArray and the value is out of range.

Comment 6

[Till Schneidereit [:till]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f26809e69199

I added lots of tests, including the ones you requested.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f26809e69199

Comment 8

[u279076]

Assuming this needs to QA testing due to the extensive automated tests which landed. Please needinfo me if this needs further testing before we release Firefox 31.

Comment 9

[u279076]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #8)
> Assuming this needs to QA testing due to the extensive automated tests which
> landed. Please needinfo me if this needs further testing before we release
> Firefox 31.

Sorry, that should read "Assuming this need no QA testing..."

Comment 10

[ziyunfei]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/Array.prototype.fill()

Comment 11

[ziyunfei]

Moved to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/fill

Comment 12

[Till Schneidereit [:till]]

(In reply to 446240525 from comment #11)
> Moved to
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Array/fill

Very nice! Especially the polyfill, which I tweeted about here:
https://twitter.com/tschneidereit/status/455027227949039616

Comment 13

[Florian Scholz (Open Web Docs)]

Thanks for documenting this!

I've also added it to "Firefox 31 for developers":
https://developer.mozilla.org/en-US/Firefox/Releases/31#JavaScript

Comment 14

[Chris Peterson [:cpeterson]]

ES6's Array.prototype.fill is probably worth relnoting.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Thanks. Added in the release notes for 31.