Closed Bug 987514 Opened 10 years ago Closed 9 years ago

Implement ES6 Reflect

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed
relnote-firefox --- 42+

People

(Reporter: bbenvie, Assigned: jorendorff)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, feature, Whiteboard: [DocArea=JS][js:p2])

Attachments

(6 files, 7 obsolete files)

6.56 KB, patch
Details | Diff | Splinter Review
12.57 KB, patch
Details | Diff | Splinter Review
6.20 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
1.51 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
19.04 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
81.94 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
Section 26.1 [1] of the ES6 draft spec (January 2014 revision) is about the Reflect property of the global object (not to be confused with the existing Reflect in SpiderMonkey that has Reflect.parse). Reflect is a plain object with functions on it that correspond to the Proxy handler traps, such that Reflect can be used as a no-op forwarding handler. This simplifies implementing Proxy handlers by making it straightforward to obtain (and potentially modify and/or return) the result of calling the operation on the Proxy's target. Some of the functions are similar to those on Object, with the same name and slightly different functionality (so that they work like valid trap handlers).


* Reflect.defineProperty (target, propertyKey, attributes)
Like Object.defineProperty but returns boolean indicating success.

* Reflect.deleteProperty (target, propertyKey)
Like `delete` operator.

* Reflect.enumerate(target)
Returns an iterator that yields the values that `for..in` would. It differs from for..in in that it won't throw if target can be coerced to an object (e.g. `Reflect.enumerate('foobar')` will work).

* Reflect.get (target, propertyKey, receiver=target)
Like getting a property from an object `target[propertyKey]`. This also allows you to specify a receiver which is used as the |this| value if a getter is encountered (implementing this in JS userland is inefficient because specifying the receiver entails the use of getOwnPropertyDescriptor and proto-walking).

* Reflect.getOwnPropertyDescriptor (target, propertyKey)
Same as Object.getOwnPropertyDescriptor.

* Reflect.getPrototypeOf (target)
Same as Object.getPrototypeOf.

* Reflect.has (target, propertyKey)
Like `in` operator, but it won't throw if target is coercible to an object.

* Reflect.hasOwn (target, propertyKey)
Same as Object#hasOwnProperty but non-method.

* Reflect.isExtensible (target)
Similar to Object.isExtensible. This edition of the spec has the Object version *not* throwing if target is null or undefined, whereas the Reflect version does.

* Reflect.ownKeys (target)
Returns an iterator for all of the target's keys, including non-enumerable ones and symbols (corresponds to [[OwnPropertyKeys]]).

* Reflect.preventExtensions (target)
Similar to Object.preventExtensions. Same deal with null/undefined as Reflect.isExtensible. Returns boolean indicating success.

* Reflect.set (target, propertyKey, V, receiver=target)
Like setting a property on an object. Same deal as Reflect.get.

* Reflect.setPrototypeOf (target, proto)
Like Object.setPrototypeOf but returns boolean indicating success.


Example usage:
> var logger = new Proxy({}, {
>   get(key, ...args) {
>     // Reflect's functions are valid trap handlers, so just forward args.
>     const result = Reflect.get(key, ...args);
>     console.log("got " + key + " | " + result);
>     return result;
>   }
> });


[1] https://people.mozilla.org/~jorendorff/es6-draft.html#sec-reflect-object
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
The current revision of the spec of Reflect is incomplete [1], missing `apply` and `construct` (corresponding to the additional two traps available to function Proxies). These functions must be provided a callable target and an array of args.

* Reflect.apply (target, thisArgument, args)
  Same as Function#apply but non-method.

* Reflect.construct (target, args)
  Like `new target(...args)`.


[1] https://bugs.ecmascript.org/show_bug.cgi?id=2477
Keywords: feature
Whiteboard: [DocArea=JS] → [DocArea=JS][js:p2]
This is just some preparatory plumbing I did a while ago. Whoever picks this bug up can take it as a first step.
Assignee: nobody → evilpies
Attachment #8432844 - Attachment is obsolete: true
Attachment #8526970 - Flags: review?(till)
Attached patch reflect-testSplinter Review
I will file new bugs for the rest of those, but starting out with an empty object is not really helpful.
Attachment #8526971 - Flags: review?(till)
oO Noticed after uploading that this didn't have any es6 references.
Attachment #8526970 - Attachment is obsolete: true
Attachment #8526970 - Flags: review?(till)
Attachment #8526982 - Flags: review?(till)
Assignee: evilpies → jorendorff
Status: NEW → ASSIGNED
Attachment #8527019 - Attachment is obsolete: true
All these are just for reference -- this is how far I got before deciding it was all premature. Since call and apply can be implemented correctly today, it would be OK to land it.

