Closed Bug 1246318 Opened 8 years ago Closed 8 years ago

Remove [[Enumerate]] and associated reflective capabilities

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: evilpie, Assigned: evilpie)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [DocArea=JS])

Attachments

(4 files)

tl;dr This is going away and we are better of that way.
Assignee: evilpies → nobody
Assignee: nobody → evilpies
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.
Attachment #8716902 - Flags: review?(efaustbmo)
Attachment #8716903 - Flags: review?(jorendorff)
Attachment #8716980 - Flags: review?(efaustbmo)
Attachment #8716902 - Attachment is patch: true
This is basically a partial backout of Bug 783829. There is no good reason to support new-style iterators anymore in this context.
Attachment #8716994 - Flags: review?(efaustbmo)
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?
Attachment #8716902 - Flags: review?(efaustbmo) → review+
Attachment #8716980 - Flags: review?(efaustbmo) → review+
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?
Attachment #8716994 - Flags: review?(efaustbmo) → review+
(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).
(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.
Whiteboard: [DocArea=JS] → [DocArea=JS], keep-open
Keywords: leave-open
Whiteboard: [DocArea=JS], keep-open → [DocArea=JS]
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.
Attachment #8716903 - Flags: review?(jorendorff) → review+
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/be82a484968d
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: