Brandon Benvie [:benvie] Reporter
	Description • 11 years ago

As requested, this has been split off from bug 932375.

Object.assign (target, source) - 19.1.2.1

Copies the values of all of the enumerable own properties from a source to a target. Uses [[Get]] on the source and [[Put]] on the target, so will invoke getters/setters. Returns target. This function behaves similarly to how Object.defineProperties in that if an error is encountered while reading or writing properties, assignment continues and the error is thrown at the end.

	André Bargull [:anba]
	Comment 1 • 11 years ago

If wanted I can try to adjust my tests from [1] to run in SM. Apart from that those tests should give some hints what needs to be tested for Object.assign().

[1] https://github.com/anba/es6draft/blob/master/src/test/scripts/suite/objects/Object/assign.js

	Till Schneidereit [:till]
	Comment 3 • 11 years ago

I'm working on this

	Till Schneidereit [:till]
	Comment 4 • 11 years ago

(In reply to André Bargull from comment #1)
> If wanted I can try to adjust my tests from [1] to run in SM. Apart from
> that those tests should give some hints what needs to be tested for
> Object.assign().

That would be great! Can you attach a patch? Alternatively, I can do the import and add the BSD license to the file itself. I'd just ask you for a review of that patch, then.

	André Bargull [:anba]
	Comment 5 • 11 years ago

Which kind of format do you prefer: js-test or jit-test?

	Till Schneidereit [:till]
	Comment 6 • 11 years ago

While I prefer running jit-tests, this really belongs into js-test.

	André Bargull [:anba]
	Comment 7 • 11 years ago

This is more or less a direct conversion of that test file to be able to run in SpiderMonkey. There are a couple of FIXME's: one is a Proxy bug, but I can't tell if it's already reported somewhere. And the other one is related to the current [[OwnKeys]] ("getOwnPropertyNames" trap in SM) implementation in SpiderMonkey. Basically the current ES6 draft says that [[OwnKeys]] returns an Iterable object whereas SpiderMonkey currently uses an Array. But those tests only make sense for iterable objects, not for arrays. 

Open issues (?): Formatting could be improved and a bit of the assert* function boilerplate could be removed, too.

	André Bargull [:anba]
	Comment 8 • 11 years ago

Missed the other case where I need to workaround that Proxy issue.

	Till Schneidereit [:till]
	Comment 9 • 11 years ago

Awesome, thanks! I'll obviously remove the `Object.assign = function ...` part, but other than that, this looks pretty great.

	Till Schneidereit [:till]
	Comment 10 • 11 years ago

This passes all of André's tests. However, it's not clear to me that the implementation is correct wrt proxies. Additionally, both jorendorff and me posted to es-discuss with requests for changes to methods for reflecting on object properties. If either or both of these proposals are accepted, that'd change Object.assign's implementation.

	André Bargull [:anba]
	Comment 11 • 11 years ago

Comment on attachment 8343759 [details] [diff] [review]
Implement Object.assign. wip

Review of attachment 8343759 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Object.js
@@ +16,5 @@
> +  var keys = std_Object_getOwnPropertyNames(from);
> +
> +  // Step 7 (omitted).
> +  // Step 8.
> +  var pendingException = undefined;

This needs an additional flag 'hasPendingException' in case the thrown exception is `undefined`.

@@ +31,5 @@
> +      if (desc && desc.enumerable) {
> +        to[key] = from[key];
> +      }
> +    } catch (e) {
> +      if (pendingException === undefined)

`pendingException` may be `undefined` in which case it gets overwritten.

@@ +37,5 @@
> +    }
> +  }
> +
> +  // Step 10.
> +  if (pendingException)

`pendingException` may be falsy, another reason why a flag is necessary.

	Till Schneidereit [:till]
	Comment 12 • 11 years ago

