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

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:p1])

Attachments

(1 attachment)

This was very delicately written last time. Unfortunately, the spec has changed. We need to audit and clean this up
Component: JavaScript Engine → JavaScript: Standard Library
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [js:p1]
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.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8416775 - Flags: review?(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.
Attachment #8416775 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/6934964f2c99
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.