Fix some issues with Array.prototype.{find, findIndex} #174

sainaen · 2015-04-21T05:03:40Z

Done

Don't skip holes in sparse arrays:

var a = [];
a[10] = 1;
var l = 0;
a.find(predicate);
assertEquals(a.length, l);

Allow passing array itself as thisArg:

var a = [1, 2];
var o;
a.findIndex(function () { o = this; }, a);
assertEquals(a, o);

Allow passing any instance of Function, but do not allow RegExp-s:

var a = [21, 22, 23, 24];

// `InterpretedFunction` or `? extends NativeFunction`
assertEquals(a[0], a.find(function () { return true; }));
// `IdScriptableObject`
assertEquals(a[0], a.find(Object.prototype.toString));
assertEquals(a[0], a.find(String));
// `BoundFunction`
assertEquals(a[0], a.find((function () { return true; }).bind({})))
// `NativeRegExp`
assertThrows('Array.prototype.findIndex.apply(a, /\d+/)', TypeError);

Also, I've updated tests: fixed those that were not in line with latest ES2015 spec (e.g. about iterating over sparse arrays) and added some cases from Gecko's test suite.

Not done

I wasn't sure I'd have time to fix these in my short “sprint”, as each of them deserves its own separate issue. Additionally, some will require adding new language version or a flag.

Allow iterating over objects where numeric properties have custom getter, like this:

var count = 0;
var a = {get 0() { return count++; }, length: 1};
Array.prototype.find.call(a, (function () { return true; }));

When thisArg is not passed, this inside predicate should be undefined even in non-strict mode:

var o = -1;
[1,2].find(function () { o = this; });
assertEquals(undefined, o);

When called with thisObj set to null or undefined, TypeError should be thrown:

assertThrows('Array.prototype.find.call(null, function() { })', TypeError);
assertThrows('Array.prototype.find.call(undefined, function() { })', TypeError);
assertThrows('Array.prototype.find.apply(null, function() { }, [])', TypeError);
assertThrows('Array.prototype.find.apply(undefined, function() { }, [])', TypeError);

Right now, when fn.call(null) is executed, this for fn is set to global (as per ES3), but in ES2015 ToObject() should be called on whatever was passed as this and it will throw on null and undefined values.

//cc @eshepelyuk

gbrail · 2015-04-21T16:28:59Z

Ivan -- first of all, thanks!

Second of all, in the last few months we were able to find a new owner of
the official Rhino (me) and I've done some releases that incorporate
everything in "apigee Rhino." So I'm going to close out this fork and not
do any more work on it.

Can you re-submit these changes as a patch to the official Rhino repo:

https://github.com/mozilla/rhino

The code should be mostly, but not completely, the same.

Thanks!

On Mon, Apr 20, 2015 at 10:03 PM, Ivan Vyshnevskyi <notifications@github.com

wrote:

Fixes for apigee#24 apigee#24
Done

Don't skip holes in sparse arrays:

var a = [];
a[10] = 1;
var l = 0;
a.find(predicate);
assertEquals(a.length, l);

Allow passing array itself as thisArg:

var a = [1, 2];var o;
a.findIndex(function () { o = this; }, a);
assertEquals(a, o);

Allow passing any instance of Function, but do not allow RegExp-s:

var a = [21, 22, 23, 24];
// InterpretedFunction or ? extends NativeFunction
assertEquals(a[0], a.find(function () { return true; }));// IdScriptableObject
assertEquals(a[0], a.find(Object.prototype.toString));
assertEquals(a[0], a.find(String));// BoundFunction
assertEquals(a[0], a.find((function () { return true; }).bind({})))// NativeRegExp
assertThrows('Array.prototype.findIndex.apply(a, /\d+/)', TypeError);

Not done

I wasn't sure I'd have time to fix in my short “sprint”, as each of them
requires its own separate issue.

Allow iterating over objects where numeric properties have custom
getter, like this:

var count = 0;var a = {get 0() { return count++; }, length: 1};Array.prototype.find.call(a, (function () { return true; }));

When thisArg is not passed, this inside predicate should be
undefined even in non-strict mode:

var o = -1;
[1,2].find(function () { o = this; });
assertEquals(undefined, o);

When called with thisObj set to null or undefined, TypeError should
be thrown:

