Array.prototype.find and findIndex should skip holes

[Joseph Pecoraro]

From: <https://webkit.org/b/130966> Implement Array.prototype.find()

> Source/JavaScriptCore/builtins/Array.prototype.js:210
> +    for (var i = 0; i < length; i++) {
> +        if (callback.@call(thisArg, array[i], i, array))
> +            return array[i];
> +    }

Should we only call the callback if "i in array" for sparse arrays?
<http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.find>

Seems to indicate it only proceeds to calling the callback if the "i" was present:

. Repeat, while k < len
. Let Pk be ToString(k).
. Let kPresent be HasProperty(O, Pk).
. ReturnIfAbrupt(kPresent).
. If kPresent is true, then
.   . Let kValue be Get(O, Pk).
.   . ReturnIfAbrupt(kValue).
.   . Let testResult be the result of calling the [[Call]] internal method of predicate with T as thisArgument and a List containing kValue, k, and O as argumentsList.


Test:

<script>
function passUndefined(element, index, array) {
    return typeof element === "undefined";
}

arrayWithHoles = [];
arrayWithHoles[10] = 0;
arrayWithHoles[20] = null;
arrayWithHoles[30] = false;
arrayWithHoles[40] = "";
arrayWithHoles.findIndex(passUndefined); // -1
arrayWithHoles[50] = undefined;
arrayWithHoles.findIndex(passUndefined); // 50
</script>

Expected: -1, 50.
Saw: 0, 0

Firefox Nightly gives the expected values.

[Alexey Proskuryakov]

Is it a problem to expose Array.prototype.find() with this bug? I winder if it should be disabled until this is fixed.

[Geoffrey Garen]

Yes, let's disable it.

[Antoine Quint]

I'll look into this tomorrow!

[Joseph Pecoraro]

(In reply to comment #0)
> From: <https://webkit.org/b/130966> Implement Array.prototype.find()
> 
> > Source/JavaScriptCore/builtins/Array.prototype.js:210
> > +    for (var i = 0; i < length; i++) {
> > +        if (callback.@call(thisArg, array[i], i, array))
> > +            return array[i];
> > +    }
> 
> Should we only call the callback if "i in array" for sparse arrays?
> <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.find>

I really think all that is needed is:

    if (!(i in array))
       continue;

I just haven't had the time to try it.

[Oliver Hunt]

(In reply to comment #4)
> (In reply to comment #0)
> > From: <https://webkit.org/b/130966> Implement Array.prototype.find()
> > 
> > > Source/JavaScriptCore/builtins/Array.prototype.js:210
> > > +    for (var i = 0; i < length; i++) {
> > > +        if (callback.@call(thisArg, array[i], i, array))
> > > +            return array[i];
> > > +    }
> > 
> > Should we only call the callback if "i in array" for sparse arrays?
> > <http://people.mozilla.org/~jorendorff/es6-draft.html#sec-array.prototype.find>
> 
> I really think all that is needed is:
> 
>     if (!(i in array))
>        continue;
> 
> I just haven't had the time to try it.

That's all the needed -- the code should essentially match all the other array methods

[Antoine Quint]

That, and a test, I'll have a patch out within 24 hours.

[Antoine Quint]

For .find(), I was thinking of adding a test where we count the number of callbacks to check that we indeed only call the callback where there are entries.

[Oliver Hunt]

(In reply to comment #7)
> For .find(), I was thinking of adding a test where we count the number of callbacks to check that we indeed only call the callback where there are entries.

Look at the tests for other array prototype functions - you want to check the number and order of callbacks

[Antoine Quint]

Created attachment 231819 [details]
Patch

[Geoffrey Garen]

Comment on attachment 231819 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 231819 [details]
Patch

Clearing flags on attachment: 231819

Committed r169162: <http://trac.webkit.org/changeset/169162>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.