Implement Array.prototype.find()

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Antoine Quint]

I've recently stumbled upon the Array.prototype.find() method proposed in Harmony while looking for a way to establish which of the values contained in an array was found in a string first. Mozilla implements this method, and it would be nice for JSC to implement it too.

[Antoine Quint]

Created attachment 228259 [details]
Patch

[Oliver Hunt]

Comment on attachment 228259 [details]
Patch

Please add tests for 0 vs undefined vs false vs null vs "" -- we want tests that cover the specific kind of equality.  Also perf numbers relative to v8 and SM would be good.

Test cases for array modification during search, 

Both test cases need to be bigger and should be using the shouldBe() shouldBeTrue(), etc functions.  In general js/regress is for performancy tests where we're trying to measure speed and jit behaviour.  and they use throw on incorrect result as they need to ensure that the jit's don't DCE the entire test.

So r-, you need more tests.  I suspect the filter, etc tests in js/ should give a good example of expected behaviour.

[Antoine Quint]

Thanks for the feedback, I had a feeling I wasn't writing the right kind of tests. V8 doesn't implement those methods.

[Antoine Quint]

Created attachment 230067 [details]
Patch

[Antoine Quint]

Created attachment 230068 [details]
Patch

[Darin Adler]

Comment on attachment 230068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230068&action=review

> Source/JavaScriptCore/builtins/Array.prototype.js:195
> +        throw new @TypeError("Array.prototype.find requires that |this| not be null");

I don’t see any test coverage of this.

> Source/JavaScriptCore/builtins/Array.prototype.js:198
> +        throw new @TypeError("Array.prototype.find requires that |this| not be undefined");

I don’t see any test coverage of this.

> Source/JavaScriptCore/builtins/Array.prototype.js:204
> +        throw new @TypeError("Array.prototype.find callback must be a function");

I don’t see any test coverage of this.

> Source/JavaScriptCore/builtins/Array.prototype.js:217
> +        throw new @TypeError("Array.prototype.findIndex requires that |this| not be null");

I don’t see any test coverage of this.

> Source/JavaScriptCore/builtins/Array.prototype.js:220
> +        throw new @TypeError("Array.prototype.findIndex requires that |this| not be undefined");

I don’t see any test coverage of this.

> Source/JavaScriptCore/builtins/Array.prototype.js:226
> +        throw new @TypeError("Array.prototype.findIndex callback must be a function");

I don’t see any test coverage of this.

[Oliver Hunt]

Comment on attachment 230068 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=230068&action=review

r=me, but add edge case tests before landing

>> Source/JavaScriptCore/builtins/Array.prototype.js:198
>> +        throw new @TypeError("Array.prototype.find requires that |this| not be undefined");
> 
> I don’t see any test coverage of this.

Add .find.call(...) tests

>> Source/JavaScriptCore/builtins/Array.prototype.js:204
>> +        throw new @TypeError("Array.prototype.find callback must be a function");
> 
> I don’t see any test coverage of this.

coverage test

>> Source/JavaScriptCore/builtins/Array.prototype.js:226
>> +        throw new @TypeError("Array.prototype.findIndex callback must be a function");
> 
> I don’t see any test coverage of this.

ditto

[Oliver Hunt]

Oh, also add tests for arrays with holes

[Antoine Quint]

Created attachment 230148 [details]
Patch for landing

[Antoine Quint]

Patch for landing should have the exceptions and array with holes covered. Thanks for the review!

[WebKit Commit Bot]

Comment on attachment 230148 [details]
Patch for landing

Clearing flags on attachment: 230148

Committed r167797: <http://trac.webkit.org/changeset/167797>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 230148 [details]
Patch for landing

View in context: https://bugs.webkit.org/attachment.cgi?id=230148&action=review

> 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.

I'm guessing a test for this would be:

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

I could be wrong though. I didn't test in Firefox to see what they do.

[Antoine Quint]

*** Bug 131698 has been marked as a duplicate of this bug. ***

[Antoine Quint]

*** Bug 131702 has been marked as a duplicate of this bug. ***

[Antoine Quint]

*** Bug 131701 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

> I could be wrong though. I didn't test in Firefox to see what they do.

Just tested in Firefox, and they do -1 and 50. We do 0 and 0. Filed:
<https://webkit.org/b/132658> Array.prototype.find and findIndex should skip holes