assertThrows('Array.prototype.find.call(null, function() { })', TypeError);
assertThrows('Array.prototype.find.call(undefined, function() { })', TypeError);
assertThrows('Array.prototype.find.apply(null, function() { }, [])', TypeError);
assertThrows('Array.prototype.find.apply(undefined, function() { }, [])', TypeError);

Right now, when fn.call(null) is executed, this for fn is set to global
(as per ES3), but in ES2015 ToObject() should be called that will throw

on null and undefined values.

You can view, comment on, or merge this pull request online at:

#174
Commit Summary

Fix and expand test cases for Array.prototype.{find, findIndex}

Allow passing any Function instance to Array.prototype.{find,
findIndex} (except RegExp-s)

Do not throw in Array.prototype.{find, findIndex} when thisArg ==
thisObj

Make Array.prototype.{find, findIndex} do not skip holes in sparse
arrays

File Changes

M src/org/mozilla/javascript/NativeArray.java
https://github.com/mozilla/rhino/pull/174/files#diff-0 (19)

M testsrc/jstests/harmony/array-find.js
https://github.com/mozilla/rhino/pull/174/files#diff-1 (442)

M testsrc/jstests/harmony/array-findIndex.js
https://github.com/mozilla/rhino/pull/174/files#diff-2 (228)

Patch Links:

https://github.com/mozilla/rhino/pull/174.patch

https://github.com/mozilla/rhino/pull/174.diff

—
Reply to this email directly or view it on GitHub
#174.

greg brail | apigee https://apigee.com/ | twitter @gbrail
http://twitter.com/gbrail

eshepelyuk · 2015-04-21T16:49:57Z

Congrats @gbrail !

sainaen · 2015-04-21T16:50:29Z

