1246318 - Remove [[Enumerate]] and associated reflective capabilities

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Tom S [:evilpie]]

tl;dr This is going away and we are better of that way.

Comment 1

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/handler/enumerate
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/enumerate

Comment 4

[Tom S [:evilpie]]

Attachment: Make the enumerate trap non-standard

The enumerate trap is not pure virtual anymore, so nobody is forced to implement this trap. In the first local iteration of this patch, I accidentally removed some calls to BaseProxyHandler::enumerate that were overwriting a call to the DirectProxyHandler version.

Comment 5

[Tom S [:evilpie]]

Attachment: Remove the still disabled Reflect.enumerate code

Comment 6

[Tom S [:evilpie]]

Attachment: Remove the enumerate trap from ES6 proxies

Comment 7

[Tom S [:evilpie]]

Attachment: Remove support for new style iterators with for..in

This is basically a partial backout of Bug 783829. There is no good reason to support new-style iterators anymore in this context.

Comment 8

[Eric Faust [:efaust]]

Comment on attachment 8716902 [details] [diff] [review]
Make the enumerate trap non-standard

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

r=me with question below addressed.

::: js/public/Proxy.h
@@ +58,5 @@
>   *     (CPOWs, implemented in js/ipc)
>   *
>   * ### Proxies and internal methods
>   *
> + * ES 2015 specifies 13 internal methods. The runtime  semantics of just

nit: Should say ES2016. We are removing it in this year's edition.

::: js/src/proxy/DeadObjectProxy.h
@@ +37,5 @@
>      virtual bool construct(JSContext* cx, HandleObject proxy, const CallArgs& args) const override;
>  
>      /* SpiderMonkey extensions. */
>      // BaseProxyHandler::getPropertyDescriptor will throw by calling getOwnPropertyDescriptor.
> +    // BaseProxyHandler::enumerate will throw by calling ownKeys.

Didn't we just remove BPH::enumerate?

Comment 9

[Eric Faust [:efaust]]

Comment on attachment 8716994 [details] [diff] [review]
Remove support for new style iterators with for..in

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

I'm kinda surprised to see this. Do we not do new iterators internally?

Comment 10

[Tom S [:evilpie]]

(In reply to Eric Faust [:efaust] from comment #8)
> Comment on attachment 8716902 [details] [diff] [review]
> Make the enumerate trap non-standard
> 
> Review of attachment 8716902 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: js/src/proxy/DeadObjectProxy.h
> @@ +37,5 @@
> >      virtual bool construct(JSContext* cx, HandleObject proxy, const CallArgs& args) const override;
> >  
> >      /* SpiderMonkey extensions. */
> >      // BaseProxyHandler::getPropertyDescriptor will throw by calling getOwnPropertyDescriptor.
> > +    // BaseProxyHandler::enumerate will throw by calling ownKeys.
> 
> Didn't we just remove BPH::enumerate?
Actually not yet. I moved BPH::enumerate to the non-standard section of BPH. It is also not pure-virtual anymore, so nobody is required to implement enumerate.

However this made me realize that we can probably completely remove BPH::enumerate, but this will break the enumerate (called iterate in this case) trap for ScriptedIndirectProxy. For that matter Wrappers are the only other special case, because they usually inherit the DirectProxy::enumerate trap, which calls GetIterator(target).

Comment 11

[Tom S [:evilpie]]

(In reply to Eric Faust [:efaust] from comment #9)
> Comment on attachment 8716994 [details] [diff] [review]
> Remove support for new style iterators with for..in
> 
> Review of attachment 8716994 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'm kinda surprised to see this. Do we not do new iterators internally?

Okay. So internally, most of the time we end up with NativeIteratorNext, so we never actually take a "standard" path. With the changes in ES2016 you can't create any custom iterator for for..in. However we have our very old non-standard extensions Iterator and __iterator__. Iterator still uses the old iteration protocol. __iterator__ is of course user defined, but I don't see why we should support the new iteration protocol there.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fd209a5854c0
https://hg.mozilla.org/integration/mozilla-inbound/rev/da0d807517ca
https://hg.mozilla.org/integration/mozilla-inbound/rev/874423b7e907

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/fd209a5854c0
https://hg.mozilla.org/mozilla-central/rev/da0d807517ca
https://hg.mozilla.org/mozilla-central/rev/874423b7e907

Comment 14

[Eric Faust [:efaust]]

Comment on attachment 8716903 [details] [diff] [review]
Remove the still disabled Reflect.enumerate code

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

Stealing, since Jason is out of the office this week. APPROVED.

Comment 15

[Pulsebot]

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

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/be82a484968d

Comment 17

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-CA/docs/2016/proxy-s-enumerate-handler-has-been-removed/

Comment 18

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/Firefox/Releases/47
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/handler/enumerate
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Reflect/enumerate