Bug 143980 – Implement `Object.assign`

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Jordan Harband]

Implment https://people.mozilla.org/~jorendorff/es6-draft.html#sec-object.assign

[Jordan Harband]

Created attachment 251220 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 251220 [details]
Patch

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

I've commented :) Overall, builtin script cannot rely on the ECMAScript's runtime function.
Writing the implementation with ECMAScript's syntax functionality is needed.

> Source/JavaScriptCore/builtins/ObjectConstructor.js:26
> +function isEnumerableOn(obj) {

In builtin scripts, we cannot define other functions. If we define it, it will be exposed as `Object.isEnumerableOn`. So implementing the whole code inside one `function assign` is needed, this is the current limitation.
It is needed to be fixed in the future.

> Source/JavaScriptCore/builtins/ObjectConstructor.js:36
> +    var objTarget = Object(target);

In builtin scripts, you can't reference the ordinal global variables like `TypeError`, `Object`.
This is because, if the malicious user rewrite global.Object, `assign` function will be affected.

To avoid to reference the ordinal global object's property, builtin scripts uses private symbols.
You can reference the ordinal `Object` by the private reference `@Object`. This syntax (@ prefixed) is only allowed in the builtins.

You can find private symbols use cases in the other builtins scripts :)
And these private symbols are exposed here. http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/runtime/JSGlobalObject.cpp#L431

