Closed Bug 978229 Opened 6 years ago Closed 5 years ago

ES6 Proxies: IsSealed() et al delenda est

Categories

(Core :: JavaScript: Standard Library, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: efaust, Assigned: efaust)

References

Details

(Whiteboard: [js:p1])

Attachments

(4 files)

Firstly, there's no way that passive paraphrastic is right, because "al" is misdeclined. (Alii, maybe? "Ad takes the?" "A-accusative!" "Right, so dom*um*")

These methods looks clever, but actually mask a few subtle mistakes in our implementations with respect to when we throw and how many times we call getOwnPropertyDescriptor on the target.
Component: JavaScript Engine → JavaScript: Standard Library
OS: Linux → All
Hardware: x86_64 → All
Whiteboard: [js:p1]
This isn't strictly necessary, but having a fallible overload of IdToValue which is only called when we are trying to expose that id as a string (property name) to a proxy trap is a poor name choice. Let's change it.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8387217 - Flags: review?(jorendorff)
This also removes a call of HasOwn, which should die for similar reasons.
Attachment #8387220 - Flags: review?(jorendorff)
Attachment #8387217 - Flags: review?(jorendorff) → review+
Comment on attachment 8387220 [details] [diff] [review]
Part 1: remove IsSealed from and cleanup [[HasProperty]]

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

Looks good.

HasOwn and IsSealed, good grief!

::: js/src/jit-test/tests/proxy/testDirectProxyHas6.js
@@ +8,5 @@
> +    'foo' in new Proxy(target, {
> +        has: function (target, name) {
> +            return false;
> +        }
> +    }), false);

One more thing to test: Don't throw a type error if the trap reports a nonexistent property as existing, regardless of extensibility. (!)

::: js/src/jsproxy.cpp
@@ +1949,5 @@
>      if (!Invoke(cx, ObjectValue(*handler), trap, ArrayLength(argv), argv, &trapResult))
>          return false;
>  
>      // step 6
>      bool success = ToBoolean(trapResult);;

Kill the extra ; while you're passing through?
Attachment #8387220 - Flags: review?(jorendorff) → review+
This is trivial, but still needs doing.
Attachment #8407610 - Flags: review?(jorendorff)
Attachment #8407610 - Flags: review?(jorendorff) → review+
the fight's not over yet.
Keywords: leave-open
Attachment #8437124 - Flags: review?(jwalden+bmo)
Attachment #8437124 - Flags: review?(jwalden+bmo) → review+
Is this done?
Flags: needinfo?(efaustbmo)
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
Flags: needinfo?(efaustbmo)
You need to log in before you can comment on or make changes to this bug.