test262: test262/test/built-ins/Object/getOwnPropertyNames/15.2.3.4-4-44.js

[Joseph Pecoraro]

test262/test/built-ins/Object/getOwnPropertyNames/15.2.3.4-4-44.js

Boils down to:

  var str = new String("abc");
  str[5] = "d";
  print( Object.getOwnPropertyNames(str) )

Expected:
['0', '1', '2', '5', 'length']

Actual:
['0', '1', '2', 'length', '5']

Index Properties should be before Non-Index properties.

Notes:
- Chrome and Firefox pass the test
- Edge passes the test but mixes up the order of non-Index properties which should be ordered by insertion order (length, ...)

Spec:

19.1.2.9 Object.getOwnPropertyNames ( O )
https://tc39.github.io/ecma262/#sec-object.getownpropertynames

> 1. Return ? GetOwnPropertyKeys(O, String).

19.1.2.10.1 Runtime Semantics: GetOwnPropertyKeys ( O, Type )
https://tc39.github.io/ecma262/#sec-getownpropertykeys

> 1. Let obj be ? ToObject(O).
> 2. Let keys be ? obj.[[OwnPropertyKeys]]().
>   ...

9.4.3.3 String exotic object [[OwnPropertyKeys]]
https://tc39.github.io/ecma262/#sec-string-exotic-objects-ownpropertykeys

> 1. Let keys be a new empty List.
> 2. Let str be the String value of O.[[StringData]].
> 3. Let len be the number of elements in str.
> 4. For each integer i starting with 0 such that i < len, in ascending order, do
>   a. Add ! ToString(i) as the last element of keys.
> 5. For each own property key P of O such that P is an integer index and ToInteger(P) ≥ len, in ascending numeric index order, do
>   a. Add P as the last element of keys.
> 6. For each own property key P of O such that Type(P) is String and P is not an integer index, in ascending chronological order of property creation, do
>   a. Add P as the last element of keys.
> 7. For each own property key P of O such that Type(P) is Symbol, in ascending chronological order of property creation, do
>   a. Add P as the last element of keys.
> 8. Return keys.

The important bits here are (4) and (5) say add the Indices first. (6) properties in ascending order.

So I'd expect something like:

    var str = new String("abc");
    str.bar = "baz";
    str[999] = "d";
    Object.getOwnPropertyNames(str); // ['0', '1', '2', '999', 'length', 'bar']

[Joseph Pecoraro]

Created attachment 307209 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 307209 [details]
[PATCH] Proposed Fix

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

> LayoutTests/js/Object-getOwnPropertyNames-expected.txt:20
> +PASS getSortedOwnPropertyNames((function(){var x=new String('abc');x.bar='baz';x[999]='d';return x;})()) is ['0', '1', '2', '999', 'bar', 'length']

Unfortunately this test sorts the order, so the actual exact order I'd expect is:

    '0', '1', '2', '999', 'bar', 'length'

I can extend this test to test the exact order.

> Source/JavaScriptCore/runtime/StringObject.cpp:168
> -    return JSObject::getOwnPropertyNames(thisObject, exec, propertyNames, mode);
> +    return JSObject::getOwnNonIndexPropertyNames(thisObject, exec, propertyNames, mode);

I've always found it kind of weird that we explicitly use the use the base class name instead of just doing "Base::methodName(...)". If the base class were to ever change, all this code using the explicit name would need to change. I kept the existing style, but I'm open to making this the more explicit "Base::".

[Joseph Pecoraro]

Created attachment 307212 [details]
[PATCH] Proposed Fix

Rebaselined ChakraCore test results. Order improvement!

[Joseph Pecoraro]

Created attachment 307214 [details]
[PATCH] Proposed Fix

Rebaselined on trunk

[Saam Barati]

Comment on attachment 307214 [details]
[PATCH] Proposed Fix

LGTM

[WebKit Commit Bot]

Comment on attachment 307214 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 307214

Committed r215400: <http://trac.webkit.org/changeset/215400>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.