evilpie has more tests, so his patch is presumably better. Haven't looked. Also I had to do some pretty ham-handed things to get the Reflect object created. Yuck.
Comment on attachment 8526982 [details] [diff] [review]
Implement Reflect infrastructure and apply/construct

Let's go with Jason's patches. However I think my implementation of apply and call looks cleaner, but I am missing the FastInvoke stuff.
Attachment #8526982 - Flags: review?(till)
Attachment #8526971 - Flags: review?(till)
*"I think my implementation of apply and *construct* look cleaner"
Comment on attachment 8527023 [details] [diff] [review]
part 3 - Make every global have a (usually empty) Reflect object; rename JS_InitReflect -> JS_InitReflectParse

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

I'm not sure I understand what you introduced all the support for compile time-disabled global members for. Reflect can't be disabled at compile time, right?

Other than that, and with the nits addressed, I think we should land this. So, if you want it: r=me.

::: js/src/builtin/ReflectParse.cpp
@@ +3611,1 @@
>          return nullptr;

return false

@@ +3611,3 @@
>          return nullptr;
> +    if (!reflectVal.isObject()) {
> +        JS_ReportError(cx, "JS_InitReflectParse must be called during global initialization");

This should be moved to js.msg. Alternatively, it could just use JSMSG_NOT_NONNULL_OBJECT as in evilpie's version.

@@ +3614,1 @@
>          return nullptr;

return false

@@ +3614,5 @@
>          return nullptr;
>      }
>  
> +    RootedObject reflectObj(cx, &reflectVal.toObject());
> +    return JS_DefineFunctions(cx, reflectObj, static_methods);

I guess this could also just use `return !!JS_DefineFunction(cx, reflectObj, "parse", reflect_parse, 1, 0)`, removing the need for the static_methods list above.

