Review comments

[anba]

FlattenIntoArray:

• The original argument of FlattenIntoArray isn't actually needed. It's passed to the Call in step 3.c.ii.2, but at the point original and source always point to the same object and I think it's even more clear to pass source instead of original to Call so it matches the sourceIndex argument.
• If step 3.c.vi (Increase targetIndex by 1.) is moved to 3.c.v.3, step 3.c.iv.3 (Set targetIndex to nextIndex - 1.) can be removed. And it also seems clearer to me if targetIndex is only incremented directly after calling CreateDataPropertyOrThrow.
• If step 3.c.iii (Let flattenable be ? IsArray(element).) is only executed if depth > 0 is true, engines can more easily optimize the code, because it's no longer necessary to inspect every array element. (Consider the case when an array element is a revoked proxy. Calling IsArray throws for revoked proxies, so if we don't call IsArray, we can simply copy all array elements for depth = 0. With the current algorithm we need to call IsArray even though we don't actually use the result for depth = 0.)

Array.prototype.flatMap:

• The other Array.prototype functions seem to invoke ? ToLength(? Get(O, "length")) before checking if the callback function is callable. So make step 4 the second step 2 for consistency?
• I'm not sure about step 7 (Perform ? Set(A, "length", nextIndex, true).). It looks like it was added per #41, but some of the comparisons are a bit moot, because the referenced functions either need to preserve holes or shrink the array, and that's why the array length is set, but both aspects don't apply to flatMap. And Array.prototype.filter and Array.prototype.map, which are both more akin to flatMap, don't explicitly set the array length at the end. (And now I'm wondering why Array.from and Array.of are actually explicitly setting the array length... :confused: )

Array.prototype.flatten:

• Similar to the issue in flatMap, make step 4 the second step.
• Similar to the issue in flatMap, step 7 doesn't look necessary to me

[ljharb]

Is there any chance you’d be willing to make these suggested changes as PRs to https://github.com/es-shims/Array.prototype.flatMap and https://github.com/es-shims/Array.prototype.flatten, which are coded in such a way that they match the spec steps exactly? The test cases have already helped me catch spec bugs; many of the changes you’re talking about may be perfectly reasonable, but it’s really hard for me to reason about a recursive algorithm correctly.

[anba]

If there are questions about the changes mentioned above, it's probably easier for others if the proposed changes are given as a diff against the proposal text than against a specific implementation. So here you go:

diff --git a/proposal.html b/proposal.html
index c266829..5476f1e 100644
--- a/proposal.html
+++ b/proposal.html
@@ -18,8 +18,7 @@ contributors: Michael Ficarra and Brian Terlson
     1. Let _O_ be ? ToObject(*this* value).
+    1. Let _sourceLen_ be ? ToLength(? Get(_O_, `"length"`)).
     1. If IsCallable(_mapperFunction_) is *false*, throw a *TypeError* exception.
     1. If _thisArg_ is present, let _T_ be _thisArg_; else let _T_ be *undefined*.
-    1. Let _sourceLen_ be ? ToLength(? Get(_O_, `"length"`)).
     1. Let _A_ be ? ArraySpeciesCreate(_O_, 0).
-    1. Let _nextIndex_ be ? FlattenIntoArray(_A_, _O_, _O_, _sourceLen_, 0, 1, _mapperFunction_, _T_).
-    1. Perform ? Set(_A_, *"length"*, _nextIndex_, *true*).
+    1. Perform ? FlattenIntoArray(_A_, _O_, _sourceLen_, 0, 1, _mapperFunction_, _T_).
     1. Return _A_.
@@ -32,2 +31,3 @@ contributors: Michael Ficarra and Brian Terlson
     1. Let _O_ be ? ToObject(*this* value).
+    1. Let _sourceLen_ be ? ToLength(? Get(_O_, `"length"`)).
     1. Let _depthNum_ be 1.
@@ -35,6 +35,4 @@ contributors: Michael Ficarra and Brian Terlson
       1. Set _depthNum_ to ? ToInteger(_depth_).
-    1. Let _sourceLen_ be ? ToLength(? Get(_O_, `"length"`)).
     1. Let _A_ be ? ArraySpeciesCreate(_O_, 0).
-    1. Let _nextIndex_ be ? FlattenIntoArray(_A_, _O_, _O_, _sourceLen_, 0, _depthNum_).
-    1. Perform ? Set(_A_, *"length"*, _nextIndex_, *true*).
+    1. Perform ? FlattenIntoArray(_A_, _O_, _sourceLen_, 0, _depthNum_).
     1. Return _A_.
@@ -44,3 +42,3 @@ contributors: Michael Ficarra and Brian Terlson
 <emu-clause id="sec-FlattenIntoArray" aoid="FlattenIntoArray">
-  <h1>FlattenIntoArray(_target_, _original_, _source_, _sourceLen_, _start_, _depth_ [ , _mapperFunction_, _thisArg_ ])</h1>
+  <h1>FlattenIntoArray(_target_, _source_, _sourceLen_, _start_, _depth_ [ , _mapperFunction_, _thisArg_ ])</h1>
   <emu-alg>
@@ -55,8 +53,10 @@ contributors: Michael Ficarra and Brian Terlson
           1. Assert: _thisArg_ is present.
-          1. Set _element_ to ? Call(_mapperFunction_, _thisArg_ , &laquo; _element_, _sourceIndex_, _original_ &raquo;).
-        1. Let _flattenable_ be ? IsArray(_element_).
-        1. If _flattenable_ is *true* and _depth_ &gt; 0, then
+          1. Set _element_ to ? Call(_mapperFunction_, _thisArg_ , &laquo; _element_, _sourceIndex_, _source_ &raquo;).
+        1. If _depth_ &gt; 0, then
+          1. Let _flattenable_ be ? IsArray(_element_).
+        1. Else,
+          1. Let _flattenable_ be *false*.
+        1. If _flattenable_ is *true*, then
           1. Let _elementLen_ be ? ToLength(? Get(_element_, `"length"`)).
-          1. Let _nextIndex_ be ? FlattenIntoArray(_target_, _original_, _element_, _elementLen_, _targetIndex_, _depth_ - 1).
-          1. Set _targetIndex_ to _nextIndex_ - 1.
+          1. Set _targetIndex_ to ? FlattenIntoArray(_target_, _element_, _elementLen_, _targetIndex_, _depth_ - 1).
         1. Else,
@@ -64,3 +64,3 @@ contributors: Michael Ficarra and Brian Terlson
           1. Perform ? CreateDataPropertyOrThrow(_target_, ! ToString(_targetIndex_), _element_).
-        1. Increase _targetIndex_ by 1.
+          1. Increase _targetIndex_ by 1.
       1. Increase _sourceIndex_ by 1.

[ljharb]

Thanks, that works too :-)

When I apply those changes to my implementations, tests that previously passed now fail.

Specifically, the source argument in FlattenToArray seems to be necessary when flattening "not the top level array".

I think the change to hoist the length check is great; I'll make that PR now.

For the isArray check, wouldn't we want a revoked proxy to throw unconditionally?

[anba]

Specifically, the source argument in FlattenToArray seems to be necessary when flattening "not the top level array".

The diff removes the original argument from FlattenToArray.

[ljharb]

Sorry, that's what I mean - they need to be two distinct objects, because in the nested cases, they're not the same - they're only the same when called from the top level.

[anba]

But original is only used in step 3.c.ii.2 for the Call(...) (ignoring when it's passed to the recursive FlattenIntoArray call). And the Call(...) is only executed when mapperFunction is present. And mapperFunction is only present for the initial call to FlattenIntoArray from Array.prototype.flatMap. And for the initial call to FlattenIntoArray, original and source are the same object.

[ljharb]

https://github.com/tc39/proposal-flatMap/blob/master/proposal.html#L60 applies with or without a mapper function - if https://github.com/tc39/proposal-flatMap/blob/master/proposal.html#L53 sets source to an array in flatMap on the first pass (when depth is 1), or, in flatten whenever the depth is > 0, then it would be a different object.

[anba]

Sorry, but I don't understand your last comment. Here's what I had in mind:

diff --git a/implementation.js b/implementation.js
index 38e2a38..751dc9e 100644
--- a/implementation.js
+++ b/implementation.js
@@ -5,13 +5,13 @@ var ES = require('es-abstract/es2017');
 var MAX_SAFE_INTEGER = Number.MAX_SAFE_INTEGER || (Math.pow(2, 53) - 1);

 // eslint-disable-next-line max-params
-var FlattenIntoArray = function FlattenIntoArray(target, original, source, sourceLen, start, depth) {
+var FlattenIntoArray = function FlattenIntoArray(target, source, sourceLen, start, depth) {
    var targetIndex = start;
    var sourceIndex = 0;

    var mapperFunction;
-   if (arguments.length > 6) {
-       mapperFunction = arguments[6];
+   if (arguments.length > 5) {
+       mapperFunction = arguments[5];
    }

    while (sourceIndex < sourceLen) {
@@ -20,15 +20,15 @@ var FlattenIntoArray = function FlattenIntoArray(target, original, source, sourc
        if (exists) {
            var element = ES.Get(source, P);
            if (typeof mapperFunction !== 'undefined') {
-               if (arguments.length <= 7) {
+               if (arguments.length <= 6) {
                    throw new TypeError('Assertion failed: thisArg is required when mapperFunction is provided');
                }
-               element = ES.Call(mapperFunction, arguments[7], [element, sourceIndex, original]);
+               element = ES.Call(mapperFunction, arguments[6], [element, sourceIndex, source]);
            }
            var spreadable = ES.IsArray(element);
            if (spreadable && depth > 0) {
                var elementLen = ES.ToLength(ES.Get(element, 'length'));
-               var nextIndex = FlattenIntoArray(target, original, element, elementLen, targetIndex, depth - 1);
+               var nextIndex = FlattenIntoArray(target, element, elementLen, targetIndex, depth - 1);
                targetIndex = nextIndex - 1;
            } else {
                if (targetIndex >= MAX_SAFE_INTEGER) {
@@ -57,7 +57,7 @@ module.exports = function flatMap(callbackfn) {

    var sourceLen = ES.ToLength(ES.Get(O, 'length'));
    var A = ES.ArraySpeciesCreate(O, 0);
-   var nextIndex = FlattenIntoArray(A, O, O, sourceLen, 0, 1, callbackfn, T);
+   var nextIndex = FlattenIntoArray(A, O, sourceLen, 0, 1, callbackfn, T);
    ES.Set(A, 'length', nextIndex, true);
    return A;
 };

[ljharb]

aha, thank you :-) Indeed, that passes all my tests.

If @michaelficarra agrees, I'll post a PR that updates the spec here as well (or you're welcome to, since you already have the diff).

[michaelficarra]

Thanks for the review, @anba. Should be fixed by https://github.com/tc39/proposal-flatMap/compare/3d057759...9da5bfde5. Let me know if that looks good to you. I didn't accept the change to not call IsArray if depth <= 0. If you feel strongly about it, we can ask the editor what would be best.