> Source/JavaScriptCore/builtins/ObjectConstructor.js:38
> +    for (s = 1; s < arguments.length; ++s) {

I think considering "step 5-a, If nextSource is undefined or null, let keys be an empty List" is needed.

> Source/JavaScriptCore/builtins/ObjectConstructor.js:40
> +        props = Object.keys(source);

`Object.keys` also references `Object`. So exposing `Object.keys` as `@objectKeys` is necessary I think.

> Source/JavaScriptCore/builtins/ObjectConstructor.js:41
> +        if (typeof Object.getOwnPropertySymbols === 'function') {

Ditto.

> Source/JavaScriptCore/builtins/ObjectConstructor.js:42
> +            props.push.apply(props, Object.getOwnPropertySymbols(source).filter(isEnumerableOn(source)));

You cannot use `push`, `apply`, `filter` and so on.
Because the user may override it with user defined functions.

Just writing it like the following.

props = @objectKeys(source);
for (i = 0, iz = props.length; i < iz; ++i) {
    var prop = props[i];
    var value = source[prop];
    objTarget[prop] = value;
}

And perform one more similar loop for @objectGetOwnPropertySymbols.

[Build Bot]

Comment on attachment 251220 [details]
Patch

Attachment 251220 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6376311794696192

New failing tests:
sputnik/Conformance/15_Native_Objects/15.2_Object/15.2.3/15.2.3.1_Object.prototype/S15.2.3.1_A3.html
js/Object-assign.html

[Build Bot]

Created attachment 251233 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 251220 [details]
Patch

Attachment 251220 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6657786771406848

New failing tests:
sputnik/Conformance/15_Native_Objects/15.2_Object/15.2.3/15.2.3.1_Object.prototype/S15.2.3.1_A3.html
js/Object-assign.html

[Build Bot]

Created attachment 251235 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Jordan Harband]

Created attachment 251368 [details]
Patch

[Benjamin Poulain]

Any idea why GTK does not build?

[Benjamin Poulain]

Comment on attachment 251368 [details]
Patch

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

> Source/JavaScriptCore/builtins/ObjectConstructor.js:2
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

You should put your own copyright here instead of Apple.

> Source/JavaScriptCore/builtins/ObjectConstructor.js:29
> +    if (target == null)

Can't this be "==="?

> Source/JavaScriptCore/builtins/ObjectConstructor.js:33
> +    var s, nextSource, from, i, keys, nextKey, desc;

Weird style.

> LayoutTests/js/script-tests/Object-assign.js:10
> +

I would assign all types for completness:
-double
-integer
-boolean
-Symbol
-NaN
-Inf
--Inf

Bugs hide in the weirdest places.

> LayoutTests/js/script-tests/Object-assign.js:23
> +shouldBeTrue("var target = {}, source = {}; Object.defineProperty(source, 'a', { value: 1, enumerable: true }); Object.assign(target, {a: 2}, source); target.a === 1");

Maybe also cover that hasOwnProperty() return false when assigning with a single non-enumerable property?

[Jordan Harband]

Thanks for the review!

(In reply to comment #9)
> Comment on attachment 251368 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251368&action=review
> 
> > Source/JavaScriptCore/builtins/ObjectConstructor.js:2
> > + * Copyright (C) 2015 Apple Inc. All rights reserved.
> 
> You should put your own copyright here instead of Apple.
Will do.
> 
> > Source/JavaScriptCore/builtins/ObjectConstructor.js:29
> > +    if (target == null)
> 
> Can't this be "==="?

no, == null checks for both null and undefined, === null only checks for null.

> 
> > Source/JavaScriptCore/builtins/ObjectConstructor.js:33
> > +    var s, nextSource, from, i, keys, nextKey, desc;
> 
> Weird style.
Since JS vars don't have block scope, it's a common best practice to declare vars explicitly, not inside blocks. I can fix it though if you have a preference.
> 
> > LayoutTests/js/script-tests/Object-assign.js:10
> > +
> 
> I would assign all types for completness:
> -double
> -integer
> -boolean
> -Symbol
> -NaN
> -Inf
> --Inf
As target/sources? Sure, I'll add some test cases.

> Bugs hide in the weirdest places.
> 
> > LayoutTests/js/script-tests/Object-assign.js:23
> > +shouldBeTrue("var target = {}, source = {}; Object.defineProperty(source, 'a', { value: 1, enumerable: true }); Object.assign(target, {a: 2}, source); target.a === 1");
> 
> Maybe also cover that hasOwnProperty() return false when assigning with a
> single non-enumerable property?
Will do.

[Gyuyoung Kim]

When I build EFL build with this patch locally, there is no build issue. I suspect that EFL and GTK ews didn't generate *objectConstructorAssign* in DerivedSources/JavaScriptCore/ObjectConstructor.lut.h at that time. Anyway EFL and GTK ews have same build error. So GTK port looks fine with this patch probably. GTK folks need to check it as well.

[Benjamin Poulain]

(In reply to comment #11)
> When I build EFL build with this patch locally, there is no build issue. I
> suspect that EFL and GTK ews didn't generate *objectConstructorAssign* in
> DerivedSources/JavaScriptCore/ObjectConstructor.lut.h at that time. Anyway
> EFL and GTK ews have same build error. So GTK port looks fine with this
> patch probably. GTK folks need to check it as well.

Thanks Gyuyoung. Let's ignore the GTK and EFL EWS on this patch.

[Csaba Osztrogonác]

(In reply to comment #8)
> Any idea why GTK does not build?

Will check in 1-2 hours.

[Csaba Osztrogonác]

(In reply to comment #13)
> (In reply to comment #8)
> > Any idea why GTK does not build?

Unfortunately it is an incremental build issue. See the JSCBuiltins rule here:
http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/CMakeLists.txt#L1106

Generating JSCBuiltins.h and cpp depends on builtins/*.js
cmake generates the actual dependency list into the makefile,
but cmake doesn't regenerate the makefile after adding a new 
js file to the builtins directory.

There are many possibly solutions for this issue:
- add the list of the files to CMakeLists.txt instead of *.js 
(con: maintenance cost would be bigger)

- add a magic script which generates/updates a builtin-dependency.stamp
file only if the list was updated, and the generator rule should depend
on this new stamp file. (con: bigger task, but it can be the ultimate solution)

- touch CMakeLists.txt to regenerate the makefile (adding a new line at
the end) It is a little bit ugly, but the easiest way to fix this issue.

I suggest touching the CMakeLists.txt now to fix EFL and GTK builds and
I'm going to implement the stamp based fix in the near future to avoid
similar problems occure again and again.

[Jordan Harband]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #8)
> > > Any idea why GTK does not build?
> 
> Unfortunately it is an incremental build issue. See the JSCBuiltins rule
> here:
> http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/CMakeLists.
> txt#L1106
> 
> Generating JSCBuiltins.h and cpp depends on builtins/*.js
> cmake generates the actual dependency list into the makefile,
> but cmake doesn't regenerate the makefile after adding a new 
> js file to the builtins directory.
> 
> There are many possibly solutions for this issue:
> - add the list of the files to CMakeLists.txt instead of *.js 
> (con: maintenance cost would be bigger)
> 
> - add a magic script which generates/updates a builtin-dependency.stamp
> file only if the list was updated, and the generator rule should depend
> on this new stamp file. (con: bigger task, but it can be the ultimate
> solution)
> 
> - touch CMakeLists.txt to regenerate the makefile (adding a new line at
> the end) It is a little bit ugly, but the easiest way to fix this issue.
> 
> I suggest touching the CMakeLists.txt now to fix EFL and GTK builds and
> I'm going to implement the stamp based fix in the near future to avoid
> similar problems occure again and again.

Thanks, I'll do that.

[Jordan Harband]

Created attachment 251418 [details]
Patch

[Csaba Osztrogonác]

I fixed the incremental build issue in bug144094 , the CMakeLists.txt whitespace
workaround can be removed if the fix from bug144094 lands before this one.

[Brian Burg]

(In reply to comment #14)
> (In reply to comment #13)
> > (In reply to comment #8)

> - add a magic script which generates/updates a builtin-dependency.stamp
> file only if the list was updated, and the generator rule should depend
> on this new stamp file. (con: bigger task, but it can be the ultimate
> solution)
>
> I suggest touching the CMakeLists.txt now to fix EFL and GTK builds and
> I'm going to implement the stamp based fix in the near future to avoid
> similar problems occure again and again.

FWIW, the inspector ran into similar problems when different protocol specification files were used depending on feature flags passed to build-webkit. The DerivedSources.make version of the build now uses a forced rule and script to detect changes; search for "EnabledInspectorDomains". There is a script called UpdateContents.py which probably does what you want.

[WebKit Commit Bot]

Comment on attachment 251418 [details]
Patch

Clearing flags on attachment: 251418

Committed r183199: <http://trac.webkit.org/changeset/183199>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 251418 [details]
Patch

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

Cool! I think we can use this in Web Inspector. Some nits after looking at the patch.

> Source/JavaScriptCore/builtins/ObjectConstructor.js:26
> +function assign(target/*[*/, /*...*/sources/*] */) {

Yikes looks confusing. Given that Object.assign.length is already 2 this might read better as:

    function assign(target, /*...*/sources)

> Source/JavaScriptCore/builtins/ObjectConstructor.js:35
> +    var objTarget = @Object(target);
> +    var s, nextSource, from, i, keys, nextKey, desc;
> +    for (s = 1; s < arguments.length; ++s) {
> +        nextSource = arguments[s];

It is weird that this style doesn't really match up with the rest of WebKit's JavaScript style. We prefer descriptive variable names where possible. For instance "desc" could be "descriptor", "nextSource" could just be "source". Likewise we don't normally have up front "var" declarations, though I know it is a popular pattern. Imagine if they had been `let` instead of var. We typically inline them at first use, which tends to produce more concise code.

> Source/JavaScriptCore/builtins/ObjectConstructor.js:46
> +            keys = @objectKeys(from);
> +            for (i = 0; i < keys.length; ++i) {
> +                nextKey = keys[i];
> +                desc = @objectGetOwnPropertyDescriptor(from, nextKey);
> +                if (typeof desc !== "undefined" && desc.enumerable) {
> +                    objTarget[nextKey] = from[nextKey];
> +                }
> +            }

Object.keys (@objectKeys) is supposed to just return the enumerable own names:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-object.keys

If so, we should be able to avoid the GetOwnPropertyDescriptor and descriptor.enumerable checks entirely here. Is there a case I might be overlooking, or is this just to closely match the steps in the spec?

Nit: Instead of the typeof string comparison, you can just compare to undefined. (desc !== undefined)
Style: WebKit style is to not include braces around single line if statements.

> Source/JavaScriptCore/runtime/JSGlobalObject.cpp:433
> +    JSFunction* privateFuncObjectKeys = JSFunction::create(vm, this, 0, String(), objectConstructorKeys);
> +    JSFunction* privateFuncObjectGetOwnPropertyDescriptor = JSFunction::create(vm, this, 0, String(), objectConstructorGetOwnPropertyDescriptor);
> +    JSFunction* privateFuncObjectGetOwnPropertySymbols = JSFunction::create(vm, this, 0, String(), objectConstructorGetOwnPropertySymbols);

Is it expected that these are created with a 0 length here? Maybe they are different elsewhere? I would expect the lengths to be 1, 2, and 1.

[Jordan Harband]

(In reply to comment #21)
> Comment on attachment 251418 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=251418&action=review
> 
> Cool! I think we can use this in Web Inspector. Some nits after looking at
> the patch.
Thanks!

> > Source/JavaScriptCore/builtins/ObjectConstructor.js:26
> > +function assign(target/*[*/, /*...*/sources/*] */) {
> 
> Yikes looks confusing. Given that Object.assign.length is already 2 this
> might read better as:
> 
>     function assign(target, /*...*/sources)
In that case there'd be `sources` which is totally unused - I was attempting to mimic (sans comments) the spec notation for the optional argument.

> > Source/JavaScriptCore/builtins/ObjectConstructor.js:35
> > +    var objTarget = @Object(target);
> > +    var s, nextSource, from, i, keys, nextKey, desc;
> > +    for (s = 1; s < arguments.length; ++s) {
> > +        nextSource = arguments[s];
> 
> It is weird that this style doesn't really match up with the rest of
> WebKit's JavaScript style. We prefer descriptive variable names where
> possible. For instance "desc" could be "descriptor", "nextSource" could just
> be "source". Likewise we don't normally have up front "var" declarations,
> though I know it is a popular pattern. Imagine if they had been `let`
> instead of var. We typically inline them at first use, which tends to
> produce more concise code.
I used the same variable names that are in the spec - I totally agree that readability is better, but since the spec is the source of truth, I'd prefer (if there's no objections) to stick with the names they use, since anybody reading this code should probably always have a copy of the spec next to it :-)
As for the "var"s up front, I'm not heavily attached to that pattern, but since they *aren't* `let`s, I just usually declare them at the top. Would you like me to submit a patch changing that?

> > Source/JavaScriptCore/builtins/ObjectConstructor.js:46
> > +            keys = @objectKeys(from);
> > +            for (i = 0; i < keys.length; ++i) {
> > +                nextKey = keys[i];
> > +                desc = @objectGetOwnPropertyDescriptor(from, nextKey);
> > +                if (typeof desc !== "undefined" && desc.enumerable) {
> > +                    objTarget[nextKey] = from[nextKey];
> > +                }
> > +            }
> 
> Object.keys (@objectKeys) is supposed to just return the enumerable own
> names:
> https://people.mozilla.org/~jorendorff/es6-draft.html#sec-object.keys
> 
> If so, we should be able to avoid the GetOwnPropertyDescriptor and
> descriptor.enumerable checks entirely here. Is there a case I might be
> overlooking, or is this just to closely match the steps in the spec?
Both - any property could have a getter that modifies enumerability or existence of a property mid-iteration.

> 
> Nit: Instead of the typeof string comparison, you can just compare to
> undefined. (desc !== undefined)
`undefined` can be redefined everywhere but in the global scope, so I generally avoid ever using a literal undefined. I'll do that in the future though if you prefer.

> Style: WebKit style is to not include braces around single line if
> statements.
That's unfortunate, since some of those openssl bugs last year were caused/exacerbated by that very (dangerous) style :-) but I'll certainly comply in the future. Do you want me to make a patch that fixes this?

> > Source/JavaScriptCore/runtime/JSGlobalObject.cpp:433
> > +    JSFunction* privateFuncObjectKeys = JSFunction::create(vm, this, 0, String(), objectConstructorKeys);
> > +    JSFunction* privateFuncObjectGetOwnPropertyDescriptor = JSFunction::create(vm, this, 0, String(), objectConstructorGetOwnPropertyDescriptor);
> > +    JSFunction* privateFuncObjectGetOwnPropertySymbols = JSFunction::create(vm, this, 0, String(), objectConstructorGetOwnPropertySymbols);
> 
> Is it expected that these are created with a 0 length here? Maybe they are
> different elsewhere? I would expect the lengths to be 1, 2, and 1.
Here I followed the pattern that was already established - since these JS functions are solely exposed for private use within builtins, nothing will ever be checking their length except our builtin code. Between consistency with the actual functions, and consistency with other builtins + explicit indication that the length doesn't matter, I opted for the latter.