(In reply to André Bargull from comment #11)
> > +  var pendingException = undefined;
> 
> This needs an additional flag 'hasPendingException' in case the thrown
> exception is `undefined`.

Thanks, excellent point. It seems somewhat absurd to throw `undefined`, but it's certainly possible. :(

	Brandon Benvie [:benvie] Reporter
	Comment 13 • 11 years ago

(In reply to André Bargull from comment #11)
> Comment on attachment 8343759 [details] [diff] [review]
> Implement Object.assign. wip
> 
> Review of attachment 8343759 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/builtin/Object.js
> @@ +16,5 @@
> > +  var keys = std_Object_getOwnPropertyNames(from);
> > +
> > +  // Step 7 (omitted).
> > +  // Step 8.
> > +  var pendingException = undefined;
> 
> This needs an additional flag 'hasPendingException' in case the thrown
> exception is `undefined`.
> 
> @@ +31,5 @@
> > +      if (desc && desc.enumerable) {
> > +        to[key] = from[key];
> > +      }
> > +    } catch (e) {
> > +      if (pendingException === undefined)
> 
> `pendingException` may be `undefined` in which case it gets overwritten.
> 
> @@ +37,5 @@
> > +    }
> > +  }
> > +
> > +  // Step 10.
> > +  if (pendingException)
> 
> `pendingException` may be falsy, another reason why a flag is necessary.

Another way of doing it that might be cleaner is to use some kind of placerholder.

const MISSING = {};
/***/
var pendingException = MISSING;
  /***/
try {
} catch (e) {
  if (pendingException === MISSING)
    pendingException = e;
}
/***/
if (pendingException !== MISSING)
  throw pendingException;

	André Bargull [:anba]
	Comment 14 • 11 years ago

It may make sense to add the following test as well. It serves as a remainder to add a new intrinsic for `Object.getOwnPropertyDescriptor()`, which will be needed to protect against any shenanigans possible through the [[Origin]] field of PropertyDescriptor records.

---
var enumerableGet = 0;
var source = new Proxy({a: 0}, {
  getOwnPropertyDescriptor: () => ({get enumerable(){ enumerableGet += 1; return true; }})
});
Object.assign({}, source);
assertEq(enumerableGet, 1);
---

	Jeff Walden [:Waldo]
	Comment 15 • 10 years ago

Another bug to work on, Nathan.  This one does give you an initial starting point (albeit one probably a bit bitrotted), so maybe it's worth getting the lay of the land from it, then starting on any needed adjustments to it, picking things up as you go.

	Jeff Walden [:Waldo]
	Comment 16 • 10 years ago

Woo, bug 992958 looks likely to have broken self-hosted functions in {Object,Function}.{,prototype.}* something fierce.  Boo-urns.  More investigation/discussion tomorrow.  Play along at home by applying/building/testing this patch:

diff --git a/js/src/builtin/Array.js b/js/src/builtin/Array.js
--- a/js/src/builtin/Array.js
+++ b/js/src/builtin/Array.js
@@ -3,4 +3,8 @@
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+function ObjectFoo()
+{
+}
+
  /* ES5 15.4.4.14. */
 function ArrayIndexOf(searchElement/*, fromIndex*/) {
diff --git a/js/src/builtin/Object.cpp b/js/src/builtin/Object.cpp
--- a/js/src/builtin/Object.cpp
+++ b/js/src/builtin/Object.cpp
@@ -1148,4 +1148,5 @@ const JSFunctionSpec js::object_static_m
     JS_FN("seal",                      obj_seal,                    1,0),
     JS_FN("isSealed",                  obj_isSealed,                1,0),
+    JS_SELF_HOSTED_FN("foo",           "ObjectFoo",                 1,0),
     JS_FS_END
 };

	Bobby Holley (:bholley)
	Comment 17 • 10 years ago

