978236 - ES6 Proxies: [[DefineProperty]] is in the weeds

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Eric Faust [:efaust]]

This was very delicately written last time. Unfortunately, the spec has changed. We need to audit and clean this up

Comment 1

[Eric Faust [:efaust]]

Attachment: definePropertyNoOrigin.patch

OK, it's too slow to work on Proxy PropDesc stuff. We still use the PropDesc cleanups, but at least we're now back to moving on ES6 stuff.

On the other hand, this is blatantly wrong with respect to [[Origin]], but at least that's still specwise in the air.

There were several spec-based concerns raised while writing this patch:

9.1.6.3 both steps 5a and 8a.i refer to properties of desc without ensureing that they are present. Is this intentional? What assertions are being used to verify that information? The implementation does not make such blithe assumptions about their presence.

Comment 2

[Jason Orendorff [:jorendorff]]

Comment on attachment 8416775 [details] [diff] [review]
definePropertyNoOrigin.patch

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

r=me with these comments addressed.

::: js/src/jsproxy.cpp
@@ +1201,5 @@
>  
>  // Aux.5 ValidateProperty(O, P, Desc)
>  static bool
> +ValidateProperty(JSContext *cx, Handle<PropertyDescriptor> current, Handle<PropDesc> desc,
> +                 bool *bp)

Where are these new step numbers coming from? In ES6, this stuff happens in ValidateAndApplyPropertyDescriptor, right?

@@ +1304,5 @@
> +    // step 1
> +    Rooted<PropertyDescriptor> current(cx);
> +    if (!GetOwnPropertyDescriptor(cx, obj, id, &current))
> +        return false;
> +    

Please delete the space characters on this blank line.

@@ +1685,5 @@
> +    if (!JSObject::getProperty(cx, handler, handler, cx->names().defineProperty, &trap))
> +         return false;
> +
> +    // step 7
> +    // FIXME: This is incorrect with respect to [[Origin]]

Step 7 doesn't do anything special regarding [[Origin]]. This seems like it was accidentally left here, a stray copy of the same comment on steps 8-9.

@@ +1711,5 @@
> +        return false;
> +
> +    // step 11, 13
> +    if (ToBoolean(trapResult)) {
> +        // step 14

14-15

(To prevent the reader from wondering if step 15 is implemented somewhere else or what.)

@@ +1716,5 @@
> +        Rooted<PropertyDescriptor> targetDesc(cx);
> +        if (!GetOwnPropertyDescriptor(cx, target, id, &targetDesc))
> +            return false;
> +
> +        // step 16

16-17

@@ +1722,5 @@
> +        if (!JSObject::isExtensible(cx, target, &extensibleTarget))
> +            return false;
> +
> +        // step 18-19
> +        // FIXME: This is wrong with respect to [[Origin]]

Again, it isn't. It's wrong with respect to hasConfigurable but that seems hardly worth commenting right here... except maybe as a note to your future self?

@@ +1736,5 @@
> +                JS_ReportErrorNumber(cx, js_GetErrorMessage, nullptr, JSMSG_CANT_DEFINE_NE_AS_NC);
> +                return false;
> +            }
> +        }
> +        // step 21

I think this implementation of step 21 needs to be in an else-block. How does it work this way?

@@ +1749,5 @@
> +            return false;
> +        }
> +    }
> +    // In theory, we're supposed to return various values, but they're ignored everywhere
> +    // in the engine, so it seems pointless.

Style nit: blank line before this comment since it's not at the top of a block.

This comment seems really misleading. Please make it say:

// [[DefineOwnProperty]] is specified to return false if defining the property
// failed in a way that should sometimes, but not always, result in a TypeError
// (e.g. in strict assignment). Not implemented yet: bug NNNNNNN.

and file the relevant bug (with "Blocks: harmony") if it's not there already.

Comment 3

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e89d2416585f

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Backed out for jsreftest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbc90d9dafdb

https://tbpl.mozilla.org/php/getParsedLog.php?id=40981766&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=40981611&tree=Mozilla-Inbound

Comment 5

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6934964f2c99

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6934964f2c99