::: js/src/tests/ecma_6/Reflect/surfaces.js
@@ +1,3 @@
> +/*
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/licenses/publicdomain/

Either https://creativecommons.org/publicdomain/zero/1.0/ or just remove the header.

@@ +16,5 @@
> +// Reflect property once it is deleted.
> +delete this.Reflect;
> +assertEq("Reflect" in this, false);
> +
> +reportCompare(0, 0, 'ok');

Isn't the `if (typeof reportCompare === 'function')` song and dance required?
(In reply to Till Schneidereit [:till] from comment #14)
> I'm not sure I understand what you introduced all the support for compile
> time-disabled global members for. Reflect can't be disabled at compile time,
> right?

Which patch are we talking about here?

> > +reportCompare(0, 0, 'ok');
> 
> Isn't the `if (typeof reportCompare === 'function')` song and dance required?

No. Some contributors do that because they like running certain tests with `$JS testfilename.js`, omitting shell.js, but this doesn't work for all tests and never has.

Some people still write BUGNUMBER and so on. That one I don't understand at all...

`reportCompare(0, 0, 'ok');` is the minimal boilerplate.
I've dusted this stuff off and started writing tests. More news later this week...
I ran into a speed bump here. Adding Reflect.construct() makes it really easy to fail this assertion in js::ArrayConstructor:

>    if (args.isConstructing())
>        MOZ_ASSERT(args.newTarget().toObject().as<JSFunction>().native() == js::ArrayConstructor);

Options:

- Reflect.construct could be separated from the rest of this.

- I could remove the assertion and leave `Reflect.construct(Array, [], C)` buggy.

- I could implement the ES6 Array constructor's use of NewTarget.
  This is necessary to support subclassing Array anyway.

Leaning toward the last option, but it's a lot more work to pile into this otherwise simple bug. :-|
Attachment #8527014 - Attachment is obsolete: true
Attachment #8527018 - Attachment is obsolete: true
Attachment #8527023 - Attachment is obsolete: true
Attachment #8527026 - Attachment is obsolete: true
Attachment #8627828 - Attachment is obsolete: true
Attachment #8627828 - Flags: review?(jwalden+bmo)
The bit in MIRGenerator.h fixes a straight-up bug in the code, masked until now by unified builds (and unmasked because this patch adds a .cpp file, perturbing the unified build boundaries).
Attachment #8627830 - Flags: review?(jwalden+bmo)
Attachment #8627829 - Attachment is obsolete: true
Attachment #8627829 - Flags: review?(jwalden+bmo)
Reflect.construct is not implemented because the builtin constructors do not support NewTarget correctly yet. Array at least asserts if you pass it an unexpected NewTarget.

Reflect.enumerate is not implemented because we do not implement the underlying internal method to spec yet.
Attachment #8627831 - Flags: review?(jwalden+bmo)
Attachment #8627830 - Attachment is obsolete: true
Attachment #8627830 - Flags: review?(jwalden+bmo)
The bit in comment 26 about NewTarget not working yet is probably not true anymore (I wrote the patch last week, and efaust is moving fast).

I do have an additional patch for Reflect.construct, and I'll coordinate with efaust on how to land it.
Attachment #8627828 - Attachment is obsolete: false
Attachment #8627828 - Flags: review?(jwalden+bmo)
Attachment #8627829 - Attachment is patch: false
Attachment #8627829 - Flags: review?(jwalden+bmo)
Attachment #8627830 - Attachment is obsolete: false
Attachment #8627830 - Flags: review?(jwalden+bmo)
Attachment #8627829 - Attachment is obsolete: false
Attachment #8627829 - Attachment is patch: true
Comment on attachment 8627828 [details] [diff] [review]
part 1 - Move the contents of jsreflect.h into the sole file that includes it (jsreflect.cpp). No change in behavior

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

::: js/src/jsreflect.cpp
@@ +104,5 @@
> +    PROP_MUTATEPROTO,
> +    PROP_LIMIT
> +};
> +
> +static char const * const aopNames[] = {

Style nit, and for the others:

static const char* const appNames[]
Attachment #8627828 - Flags: review?(jwalden+bmo) → review+
Attachment #8627829 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8627830 [details] [diff] [review]
part 3 - Make every global have a (usually empty) Reflect object; rename JS_InitReflect -> JS_InitReflectParse

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

::: js/src/builtin/Reflect.cpp
@@ +18,5 @@
> +    RootedObject proto(cx, obj->as<GlobalObject>().getOrCreateObjectPrototype(cx));
> +    if (!proto)
> +        return nullptr;
> +
> +    RootedObject Reflect(cx, NewObjectWithGivenProto<PlainObject>(cx, proto, SingletonObject));

I'd prefer |reflect| to |Reflect|, even if the object is |Reflect| in JS.

::: js/src/builtin/Reflect.h
@@ +11,5 @@
> +
> +namespace js {
> +
> +extern JSObject *
> +InitReflect(JSContext *cx, js::HandleObject obj);

Stars in wrong places.

::: js/src/builtin/ReflectParse.cpp
@@ +3751,5 @@
> +    if (!GetProperty(cx, global, global, cx->names().Reflect, &reflectVal))
> +        return false;
> +    if (!reflectVal.isObject()) {
> +        JS_ReportError(cx, "JS_InitReflectParse must be called during global initialization");
> +        return false;

Fine enough, but it would be nicer if Reflect.parse were specified as a compartment option when the global/compartment is created.  I'd prefer seeing this as a followup change, honestly.

::: js/src/jsapi.cpp
@@ +1272,5 @@
>      // If this class is anonymous, then it doesn't exist as a global
>      // property, so we won't resolve anything.
>      JSProtoKey key = stdnm ? stdnm->key : JSProto_Null;
> +    if (key != JSProto_Null) {
> +        const Class *clasp = ProtoKeyToClass(key);

Star in wrong place.
Attachment #8627830 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8627831 [details] [diff] [review]
part 4 - Implement most of the standard Reflect methods. try: -b do -p linux64,macosx64,win32,android-api-9,emulator -u all -t none

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

::: js/src/builtin/Reflect.cpp
@@ +20,5 @@
> + * The elementTypes argument is not supported. The result list is
> + * pushed to *args.
> + */
> +static bool
> +InitArgsFromArrayLike(JSContext *cx, HandleValue v, InvokeArgs *args, bool construct)

Stars in wrong places.  (Is this something wrong in the entire patch, probably?  Please fix all of it.)