Yes, I saw your posts on the mailing list. (Btw, accept my late congratulations! Having motivated maintainer is a must for any OSS project and you're doing great job as far as I can tell from number of releases and PR-s merged. :)

I think my "Fixes for apigee#24" confused you (sorry for that) -- this PR is already created in mozilla/rhino repository.

eshepelyuk · 2015-04-27T20:05:58Z

@sainaen
Removed code was intended to check if functions are called on null or undefined.
Getting it back to NativeArray would help to uncomment the test cases.

     if ((Id_find == id || Id_findIndex == id) && thisArg == thisObj) {
          throw ScriptRuntime.typeError("Array.prototype method called on null or undefined");
     }

sainaen · 2015-04-28T03:43:05Z

@eshepelyuk
Well, not actually. It was checking that thisObj is not the same as thisArg, throwing exception for Array.prototype.find.call(null, f), because both thisObj and thisArg have global as value in this case. It would not throw for Array.prototype.find.call(null, f, []). And the main issue for me was that it would throw on const a = []; a.find(f, a);

Right now there are two problems:

Rhino always inserts global as thisObj when function called on null or undefined (as per ES3)
Array.prototype.* methods that expect optional thisArg, will use global when it is not provided or it's null/undefined.

I think fixing single case when these both are in play is not worth of really unexpected "Array.prototype method called on null or undefined" on const a = []; a.find(f, a); and we should introduce new language version where behaviour of Function.prototype.{call, apply} and all Array.prototype.* will change with regard to thisObj and thisArg.

eshepelyuk · 2015-04-28T04:43:09Z

Having 70% of spec implemented is better than having 65% implemented (numbers are arbitrary). So I see no sense of reducing functionality now. Btw I think we can apply the same approach to analyze if this argument is null or undefined as was used for the object where the function was called on, i.e. comparing with object returned by getTopLevelScope.

sainaen · 2015-04-30T04:20:07Z

Sorry, I have to disagree. First of all, it wasn't implementing a spec 5% more compared to this PR, because there's no rule in spec that disallows passing array itself as a thisArg and it still allowed passing null when thisArg is defined. Second, it was too inconsistent with other Array.prototype methods that, according to spec, should behave similarly.
Yes, we can check that thisObj is not the global, but why should iterating on global [].find.call(this, f); be forbidden?
I'd prefer to work on making Function.{call, apply} more ES2015 and not using global when this is undefined or null. This way we'll have a clear way to implement required check for all iterative methods on Array.prototype without hacks and unexpected behaviour.

gbrail · 2015-05-01T01:42:03Z

I'd love to hear (here or on the mailing list) if anyone else has an opinion on this issue about the handling of "this" in these functions.

eshepelyuk · 2015-05-05T19:49:14Z

@sainaen @gbrail

Point No 1.

First of all I'd like to get us back to the initial point of the discussion that I've raised. And this is not about how to treat this when its value null or undefined when passed to apply() or call() method of Function.prototype.
The problem I see with this PR is that it is intentionally reduces implementation scope of find and findIndex methods, especially compatibility with V8 engine test suites, without providing proper fixes or other appropriate approach to keep up with V8 tests.

I believe we never should reduce implementation scope, especially when it is already included in already released version of Rhino.
Instead, before approving this PR we should elaborate a solution for point 2

Point No 2. How to detect if `null` or `undefined` passed as `this`

I've been digging the code for the source of the issue and have discovered exact place where it's produced. Also I've been able to play with possible solutions and I could propose two ways to go

Introduce breaking change in Rhino for the next version and stop passing global scope when null or undefined is passed as this. Apparently this have to be raised in mailing list as open question to discuss. Additionally we could control this breaking change using language level.

After applying the fix - there not so much tests to fix, so implementation is easy.
Introduce workaround to org.mozilla.javascript.ScriptRuntime#applyOrCall method to mark global with special flag before passing it to function and cleanup flag after the call.
The really quick and dirty implementaiton you can check out at this branch (branhced from my PR about ES6 additions to String).
In ES6 additions to String branch I've introduced RequireObjectCoercible implementation with old approach that relies on analyzing if global scope equals to this. And in forked branch I'm using workaround with marker flag to properly analyze if null or undefined is passed.

The applied fix did not break any existing tests. So it's a cheap, quick and not breaking way to proceed with proper implementation of ECMA specification.

sainaen · 2015-05-07T21:33:01Z

The problem I see with this PR is that it is intentionally reduces implementation scope of find and findIndex methods, especially compatibility with V8 engine test suites, without providing proper fixes or other appropriate approach to keep up with V8 tests.

Sorry, I have to disagree on that (again). I wouldn't call removal of a particular hack that tries to hide one of the Rhino's existing issues for two specific functions, but also adds two new weird issues to them, a reduction of implementation scope. That part of the “scope” just wasn't there in the first place.

Apparently this have to be raised in mailing list as open question to discuss.

I don't see what to discuss here as it's what is required by the spec, isn't it?

Anyway, I'd prefer correct full implementation to workaround/hack almost any time, so I'd like to go with the first option.

About the order in which fixes should be applied — I think, with changes to how this is treated in call(null, ...) and apply(null, ...) a lot of methods of built-in object will need an update, so I don't see any reason to hold this PR from merging before that will be fixed and, in fact, would even prefer merge it first.

gbrail · 2015-06-16T21:29:45Z

Since this code is out there in 1.7.6, I would like to the special "this" handling for "find" and "findIndex" as it is for now.

I just added the constant "Context.VERSION_ES6" to the Context class so that we can start to add things that test for "ES6" compliance. Now, we had some debate about what exactly this means, so for the purposes of Rhino, it's going to mean that when set, things that changed in versions of JavaScript up to and including ES6 can be implemented, even if they break compatibility in some cases.

Both of you introduced the idea of using the new language level flag to test and add the new behavior of "this" and "thisArg" only when the language level is >= "ES6". I think that we should do that, which would make all the iterative methods on Array consistent.

Would either of you have time to make a patch that fixes that (and only that)?

In addition, the "do not skip holes in sparse arrays" bit seems orthogonal -- could we just take that patch for now? If you think it makes sense then I can cherry-pick what is in this PR.

Finally, I am going to try and do a release very soon, and none of this would make it in, but we can easily do a 1.7.8 or even a 1.8.0 in the near future.

eshepelyuk · 2015-06-17T00:08:46Z

Hello
Thank for finally putting some light on the issues discussed here.
I would suggest to not proceed with this PR as it looks now until ES6 handling of this will be implemented as you've described.
I can create a new github issue for that task and create pull request as soon as new flag available in master branch.

eshepelyuk · 2015-06-17T00:44:54Z

@gbrail, in fact, I would rather proceed with introducing a breaking change for this handling. Because such behaviour is not specific to ES6, but was already introduced in ES5, so Rhino already should support it.
What do you think ?

gbrail · 2015-06-17T00:55:21Z

I think it depends on what breaking change you are talking about...

There is so much Rhino code out in the world, I fear that any breaking
change to long-implemented Array methods (and other methods) could cause
unforeseen things to go wrong. I would only really want to do that if the
change was "protected" by a version flag. IMHO the "ES6" version flag is a
reasonable place to put this. Alternatively, there are lots of other flags
on Context, and Rhino has used them before to implement things like this --
we could also add a separate flag for this.

On the other hand, if you are just talking about changing the handling of
"this" on "find" and "findIndex" to work like the rest of the Array class,
then I think that would affect many fewer people since those functions are
so new to Rhino.

So how about this (a little complicated but probably safe):

Add a feature flag such as "FEATURE_OLD_THIS_HANDLING".
The default value is "true" for all releases <= VERSION_1_8, and
"false" for >= ES6.
Of course, people who have problems can use the ContextFactory to
change the flag if necessary.
ALL methods on Array, and others, are the same (no special handling
for find and findIndex)
All Array methods use the feature flag to determine which set of
behavior to use.

Strictly speaking, that feature flag should be "true" for only releases <=
1.7. But it sounds like we are talking about a lot of methods here and we
don't want to break lots of existing code, hence the set of flags.

On Tue, Jun 16, 2015 at 5:44 PM, Evgeny Shepelyuk notifications@github.com
wrote:

@gbrail https://github.com/gbrail, in fact, I would rather proceed with
introducing a breaking change for this handling. Because such behaviour
is not specific to ES6, but was already introduced in ES5, so Rhino already
should support it.
What do you think ?

—
Reply to this email directly or view it on GitHub
#174 (comment).

Greg Brail | apigee https://apigee.com/ | twitter @gbrail
http://twitter.com/gbrail @apigee https://twitter.com/apigee
http://iloveapis.com/

eshepelyuk · 2015-06-17T01:08:38Z

Introducing feature flag on Context sounds great to me. I would like to go this way.
Initial intention will be to try to set it to true for versions <= 1.7 and check if all goes right.
In case of problems - set to true for versions <= 1.8.
Does it make sense for you ? If we agree - I will proceed with PR creation.

gbrail · 2015-06-17T01:14:26Z

OK -- it would make sense to have the "old behavior" flag be true only for
<= 1.7. Let me know if you have any questions but if you follow the
existing patterns it shouldn't be too hard,.

On Tue, Jun 16, 2015 at 6:08 PM, Evgeny Shepelyuk notifications@github.com
wrote:

Introducing feature flag on Context sounds great to me. I would like to go
this way.
Initial intention will be to try to set it to true for versions <= 1.7 and
check if all goes right.
In case of problems - set to true for versions <= 1.8.
Does it make sense for you ? If we agree - I will proceed with PR creation.

—
Reply to this email directly or view it on GitHub
#174 (comment).

Greg Brail | apigee https://apigee.com/ | twitter @gbrail
http://twitter.com/gbrail @apigee https://twitter.com/apigee
http://iloveapis.com/

eshepelyuk · 2015-06-26T22:27:40Z

@gbrail, I think this PR was accidentaly auto-closed by 204, wasn't it ?

gbrail · 2015-06-26T22:31:54Z

Yes -- I didn't actually merge this one.

…ex} (except RegExp-s)

…rays According to ES2015 spec, sections 22.1.3.8 and 22.1.3.9, predicate should be called for every k from 0 to length, without checking if property with such index is defined (i.e. even for holes). See section 22.1.3.15 with algorithm for Array.prototype.map, where implementation is required to check HasProperty(O, k) on every iteration.

sainaen · 2015-06-28T07:39:11Z

Hi, I've rebased on current master and removed my changes related to handling this == null case.

Checked with the test262 — this PR fixes 4 tests:
3 about iterating over holes:

Array.prototype.find_treats-holes-as-undefined-not-skipped.js
Array.prototype.find_remove-after-start.js
Array.prototype.findIndex_treats-holes-as-undefined-not-skipped.js

1 about passing built-in function as a callback

Array.prototype.find_callable-forEach.js

Also, I've updated the check that disallows RegExps as callbacks to apply on the new ES6 language level for all Array.prototype methods, not only find and findIndex.

… this'

eshepelyuk · 2015-06-28T07:47:02Z

Btw, you could integrate those test262 cases into Rhino using Test262SuiteTest.java

sainaen · 2015-06-28T08:17:57Z

Yeah, I've used it for testing, but there are still ~168 failing tests and that's to much exclusions to add. I'd rather try to fix some, before doing it manually.

sainaen · 2015-06-29T17:30:21Z