(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> Woo, bug 992958 looks likely to have broken self-hosted functions in
> {Object,Function}.{,prototype.}* something fierce.  Boo-urns. 

From IRC discussion - it sounds like Jeff hasn't actually confirmed that bug 992958 broke this. If it did, it should be easy to fix, because the ClassSpec code is still supposed to execute all the same code that initFunctionAndObjectClasses did, just in a slightly more data-driven manner.

	Nathan Braswell [:nathan] Assignee
	Comment 18 • 10 years ago

This patch updates Till's patch to work with Bug 1032956's fix and adds Brandon's const MISSING suggestion. Unfortunately, the test (assign_test.patch) fails because for some reason Object.getOwnPropertyNames does not correctly call the Proxy's version of getOwnPropertyNames (Bug 1038427).

	Nathan Braswell [:nathan] Assignee
	Comment 19 • 10 years ago

Test cases updated to use ownKeys instead of getOwnPropertyNames, which is no longer in spec. Thanks bz for the info!

	Jeff Walden [:Waldo]
	Comment 20 • 10 years ago

Comment on attachment 8455725 [details] [diff] [review]
workingAssignForReview.patch

Review of attachment 8455725 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Object.cpp
@@ +1232,5 @@
>   * time, after the intrinsic holder has been set, so we put them
>   * in a different array.
>   */
>  const JSFunctionSpec js::object_static_selfhosted_methods[] = {
> +    JS_SELF_HOSTED_FN("assign",        "ObjectStaticAssign",        2,0),

Shouldn't that be 1?  Spec has Object.assign(target, ...source) in it, and per chapter 17, "this value is equal to the largest number of named arguments shown in the subclause headings for the function description, including optional parameters. However, rest parameters shown using the form “...name” are not included in the default argument count."

This needs a test.

::: js/src/builtin/Object.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* ES6 draft 2013-11-08 19.1.2.1. */

You're working off the draft that was complete at the time of the original patch -- but we're well past that now.  Please grab the latest from http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts and update this method to implement those semantics, with according step numbering.

Given this'll completely change the patch, I'm going to mostly stop reviewing at this point.  :-)  Too much likely to have changed between then and now, for the effort to pay off.

@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* ES6 draft 2013-11-08 19.1.2.1. */
> +function ObjectStaticAssign(target, source) {
> +  "use strict"; // To get the correct behavior in step 9.d.v.3

I think you can get rid of this, now, as we compile all self-hosted code as strict mode code.  Try removing this and see if tests pass, and if so it should go.

	Nathan Braswell [:nathan] Assignee
	Comment 21 • 10 years ago

Added a simple test for multiple sources.

	Jeff Walden [:Waldo]
	Comment 23 • 10 years ago

Comment on attachment 8462807 [details] [diff] [review]
my_patch_new_spec.patch

Review of attachment 8462807 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/Object.js
@@ +5,5 @@
> +/* ES6 draft 2014-07-18 19.1.2.1. */
> +function ObjectStaticAssign(target, firstSource) {
> +    // Steps 1-2.
> +    var to = ToObject(target);
> +    // Step 3.

So few blank lines!  Add a blank line before every // Step comment to give the algorithm a little breathing room.  Right now code and comments jumble all together in a mishmash.

@@ +17,5 @@
> +        // Step 5.c-d.
> +        var keysArray = std_Object_getOwnPropertyNames(from);
> +        // Steps 5.e-f.
> +        var len = keysArray.length;
> +        // Step 5.i (Modified a bit because we can't catch and store the 

Trailing whitespace, and one other place.

@@ +21,5 @@
> +        // Step 5.i (Modified a bit because we can't catch and store the 
> +        // actual Completion Record). Instead we have a marker object.
> +        const MISSING = {};
> +        var pendingException = MISSING;
> +        // Step 5.h, 5.j (out of order b/c for loop).

I'd actually prefer a do-while loop to be closer to the spec, even if it's not quite the idiomatic way it might be written in code.

@@ +29,5 @@
> +            // Step 5.j.iii-v.
> +            try {
> +                var desc = std_Object_getOwnPropertyDescriptor(from, nextKey); 
> +                if (desc !== undefined && desc.enumerable)
> +                    to[nextKey]= from[nextKey];

So this works, but a few things.  It's going to create a full-fledged descriptor object every time.  Property accesses on that object are always a dodgy idea, unless you're absolutely sure you'll always hit an own property and not query Object.prototype.

So instead of this, add std_Object_propertyIsEnumerable and do callFunction(that, from, nextKey) instead.  That returns true only if the property is present and enumerable, which is exactly what you want here.

	Nathan Braswell [:nathan] Assignee
	Comment 24 • 10 years ago

Fixed stuff from review. Unfortunatly, due to the way propertyIsEnumerable works, we had to keep using getOwnPropertyDescriptor.

	Jeff Walden [:Waldo]
	Comment 25 • 10 years ago

Comment on attachment 8462804 [details] [diff] [review]
assign_test_edited_plus_mult_test.patch

Review of attachment 8462804 [details] [diff] [review]:
-----------------------------------------------------------------

Good tests in general, but a bunch of changes to make to them.

::: js/src/tests/ecma_6/Object/assign.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +function ToObject(o) {
> +    if (o == null) throw new TypeError();

Believe it or not, in our engine (and most others) there's a difference in behavior between using == and === here.  You want ===.  (The difference is == will consider certain objects to equal null/undefined, those objects being pretty much exclusively document.all.)

@@ +20,5 @@
> +}
> +
> +// Calls ToObject() for target and source
> +{
> +    assertThrows(() => Object.assign(null, null), TypeError);

Object.assign() also worth a test.

@@ +33,5 @@
> +
> +// Invokes [[OwnPropertyKeys]] on ToObject(source)
> +{
> +    assertThrows(() => Object.assign(null, new Proxy({}, {
> +        getOwnPropertyNames: () => fail("ToObject(target) succeeded")

Not sure where fail comes from here.  Make this { throw new Error("not called"); } or so, so it's absolutely clear what the effect of this being called is.

@@ +36,5 @@
> +    assertThrows(() => Object.assign(null, new Proxy({}, {
> +        getOwnPropertyNames: () => fail("ToObject(target) succeeded")
> +    })), TypeError);
> +
> +    let ownKeysCalled = false;

Use var so we don't need the version opt-in.  If you don't want scoping overflow, make each of these a named function statement, then immediately call each one after it.

@@ +43,5 @@
> +            ownKeysCalled = true;
> +            return [];
> +        }
> +    }));
> +    var thing = new Proxy({}, {

This variable isn't used, please remove.

@@ +85,5 @@
> +
> +// Enumerability is decided on-time, not before main loop (1)
> +{
> +    let getterCalled = false;
> +    // FIXME: sourceTarget should not be necessary, Proxy bug in SpiderMonkey

This is wrong.  The getter is called with the receiver object from the original get-operation, which is the proxy.  So it is correct for |this| inside get b() here to be |source|, not |sourceTarget|.

...or maybe this was a claim that Object.defineProperty(proxy, ...) should look for a define hook on the handler, then if none forward to the target, and we're not forwarding to the target?  That might be it.  If so, tho, testing this particular quirk here seems unnecessarily obfuscatory.  We're not trying to test the behavior of Object.defineProperty on a proxy with a handler with no definition trap.  Invoking one just makes the overall test harder to read.  Generally you should avoid doing that, so that future people debugging the test won't have to spend extra time just understanding the test.

So, then, you want:

function testEnumerabilityDeterminedInLoop1()
{
  var getterCalled = false;
  var sourceTarget = {
    get a() { getterCalled = true },
    get b() { Object.defineProperty(sourceTarget, "a", {enumerable: false}); },
  };
  var source = new Proxy(sourceTarget, { ownKeys: () => ["b", "a"] });
  Object.assign({}, source);
  assertEq(getterCalled, false);
}
testEnumerabilityDeterminedInLoop1();

with no FIXME, no claims of there being a bug, etc. because to the extent there's a bug, it's orthogonal to the functionality actually being tested.  (Also with |sourceTarget| being assigned in its own declaration -- having the assignment in the expression passed when creating the proxy is needlessly difficult to read.)

@@ +102,5 @@
> +{
> +    let getterCalled = false;
> +    // FIXME: sourceTarget should not be necessary, Proxy bug in SpiderMonkey
> +    let sourceTarget;
> +    let source = new Proxy(sourceTarget = {

Same bits of stylistic change as mentioned wrt the previous test -- function, var, make sourceTarget be assigned at its declaration, remove the FIXME comment, and so on.

@@ +108,5 @@
> +        get b() { Object.defineProperty(sourceTarget /* this */, "a", {enumerable: true}) },
> +    }, {
> +        ownKeys: () => ["b", "a"]
> +    });
> +    Object.defineProperty(sourceTarget /* source */, "a", {enumerable: false});

I think we can get rid of the /* source */ comment here.  It's reasonable to expect a basic understanding of how |new Proxy| functions, that implies that changes to |sourceTarget| affect |source|'s observable behavior too.

@@ +113,5 @@
> +    Object.assign({}, source);
> +    assertTrue(getterCalled);
> +}
> +
> +// Properties are retrieved through Get()

Might be worth adding "and assigned onto the target as data properties, not in any sense cloned over as descriptors" just for the reader's understanding.  (I will not admit that it took me until this moment to realize that this was what Object.assign did, due to reviewing the algorithm in a stepwise manner.  ;-)   Also, ask your doctor to talk to you about apophasis.)

@@ +192,5 @@
> +    assertTrue("b" in target);
> +    assertFalse(hasB);
> +}
> +
> +// Properties deleted during traversal are not copied

A nice thought, and a nice test.  But we should go further and check that deletion, that exposes a shadowed property, still causes skipping.  So add something like

function testDeletionExposingShadowedProperty()
{
  var srcProto = { b: 42 };
  var src =
    Object.create(srcProto,
                  { a: { get: () => { delete this.b; },
                    b: { value: 2, configurable: true } });
  var source = new Proxy(src, { getOwnPropertyNames: () => ["a", "b"] });
  var target = Object.assign({}, source);
  assertEq("a" in target, true);
  assertEq("b" in target, false);
}
testDeletionExposingShadowedProperty();

@@ +244,5 @@
> +    let target = Object.assign({}, source);
> +    assertDataProperty(target, keyA, {value: 1, writable: true, enumerable: true, configurable: true});
> +}
> +
> +// Intermediate exceptions do not stop property traversal, first exception is reported (1)

These four tests are really nice.

@@ +312,5 @@
> +}
> +
> +// Exceptions in Iterator directly stop property traversal (1)
> +// FIXME: requires [[OwnKeys]] to be an Iterable
> +if (false) {

This test should disappear too, like the function*() one mentioned previously.

@@ +326,5 @@
> +}
> +
> +// Exceptions in Iterator directly stop property traversal (2)
> +// FIXME: requires [[OwnKeys]] to be an Iterable
> +if (false) {

And this.

@@ +348,5 @@
> +}
> +
> +// Exceptions in Iterator directly stop property traversal (3)
> +// FIXME: requires [[OwnKeys]] to be an Iterable
> +if (false) {

And this.

::: js/src/tests/ecma_6/Object/shell.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +if (typeof version != 'undefined')
> +{
> +  version(185);

Definitely, absolutely, I don't want this.  Search for version(185) in the tree, and note the big scary comments next to all of those.  There's no legitimate reason for tests to need anything 185 or ES6-related in terms of syntax, so we shouldn't be opting into it.  (And if we were, I would require the same sort of adjacent comment, and the top-level test runner JS script changed accordingly.)

@@ +6,5 @@
> +{
> +  version(185);
> +}
> +
> +var assertThrows = assertThrowsInstanceOf;

Please just use assertThrowsInstanceOf in the test.  And assertEq instead of assertSame, assertEq(..., true) instead of assertTrue, assertEq(..., false) instead of assertFalse, and so on for all the simple things here.  There's no precedent for tests being written in some sort of jUnit style, versus using assertEq directly.

Additionally, tests are much more readable when these sorts of one-line tests are written out fully.  This makes it unambiguous that there's nothing unusual happening behind the scenes.  In contrast, it's not inconceivable that someone might see assertTrue and interpret it as a truthiness test, not an is-exactly-true test.

@@ +53,5 @@
> +function assertNotConstructor(o) {
> +    return assertFalse(IsConstructor(o));
> +}
> +
> +function assertDataProperty(object, propertyKey, {value:value, writable:writable, enumerable:enumerable, configurable:configurable}) {

Move this function into assign.js itself, rename it to something like checkDataProperty.  And just expand out value/writable/enumerable/configurable vars and avoid destructuring, for now.  Makes the test more portable, and if this is the only reason for the version(185) thing, it's just not worth it.

@@ +63,5 @@
> +    assertSame(enumerable, desc.enumerable);
> +    assertSame(configurable, desc.configurable);
> +}
> +
> +function assertAccessorProperty(object, propertyKey, {get:get, set:set, enumerable:enumerable, configurable:configurable}) {

Same sort of treatment for this function as for the other one.

@@ +73,5 @@
> +    assertSame(enumerable, desc.enumerable);
> +    assertSame(configurable, desc.configurable);
> +}
> +
> +function assertBuiltinFunction(fun, name, arity) {

Honestly, the testing here is a little bit excessive.  :-)  Just stick with assertEq(Object.assign.length, 2) in the test and leave it at that.

	Jeff Walden [:Waldo]
	Comment 26 • 10 years ago

Comment on attachment 8468908 [details] [diff] [review]
my_patch_reviewEdited_20140806.patch

Review of attachment 8468908 [details] [diff] [review]:
-----------------------------------------------------------------

I'll make the few little changes here myself and queue this up to land, waiting on the test to do so.

::: js/src/builtin/Object.js
@@ +3,5 @@
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* ES6 draft 2014-07-18 19.1.2.1. */
> +function ObjectStaticAssign(target, firstSource) {
> +

No blank line before "// Step" at the very start of the function.

@@ +15,5 @@
> +    // Step 4.
> +    var i = 1;
> +    do {
> +
> +        // Step 5.a-b.

Or here, either.  The point of the blank line is to keep bits of algorithm/code separate.  Structural boundaries don't need any such clarification.

@@ +44,5 @@
> +            try {
> +                 /*
> +                  * We would like to use Object.propertyIsEnumerable here, but can't because
> +                  * if a property does not exist it does not properly call getOwnPropertyDescriptor
> +                  * (important if proxied)

Period at the end of a complete sentence.  (And "if proxied" is not really right, it's "if |from| is a proxy".)

@@ +48,5 @@
> +                  * (important if proxied)
> +                  */
> +                var desc = std_Object_getOwnPropertyDescriptor(from, nextKey);
> +                if (desc !== undefined && desc.enumerable)
> +                    to[nextKey]= from[nextKey];

So looking a second time, I wondered if the evaluation of this line might *observably* evaluate the left-hand-side before the right-hand-side, contrary to the algorithm being implemented.  (The LHS is evaluated first to some degree, think |undefined[5] = ({ get p() { throw 42; } }).p;|, but observability in the confines here wasn't obvious.)

There *are* evaluations of the LHS first, but they are 1) CheckObjectCoercible(to) and 2) ToString(nextKey) (really ToPropertyKey, should report a spec bug on this).  Both of which have no untoward effects, in the confines of this method.  So doing it this way without a temporary is fine, tho I could be persuaded that splitting it up with a temporary would be more readably, obviously correct.

Oh, spaces around the = here.

::: js/src/builtin/Utilities.js
@@ +63,5 @@
>  var std_Object_create = Object.create;
>  var std_Object_getOwnPropertyNames = Object.getOwnPropertyNames;
>  var std_Object_hasOwnProperty = Object.prototype.hasOwnProperty;
>  var std_Object_getPrototypeOf = Object.getPrototypeOf;
> +var std_Object_getOwnPropertyDescriptor = Object.getOwnPropertyDescriptor;

Thinking harder about this, given that this method can only be used safely to check for 1) the existence of a property, 2) its enumerability, or 3) its configurability, and nothing else is safe (unless you do really onerous things like check for "get"/"set"/"value"/"writable" as own properties, which no one is going to do), we probably shouldn't expose this in the long run.  But for now let's just pocket it to get this patch landed.

	Nathan Braswell [:nathan] Assignee
	Comment 27 • 10 years ago

Updated from review!

	Jeff Walden [:Waldo]
	Comment 28 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/53769e48d35b

	Nigel Babu [:nigelb]
	Comment 29 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/53769e48d35b

	[SV Manager] Florin Mezei, QA (:FlorinMezei)
	Comment 30 • 10 years ago

Is this fully covered by automatic tests?

	Nathan Braswell [:nathan] Assignee
	Comment 31 • 10 years ago

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #30)
> Is this fully covered by automatic tests?

One of the two patches is test cases (assign.js) and was a part of this commit, so it should be covered in the regular JS tests, yes. (unless I misunderstood your question)
Thanks!

	teramako
	Comment 32 • 10 years ago

current implementation is not complete because it doesn't handle symbol properties.

	ziyunfei
	Comment 33 • 10 years ago

(In reply to teramako from comment #32)
> current implementation is not complete because it doesn't handle symbol
> properties.

teramako is right, I think we need a `std_Object_getOwnPropertyKeys` function to get all string-type and symbol-type properties

	David Bruant
	Comment 34 • 10 years ago

(In reply to 446240525 from comment #33)
> (In reply to teramako from comment #32)
> > current implementation is not complete because it doesn't handle symbol
> > properties.
> 
> teramako is right, I think we need a `std_Object_getOwnPropertyKeys`
> function to get all string-type and symbol-type properties
Filed https://bugzilla.mozilla.org/show_bug.cgi?id=1052358 for this purpose.

	Sebastian Markbåge
	Comment 35 • 10 years ago

This implementation will be incorrect because it was concluded at the last TC-39 meeting that this should not throw for null or undefined sources.

https://github.com/rwaldron/tc39-notes/blob/master/es6/2014-07/jul-29.md#revisit-objectassign

The ES6 draft has not been updated. This would break Facebook.com in it's current state since it relies on the new spec.

	Florian Scholz (Open Web Docs)
	Comment 36 • 10 years ago

Added to the typical places:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object#Methods
https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/ECMAScript_6_support_in_Mozilla#Additions_to_the_Object_object
https://developer.mozilla.org/en-US/Firefox/Releases/34#JavaScript

Main new doc:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign

Any feedback or review appreciated. Could need some more examples and maybe provide a Polyfill?

	ziyunfei
	Comment 37 • 10 years ago

(In reply to Florian Scholz [:fscholz] (elchi3) from comment #36)
> Added to the typical places:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Object#Methods
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/New_in_JavaScript/
> ECMAScript_6_support_in_Mozilla#Additions_to_the_Object_object
> https://developer.mozilla.org/en-US/Firefox/Releases/34#JavaScript
> 
> Main new doc:
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/
> Global_Objects/Object/assign
> 
> Any feedback or review appreciated. Could need some more examples and maybe
> provide a Polyfill?

Added a polyfill.

	Tom S [:evilpie]
	Comment 38 • 10 years ago

From what I can tell the shim is wrong in several ways. I would suggested we just adapt the SpiderMonkey code for the shim.

	ziyunfei
	Comment 39 • 10 years ago

(In reply to Tom Schuster [:evilpie] from comment #38)
> From what I can tell the shim is wrong in several ways. I would suggested we
> just adapt the SpiderMonkey code for the shim.

Hi, Tom. Actually, the first version of that shim was adapted from SpiderMonkey code. And could you please point out the specific problems that shim has.

	Emanuel Hoogeveen [:ehoogeveen]
	Comment 40 • 10 years ago

The implementation presumably changed in bug 1054426 due to changes in the spec.

	Tom S [:evilpie]
	Comment 41 • 10 years ago

1. Step 5.d.i is missing.
2. All the stuff which handles exceptions is missing.

Here is our implementation adjusted to work on the web: http://jsfiddle.net/yfhwkqm4/

	Tom S [:evilpie]
	Comment 42 • 10 years ago

Actually this one: http://jsfiddle.net/yfhwkqm4/1/

	ziyunfei
	Comment 43 • 10 years ago

(In reply to Tom Schuster [:evilpie] from comment #41)
> 1. Step 5.d.i is missing.
I don't think we need to care about this step since ES5 doesn't have symbols and Proxy, using Object.keys is enough.

> 2. All the stuff which handles exceptions is missing.
Updated that polyfill to handle [[Get]], [[Set]] exceptions.

Please correct me if I did something wrong. Thanks!