@@ +65,5 @@
> +    }
> +
> +    // Steps 2-3.
> +    FastInvokeGuard fig(cx, args.get(0));
> +    InvokeArgs &invokeArgs = fig.args();

And I guess & is in the wrong place, too.

@@ +164,5 @@
> +/*
> + * ES6 draft rev 32 (2015 Feb 2) 26.1.6
> + *
> + * TODO:
> + * - Change type of JSObject::getGeneric et al to permit non-object receiver.

This comment needs rewriting slightly, at least.

@@ +186,5 @@
> +    // Step 4.
> +    RootedValue receiver(cx, argc > 2 ? args[2] : args.get(0));
> +
> +    // Non-standard hack: Throw a TypeError if the receiver isn't an object.
> +    // See bug ######.

"This should be fixed in bug 603201."

@@ +206,5 @@
> +        return false;
> +
> +    // The other steps are identical to ES6 draft rev 32 (2015 Feb 2) 19.1.2.6
> +    // Object.getOwnPropertyDescriptor.
> +    return js::obj_getOwnPropertyDescriptor(cx, argc, vp);

Is it that much hassle to just do the literal steps?  This is messier to verify for every reader, code size improvement seems negligible.

@@ +313,5 @@
> +/*
> + * ES6 draft rev 32 (2015 Feb 2) 26.1.3
> + *
> + * The specification is not quite similar enough to Object.setPrototypeOf to
> + * share code.

Again, I don't think this is something to aspire to.

::: js/src/builtin/Reflect.h
@@ +21,5 @@
> +extern bool
> +Reflect_getPrototypeOf(JSContext *cx, unsigned argc, Value *vp);
> +
> +extern bool
> +Reflect_isExtensible(JSContext *cx, unsigned argc, Value *vp);

Stars in wrong places.

::: js/src/builtin/Reflect.js
@@ +8,5 @@
> +    if (!IsObject(target))
> +        ThrowTypeError(JSMSG_NOT_NONNULL_OBJECT, DecompileArg(0, target));
> +
> +    // Steps 2-4 are identical to the runtime semantics of the "in" operator.
> +    return propertyKey in target;

Fine enough, but to be honest, I'm leery about having Reflect.* operations not directly call the C++ versions.  Having a bug break Reflect.*, in ways identical to how an operator we might be testing using Reflect.* would work, seems unideal.  You see what I'm saying?

::: js/src/jit-test/tests/proxy/testDirectProxySetArray2.js
@@ -2,3 @@
>  
> -if (this.Reflect && Reflect.set)
> -    throw new Error("Congrats on implementing Reflect.set! Uncomment this test for 1 karma point.");

jorendorff++

::: js/src/js.msg
@@ +81,5 @@
>  MSG_DEF(JSMSG_BAD_GENERATOR_YIELD,     1, JSEXN_TYPEERR, "yield from closing generator {0}")
>  MSG_DEF(JSMSG_EMPTY_ARRAY_REDUCE,      0, JSEXN_TYPEERR, "reduce of empty array with no initial value")
>  MSG_DEF(JSMSG_UNEXPECTED_TYPE,         2, JSEXN_TYPEERR, "{0} is {1}")
>  MSG_DEF(JSMSG_MISSING_FUN_ARG,         2, JSEXN_TYPEERR, "missing argument {0} when calling function {1}")
> +MSG_DEF(JSMSG_NOT_NONNULL_OBJECT,      1, JSEXN_TYPEERR, "{0} is not an object")

Nice.  Micro-bonus points for JSMSG_NOT_AN_OBJECT even, as a followup tweak if you want and all (note this would require bumping XDR, as I think we have this particular error name burned into self-hosted bytecode).

::: js/src/jsobjinlines.h
@@ +588,5 @@
>  }
>  
> +/* ES6 draft rev 28 (2014 Oct 14) 7.1.14 */
> +inline bool
> +ToPropertyKey(JSContext *cx, Value argument, MutableHandleId result)

Star wrong place.

::: js/src/tests/ecma_6/Reflect/apply.js
@@ +12,5 @@
> +}
> +
> +// When target is not callable, Reflect.apply does not try to get argumentList.length before throwing.
> +var hits = 0;
> +var bogusArgumentList = {get length() { hit++; return 0;}};

Could just throw 42 as well.

