RESOLVED FIXED 143980
Implement `Object.assign`
https://bugs.webkit.org/show_bug.cgi?id=143980
Summary Implement `Object.assign`
Attachments
Patch (13.47 KB, patch)
2015-04-20 23:52 PDT, Jordan Harband
no flags
Archive of layout-test-results from ews103 for mac-mavericks (613.03 KB, application/zip)
2015-04-21 09:06 PDT, Build Bot
no flags
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (652.51 KB, application/zip)
2015-04-21 09:26 PDT, Build Bot
no flags
Patch (18.86 KB, patch)
2015-04-22 14:35 PDT, Jordan Harband
no flags
Patch (24.58 KB, patch)
2015-04-23 01:29 PDT, Jordan Harband
no flags
Jordan Harband
Comment 1 2015-04-20 23:52:54 PDT
Yusuke Suzuki
Comment 2 2015-04-21 00:17:53 PDT
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 3 2015-04-21 09:06:10 PDT
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
Comment 4 2015-04-21 09:06:13 PDT
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 5 2015-04-21 09:26:45 PDT
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
Comment 6 2015-04-21 09:26:48 PDT
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
Comment 7 2015-04-22 14:35:30 PDT
Benjamin Poulain
Comment 8 2015-04-22 21:36:35 PDT
Any idea why GTK does not build?
Benjamin Poulain
Comment 9 2015-04-22 21:51:48 PDT
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
Comment 10 2015-04-22 22:02:03 PDT
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
Comment 11 2015-04-22 22:39:28 PDT
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
Comment 12 2015-04-22 23:04:10 PDT
(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
Comment 13 2015-04-22 23:25:03 PDT
(In reply to comment #8) > Any idea why GTK does not build? Will check in 1-2 hours.
Csaba Osztrogonác
Comment 14 2015-04-23 00:37:21 PDT
(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
Comment 15 2015-04-23 00:53:24 PDT
(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
Comment 16 2015-04-23 01:29:57 PDT
Csaba Osztrogonác
Comment 17 2015-04-23 03:56:28 PDT
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
Comment 18 2015-04-23 11:02:38 PDT
(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 19 2015-04-23 11:45:26 PDT
Comment on attachment 251418 [details] Patch Clearing flags on attachment: 251418 Committed r183199: <http://trac.webkit.org/changeset/183199>
WebKit Commit Bot
Comment 20 2015-04-23 11:45:32 PDT
All reviewed patches have been landed. Closing bug.
Joseph Pecoraro
Comment 21 2015-04-23 16:51:01 PDT
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
Comment 22 2015-04-23 17:00:33 PDT
(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.
Note You need to log in before you can comment on or make changes to this bug.