Just in case:
Please, don't merge. Found a bug. Will fix in a moment.

Update: Done. Feel free to merge now. :)

gbrail · 2015-07-02T15:59:25Z

Rebased and merged the final code. Thanks!

sainaen · 2015-07-02T17:40:22Z

Great! Thank you.

fixes #174

sainaen force-pushed the array_find branch from 4ebe4e7 to 8a8a7ce Compare April 21, 2015 05:27

gbrail added this to the Release 1.7.8 milestone Jun 17, 2015

eshepelyuk mentioned this pull request Jun 18, 2015

ES5 handling of null and undefined passed as this to Function.prototype.call / Function.prototype.apply #204

Merged

gbrail closed this in #204 Jun 26, 2015

gbrail reopened this Jun 26, 2015

sainaen added 3 commits June 28, 2015 09:10

Fix and expand test cases for Array.prototype.{find, findIndex}

1c903e8

Allow passing any Function instance to Array.prototype.{find, findInd…

a6abe5e

…ex} (except RegExp-s)

sainaen force-pushed the array_find branch from 8a8a7ce to 3a79a96 Compare June 28, 2015 07:09

Unignore case for 'Array.prototype.{find,findIndex}' when 'thisArg ==…

69fd791

… this'

Fix handling of holes in case of Array.prototype.{find, findIndex}

3613a2d

gbrail closed this Jul 2, 2015

sainaen deleted the array_find branch July 2, 2015 17:40

rbri mentioned this pull request May 20, 2020

improve seal() and freeze() processing; #677

Merged

gbrail pushed a commit that referenced this pull request May 27, 2020

improve seal() and freeze() processing;

dc7e2a4

fixes #174

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix some issues with Array.prototype.{find, findIndex} #174

Fix some issues with Array.prototype.{find, findIndex} #174

sainaen commented Apr 21, 2015

gbrail commented Apr 21, 2015

on null and undefined values.

eshepelyuk commented Apr 21, 2015

sainaen commented Apr 21, 2015

eshepelyuk commented Apr 27, 2015

sainaen commented Apr 28, 2015

eshepelyuk commented Apr 28, 2015

sainaen commented Apr 30, 2015

gbrail commented May 1, 2015

eshepelyuk commented May 5, 2015

sainaen commented May 7, 2015

gbrail commented Jun 16, 2015

eshepelyuk commented Jun 17, 2015

eshepelyuk commented Jun 17, 2015

gbrail commented Jun 17, 2015

eshepelyuk commented Jun 17, 2015

gbrail commented Jun 17, 2015

eshepelyuk commented Jun 26, 2015

gbrail commented Jun 26, 2015

sainaen commented Jun 28, 2015

eshepelyuk commented Jun 28, 2015

sainaen commented Jun 28, 2015

sainaen commented Jun 29, 2015

gbrail commented Jul 2, 2015

sainaen commented Jul 2, 2015

Fix some issues with Array.prototype.{find, findIndex} #174

Fix some issues with Array.prototype.{find, findIndex} #174

Conversation

sainaen commented Apr 21, 2015

Done

Not done

gbrail commented Apr 21, 2015

on null and undefined values.

eshepelyuk commented Apr 21, 2015

sainaen commented Apr 21, 2015

eshepelyuk commented Apr 27, 2015

sainaen commented Apr 28, 2015

eshepelyuk commented Apr 28, 2015

sainaen commented Apr 30, 2015

gbrail commented May 1, 2015

eshepelyuk commented May 5, 2015

Point No 1.

Point No 2. How to detect if null or undefined passed as this

sainaen commented May 7, 2015

gbrail commented Jun 16, 2015

eshepelyuk commented Jun 17, 2015

eshepelyuk commented Jun 17, 2015

gbrail commented Jun 17, 2015

eshepelyuk commented Jun 17, 2015

gbrail commented Jun 17, 2015

eshepelyuk commented Jun 26, 2015

gbrail commented Jun 26, 2015

sainaen commented Jun 28, 2015

eshepelyuk commented Jun 28, 2015

sainaen commented Jun 28, 2015

sainaen commented Jun 29, 2015

gbrail commented Jul 2, 2015

sainaen commented Jul 2, 2015

Point No 2. How to detect if `null` or `undefined` passed as `this`