@@ +115,5 @@
> +// object.
> +var testValues = [true, 1e9, "ok", Symbol("ok")];
> +function nonStrictThis(expected) {
> +    assertEq(typeof this, "object");
> +    print(`actual value:  ${uneval(this)}`);

uneval isn't standard, so putting this test in ecma_6/Reflect rather than ecma_6/extensions is bad.  Make testValues an array of [val, repr] pairs and put literal descriptive strings in as reprs.

::: js/src/tests/ecma_6/Reflect/argumentsList.js
@@ +52,5 @@
> +assertEq(Reflect.apply(testLess, undefined, args), "ok");
> +//assertEq(Reflect.construct(testLess, args) instanceof testLess, true);
> +
> +// argumentList.length can be more than func.length.
> +function testMore(a) {

The proper spelling is testMoar, isn't it?

@@ +59,5 @@
> +}
> +assertEq(Reflect.apply(testMore, undefined, args), "good");
> +//assertEq(Reflect.construct(testMore, args) instanceof testMore, true);
> +
> +// argumentList can be any object with a .length property.

Please add a test that the generator protocol is *not* invoked for this junk.

@@ +81,5 @@
> +    assertEq(method(count, undefined, {"0": 0, "1": 1}).numArgsReceived,
> +             0);
> +    function* g() { yield 1; yield 2; }
> +    assertEq(method(count, undefined, g()).numArgsReceived,
> +             0);

Oh.  Never mind, then!

::: js/src/tests/ecma_6/Reflect/defineProperty.js
@@ +1,3 @@
> +/* Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/licenses/publicdomain/ */
> +

Add a test like this somewhere:

var poison =
  (counter => new Proxy({}, new Proxy({}, { get() { throw counter++; } })))(42);
assertThrowsValue(() => Reflect.defineProperty(poison,
                                               { toString() { throw 17; }, valueOf() { throw 8675309; } },
                                               poison),
                  17);

@@ +11,5 @@
> +                    enumerable: false,
> +                    configurable: false});
> +
> +// Reflect.defineProperty can define a symbol-keyed property.
> +var key = Symbol(":o)");

Maybe use Object(Symbol...) or new Symbol(...) to ensure object-to-symbol conversion works?  And then check for obj[just the symbol, not an object] below.

@@ +30,5 @@
> +    }
> +});
> +assertEq(Reflect.defineProperty(proxy, "prop", {value: 7}), true);
> +assertEq(obj.prop, 1);
> +assertEq(delete obj.prop, true);

assertEq("prop" in proxy, false) for good measure?

