Implement ES6 Object.assign

RESOLVED FIXED in mozilla34

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bbenvie, Assigned: nathan)

Tracking

(Blocks 1 bug, {dev-doc-complete, feature})

Trunk
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [DocArea=JS][js:p2])

Attachments

(2 attachments, 7 obsolete attachments)

Reporter

Description

6 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.
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
Duplicate of this bug: 932375
I'm working on this
Assignee: nobody → till
Status: NEW → ASSIGNED
(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.
Which kind of format do you prefer: js-test or jit-test?
While I prefer running jit-tests, this really belongs into js-test.
Posted patch assign_test.patch (obsolete) — Splinter Review
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.
Posted patch assign_test.patch (obsolete) — Splinter Review
Missed the other case where I need to workaround that Proxy issue.
Attachment #8341713 - Attachment is obsolete: true
Awesome, thanks! I'll obviously remove the `Object.assign = function ...` part, but other than that, this looks pretty great.
Posted patch Implement Object.assign. wip (obsolete) — Splinter Review
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.
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.
(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. :(
Reporter

Comment 13

6 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;
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);
---
Whiteboard: [DocArea=JS]
Keywords: feature
Whiteboard: [DocArea=JS] → [DocArea=JS][js:p2]
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.
Assignee: till → nathan
Assignee: nathan → miloignis
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
 };
(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.
Assignee

Updated

5 years ago
Depends on: 1032956
Assignee

Updated

5 years ago
Depends on: 1038427
Assignee

Comment 18

5 years ago
Posted patch workingAssignForReview.patch (obsolete) — Splinter Review
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).
Attachment #8343759 - Attachment is obsolete: true
Attachment #8455725 - Flags: review?(jwalden+bmo)
No longer depends on: 1038427
Assignee

Comment 19

5 years ago
Posted patch assign_test_edited2.patch (obsolete) — Splinter Review
Test cases updated to use ownKeys instead of getOwnPropertyNames, which is no longer in spec. Thanks bz for the info!
Attachment #8341733 - Attachment is obsolete: true
Attachment #8456475 - Flags: review?(jwalden+bmo)
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.
Attachment #8455725 - Flags: review?(jwalden+bmo) → review-
Assignee

Comment 21

5 years ago
Added a simple test for multiple sources.
Attachment #8456475 - Attachment is obsolete: true
Attachment #8456475 - Flags: review?(jwalden+bmo)
Attachment #8462804 - Flags: review?(jwalden+bmo)
Assignee

Comment 22

5 years ago
Posted patch my_patch_new_spec.patch (obsolete) — Splinter Review
Attachment #8462807 - Flags: review?(jwalden+bmo)
Assignee

Updated

5 years ago
Attachment #8455725 - Attachment is obsolete: true
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.
Attachment #8462807 - Flags: review?(jwalden+bmo) → review+
Assignee

Comment 24

5 years ago
Fixed stuff from review. Unfortunatly, due to the way propertyIsEnumerable works, we had to keep using getOwnPropertyDescriptor.
Attachment #8462807 - Attachment is obsolete: true
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.
Attachment #8462804 - Flags: review?(jwalden+bmo) → review-
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.
Attachment #8468908 - Flags: review+
Assignee

Comment 27

5 years ago
Updated from review!
Attachment #8462804 - Attachment is obsolete: true
Attachment #8470375 - Flags: review?(jwalden+bmo)
Attachment #8470375 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/53769e48d35b
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
Is this fully covered by automatic tests?
Flags: in-testsuite?
Assignee

Comment 31

5 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!
Assignee

Updated

5 years ago
Flags: in-testsuite? → in-testsuite+

Comment 32

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

Comment 33

5 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

Updated

5 years ago
Depends on: 1052358

Comment 34

5 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.
No longer depends on: 1052358

Updated

5 years ago
Depends on: 1052358
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.

Updated

5 years ago
Depends on: 1055902

Comment 37

5 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.
From what I can tell the shim is wrong in several ways. I would suggested we just adapt the SpiderMonkey code for the shim.

Comment 39

5 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.
The implementation presumably changed in bug 1054426 due to changes in the spec.
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/

Comment 43

5 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!
You need to log in before you can comment on or make changes to this bug.