978229 - ES6 Proxies: IsSealed() et al delenda est

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect)

Description

[Eric Faust [:efaust]]

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.

Comment 1

[Eric Faust [:efaust]]

Attachment: Part 0: General cleanup: Rename jsproxy.cpp internal IdToValue overload

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.

Comment 2

[Eric Faust [:efaust]]

Attachment: Part 1: remove IsSealed from and cleanup [[HasProperty]]

This also removes a call of HasOwn, which should die for similar reasons.

Comment 3

[Jason Orendorff [:jorendorff]]

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?

Comment 4

[Eric Faust [:efaust]]

Attachment: Part 2: remove IsSealed from [[Delete]]

This is trivial, but still needs doing.

Comment 5

[Eric Faust [:efaust]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/8d3be1c62d8d
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/21d545a5ac3f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/159a32e886d0

Comment 6

[Eric Faust [:efaust]]

the fight's not over yet.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/8d3be1c62d8d
https://hg.mozilla.org/mozilla-central/rev/21d545a5ac3f
https://hg.mozilla.org/mozilla-central/rev/159a32e886d0

Comment 8

[Eric Faust [:efaust]]

Attachment: rm now unused NormalizePropertyDescriptor...

Comment 9

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/06bd71f490fa

Comment 10

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ed25ba82a9d

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/06bd71f490fa
https://hg.mozilla.org/mozilla-central/rev/8ed25ba82a9d

Comment 12

[Jason Orendorff [:jorendorff]]

Is this done?