@@ +45,5 @@
> +    defineProperty(t, id, desc) {
> +        assertEq(desc !== attributes, true);
> +        assertEq(desc.configurable, true);
> +        assertEq(desc.enumerable, false);
> +        assertEq(desc.value, null);

Also writable: false?

@@ +119,5 @@
> +assertEq(Reflect.defineProperty(obj, "prop", {set: g}), false);
> +assertEq(Reflect.defineProperty(obj, "prop", {set: s}), true);  // no-op
> +
> +// Proxy defineProperty handler method that returns false
> +var falseValues = [false, 0, -0, "", NaN, null, undefined];

typeof objectEmulatingUndefined === "function" ? objectEmulatingUndefined() : null

::: js/src/tests/ecma_6/Reflect/get.js
@@ +10,5 @@
> +
> +
> +// === Various targets
> +
> +// Array

for (var v of thatPrimitivesArray)
  assertThrowsInstanceOf(() => Reflect.get(v, "foo"), TypeError);

::: js/src/tests/ecma_6/Reflect/getPrototypeOf.js
@@ +11,5 @@
> +var proxy = new Proxy({}, {
> +    getPrototypeOf(t) { return Math; }
> +});
> +var result = Reflect.getPrototypeOf(proxy);
> +if (result === Math) {

This'll never be true, as if the target's [[Prototype]] and the trap result differ, a TypeError is thrown.  You'd have to have a multilevel proxy-chain where the outermost level mutated the inner level's chain to write a test that fails if-ily like this.  Up to you whether to do it all just for a nice error message here.

::: js/src/tests/ecma_6/Reflect/has.js
@@ +10,5 @@
> +var arr = ["zero"];
> +arr[10000] = 0;
> +assertEq(Reflect.has(arr, "10000"), true);
> +assertEq(Reflect.has(arr, 10000), true);
> +assertEq(Reflect.has(arr, -0), true);

(arr, "-0"), false?

::: js/src/tests/ecma_6/Reflect/isExtensible.js
@@ +10,5 @@
> +    {},
> +    {a: "a"},
> +    [0, 1],
> +    new Uint8Array(64),
> +    Object(Symbol("table")),

Is new Symbol("table") identical?

::: js/src/tests/ecma_6/Reflect/propertyKeys.js
@@ +41,5 @@
> +    }
> +];
> +
> +if ("toPrimitive" in Symbol)
> +    throw new Exception("Congratulations on implementing Symbol.toPrimitive! " +

Error?  I mean, it hardly matters, but.

::: js/src/tests/ecma_6/Reflect/set.js
@@ +53,5 @@
> +obj = {};
> +var obj2 = {};
> +assertEq(Reflect.set(obj, "prop", 47, obj2), true);
> +assertDeepEq(obj, {});
> +assertDeepEq(obj2, {prop: 47});

Would rather compare Object.getOwnPropertyDescriptor(obj2), { value: 47, writable: true, enumerable: true, configurable: true }, myself.

@@ +152,5 @@
> +
> +// With two arguments, the value is assumed to be undefined.
> +obj = {};
> +assertEq(Reflect.set(obj, "size"), true);
> +assertEq(obj.size, undefined);

Again, compare descriptors.

@@ +157,5 @@
> +
> +// With just one argument, the key is "undefined".
> +obj = {};
> +assertEq(Reflect.set(obj), true);
> +assertEq(obj.undefined, undefined);

Ditto.

@@ +231,5 @@
>  var a = [0, 1, 2, 3];
>  var p = Object.create(a);
>  Reflect.set(p, "0", 42, a);
>  assertEq(p.hasOwnProperty("0"), false);
>  assertEq(a[0], 42);

Descriptor comparison.

@@ +263,5 @@
> +}
> +
> +existingDescriptor = {value: 7, writable: true, enumerable: false, configurable: true};
> +expected = {value: 4};
> +for (var defineReuslt of [true, false]) {

This is a typo.  I...think the test remains the same with this changed, just with better testing of what should have been tested.

::: js/src/tests/ecma_6/Reflect/setPrototypeOf.js
@@ +16,5 @@
> +// The target argument is required.
> +assertThrowsInstanceOf(() => Reflect.setPrototypeOf(), TypeError);
> +
> +// Throw if the target argument is not an object.
> +for (var value of [null, undefined, "fail"]) {

Not that all-primitive-values thingy?  And is this needed if targets.js had a [null] entry and included this method?

@@ +34,5 @@
> +proto = {};
> +obj = Object.preventExtensions(Object.create(proto));
> +assertEq(Reflect.setPrototypeOf(obj, {}), false);
> +assertEq(Reflect.setPrototypeOf(obj, null), false);
> +assertEq(Reflect.setPrototypeOf(obj, proto), false);  // even if not changing anything

This isn't consistent with 9.1.2 steps 2-5.  Quick followup bug?

::: js/src/tests/ecma_6/Reflect/shell.js
@@ +1,5 @@
> +// List of a few values that are not objects.
> +var SOME_PRIMITIVE_VALUES = [
> +    undefined, null,
> +    false,
> +    1, 1.6e99, NaN,

Add -Infinity, -1.6e99, -1, -0, +0, Math.pow(2, -1074), 4294967295, Number.MAX_SAFE_INTEGER, Number.MAX_SAFE_INTEGER + 1, Infinity here.  Also (typeof objectEmulatingUndefined === "function" ? objectEmulatingUndefined() : null).

@@ +3,5 @@
> +    undefined, null,
> +    false,
> +    1, 1.6e99, NaN,
> +    "", "Phaedo",
> +    Symbol(), Symbol.for("good"), Symbol.iterator

Symbol.for("iterator") might be better/trippier.  Or something that distinguishes registered symbols from unregistered symbols from language-provided ones that all share the same description-ful thing.

::: js/src/tests/ecma_6/Reflect/surfaces.js
@@ +5,3 @@
>  assertEq(typeof Reflect, 'object');
>  assertEq(Object.getPrototypeOf(Reflect), Object.prototype);
>  assertEq(Reflect.toString(), '[object Object]');

assertThrowsInstanceOf(() => new Reflect, TypeError); to verify no [[Construct]], just as typeof verified no [[Call]].

@@ +36,5 @@
> +for (var name of Reflect.ownKeys(Reflect)) {
> +    // If this assertion fails, congratulations on implementing a new Reflect feature!
> +    // Add it to the list of methods above.
> +    if (name !== "parse")
> +      assertEq(name in methods, true);

Could add the comment as a message in the assertion, interpolating the name, for a nicer failure message.
Attachment #8627831 - Flags: review?(jwalden+bmo) → review+
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #31)
> Stars in wrong places.  (Is this something wrong in the entire patch,
> probably?  Please fix all of it.)

Yeah. It was originally written before the style change and I've been carrying the patches around ever since. I'll do a last pass before landing.
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #31)
In Reflect.cpp:
> > +    InvokeArgs &invokeArgs = fig.args();
> 
> And I guess & is in the wrong place, too.

fixed

> @@ +164,5 @@
> > + * TODO:
> > + * - Change type of JSObject::getGeneric et al to permit non-object receiver.
> 
> This comment needs rewriting slightly, at least.

rewroten

> > +    // See bug ######.
> 
> "This should be fixed in bug 603201."

thanks, fixed

> > +    // The other steps are identical to ES6 draft rev 32 (2015 Feb 2) 19.1.2.6
> > +    // Object.getOwnPropertyDescriptor.
> > +    return js::obj_getOwnPropertyDescriptor(cx, argc, vp);
>
> Is it that much hassle to just do the literal steps?  This is messier to
> verify for every reader, code size improvement seems negligible.

Verifying that two six-line chunks of spec text are effectively equivalent seems easier than verifying that a new chunk of C++ code correctly implements a chunk of spec text and also that the C++ isn't buggy in some horrible way.

> Fine enough, but to be honest, I'm leery about having Reflect.* operations
> not directly call the C++ versions.  Having a bug break Reflect.*, in ways
> identical to how an operator we might be testing using Reflect.* would work,
> seems unideal.  You see what I'm saying?

I see, but then why stop at js::HasProperty, under that argument? Why not have two redundant implementations of that as well? They'll be sure to have different bugs, as desired!

> ::: js/src/js.msg
> > +MSG_DEF(JSMSG_NOT_NONNULL_OBJECT,      1, JSEXN_TYPEERR, "{0} is not an object")
> 
> Nice.  Micro-bonus points for JSMSG_NOT_AN_OBJECT even, as a followup tweak
> if you want and all (note this would require bumping XDR, as I think we have
> this particular error name burned into self-hosted bytecode).

ARRRRGGGGHHH. I noticed last week that we have Jetpack tests that hard-code this error message. I'll have to fix it in a follow-up bug and ping the addon-sdk folks about the change... I'll do it though.

> uneval isn't standard, so putting this test in ecma_6/Reflect rather than
> ecma_6/extensions is bad.  Make testValues an array of [val, repr] pairs and
> put literal descriptive strings in as reprs.

killed the print statement

> > +function testMore(a) {
> 
> The proper spelling is testMoar, isn't it?

fixed, thx

> Please add a test that the generator protocol is *not* invoked for this junk.

nice, done

> Oh.  Never mind, then!

TOO LATE

> Add a test like this somewhere:

done

> Maybe use Object(Symbol...) or new Symbol(...) to ensure object-to-symbol
> conversion works?  And then check for obj[just the symbol, not an object]
> below.

propertyKeys.js covers it.

> > +assertEq(delete obj.prop, true);
> assertEq("prop" in proxy, false) for good measure?

done

In Reflect/defineProperty.js:
> Also writable: false?

Yes, except it has to be:
    assertEq("writable" in desc, false);

> typeof objectEmulatingUndefined === "function" ? objectEmulatingUndefined()
> : null

added, eeeeeeexcellent

> ::: js/src/tests/ecma_6/Reflect/get.js
> for (var v of thatPrimitivesArray)
>   assertThrowsInstanceOf(() => Reflect.get(v, "foo"), TypeError);

See target.js.

> ::: js/src/tests/ecma_6/Reflect/getPrototypeOf.js
> > +var proxy = new Proxy({}, {
> > +    getPrototypeOf(t) { return Math; }
> > +});
> > +var result = Reflect.getPrototypeOf(proxy);
> > +if (result === Math) {
> 
> This'll never be true, as if the target's [[Prototype]] and the trap result
> differ, a TypeError is thrown.

Only if the target is inextensible, right?

Thanks for looking extra close at these ---- I would hate to do something wrong while putting a sleeper test into its cryo-cell and have it never wake up...

> ::: js/src/tests/ecma_6/Reflect/has.js
> (arr, "-0"), false?

added

> ::: js/src/tests/ecma_6/Reflect/isExtensible.js
> > +    Object(Symbol("table")),
> 
> Is new Symbol("table") identical?

No, for some reason they decided to make that throw a TypeError. (i know don't even start)

> ::: js/src/tests/ecma_6/Reflect/propertyKeys.js
> > +if ("toPrimitive" in Symbol)
> > +    throw new Exception("Congratulations on implementing Symbol.toPrimitive! " +
> 
> Error?  I mean, it hardly matters, but.

heh fixed

> ::: js/src/tests/ecma_6/Reflect/set.js
> > +assertDeepEq(obj2, {prop: 47});
> 
> Would rather compare Object.getOwnPropertyDescriptor(obj2), { value: 47,
> writable: true, enumerable: true, configurable: true }, myself.

I changed this, but assertDeepEq does check attributes too.

> > +assertEq(obj.size, undefined);
> 
> Again, compare descriptors.

ok

> > +assertEq(obj.undefined, undefined);
> 
> Ditto.

ok

> >  assertEq(a[0], 42);
> 
> Descriptor comparison.

ok

> > +for (var defineReuslt of [true, false]) {
> 
> This is a typo.  I...think the test remains the same with this changed, just
> with better testing of what should have been tested.

yow, fixed

> ::: js/src/tests/ecma_6/Reflect/setPrototypeOf.js
> > +// Throw if the target argument is not an object.
> > +for (var value of [null, undefined, "fail"]) {
> 
> [...] is this needed if targets.js had
> a [null] entry and included this method?

no, deleted

> > +assertEq(Reflect.setPrototypeOf(obj, proto), false);  // even if not changing anything
> 
> This isn't consistent with 9.1.2 steps 2-5.  Quick followup bug?

Nice catch! Filed bug 1184414.

> ::: js/src/tests/ecma_6/Reflect/shell.js
> > +var SOME_PRIMITIVE_VALUES = [
> 
> Add -Infinity, -1.6e99, -1, -0, +0, Math.pow(2, -1074), 4294967295,
> Number.MAX_SAFE_INTEGER, Number.MAX_SAFE_INTEGER + 1, Infinity here.

done

> Also
> (typeof objectEmulatingUndefined === "function" ? objectEmulatingUndefined()
> : null).

But that is not a primitive value! A bunch of stuff fails if I do that.

> > +    Symbol(), Symbol.for("good"), Symbol.iterator
> 
> Symbol.for("iterator") might be better/trippier.  Or something that
> distinguishes registered symbols from unregistered symbols from
> language-provided ones that all share the same description-ful thing.

sure, ok

> ::: js/src/tests/ecma_6/Reflect/surfaces.js
> assertThrowsInstanceOf(() => new Reflect, TypeError); to verify no
> [[Construct]], just as typeof verified no [[Call]].

done

> > +    if (name !== "parse")
> > +      assertEq(name in methods, true);
> 
> Could add the comment as a message in the assertion, interpolating the name,
> for a nicer failure message.

done
Reflect.enumerate remains to be implemented (per comment 26).
See Bug 1185493.
Documentation

Developer release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/42#JavaScript

Reference pages:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/apply
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/construct
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/defineProperty
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/deleteProperty
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/enumerate
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/get
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/getOwnPropertyDescriptor
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/getPrototypeOf
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/has
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/isExtensible
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/ownKeys
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/preventExtensions
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/set
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/setPrototypeOf

Guide:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Meta_programming#Reflection

As always: review, additional examples, etc. are welcome and very much appreciated in the MDN wiki.
Release Note Request (optional, but appreciated)
[Why is this notable]: moar ES6
[Suggested wording]: Implemented ES6 Reflect
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Meta_programming#Reflection or https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect
relnote-firefox: --- → ?
Added to the release notes with your wording: "Implemented ES6 Reflect"
thanks!
Depends on: 1206700
You need to log in before you can comment on or make changes to this bug.