1296236 - Proxy revocation function should be anonymous

Status: RESOLVED FIXED

(Core :: JavaScript: Standard Library, defect, P3)

Description

[André Bargull [:anba]]

Test case:
---
var revocationFunction = Proxy.revocable({}, {}).revoke;

assertEq(revocationFunction.hasOwnProperty("name"), false);
---

Expected: assertEq call succeeds
Actual: assertEq call fails

https://tc39.github.io/ecma262/#sec-proxy-revocation-functions

Comment 2

[Luna Phipps-Costin]

The test isn't quite right is it? Take this example:

assertEq((function(){}).name, "");
assertEq((function(){}).hasOwnProperty("name"), true);

both pass. So the correct test is:

var revocationFunction = Proxy.revocable({}, {}).revoke;
assertEq(revocationFunction.name, "");

Comment 3

[Luna Phipps-Costin]

Attachment: Bug 1296236 - The revoke function created by Proxy.revocable() should be anonymous. r=jorendorff

Comment 4

[André Bargull [:anba]]

(In reply to Luna Phipps-Costin from comment #2)

The test isn't quite right is it? Take this example:

The test is correct, SpiderMonkey (and most other engines) simply doesn't implement the current spec correctly, but instead always assigns an own "name" property even if none should be present per spec. There is an open proposal to change the spec to follow web-reality at https://github.com/tc39/ecma262/issues/1049.

Comment 5

[Luna Phipps-Costin]

Shouldn't that issue be a separate bug then? This patch brings the Proxy interface at least to consistency with SpiderMonkey / "Web Reality"

Comment 6

[Jason Orendorff [:jorendorff]]

OK, having read the github discussion, I agree with Luna. Reviewing the patch now...

Comment 7

[Luna Phipps-Costin]

Okay, I think I shouldn't have changed that name at all, I changed it when I was trying to understand the code and forgot to change it back. I'm not sure how to send a fixup commit to Phabricator, I ended up accidentally making a whole new Phabricator URL, let me know if that's a problem

Comment 8

[André Bargull [:anba]]

(In reply to Luna Phipps-Costin from comment #5)

Shouldn't that issue be a separate bug then?

Err, this bug report was about not having a "name" property for the revoke function, which in spec terms means that the function should be anonymous. Or in other words: In the spec, an anonymous function is a function without a "name" property, which is different from a function whose "name" property is the empty string. So basically the patch does something different than what was described in the bug title and comment #0.

Comment 9

[Jason Orendorff [:jorendorff]]

André: Right, this bug has been hijacked, but I think we should just go with it...

This patch is an improvement even if the current standard remains: it fixes some uses, just not all.

I don't think it'd be super hard to implement the current standard in SM, but it's a bad idea to move away from a strong consensus behavior. I think the spec should change.

So, I'm going to close this bug and bug 1296235 after landing this patch, and I'm moving the related entries in jstests.list to the "# Tests disabled due to intentional alternative implementations #" section. If TC39 decides not to change anything and V8 changes behavior, we'll reopen.

Comment 10

[André Bargull [:anba]]

Bug 1296234 is another case where the spec wants to have an anonymous function, but we're currently assigning an own name. For example print(Intl.Collator().compare.name) prints "bound collatorCompareToBind".

Comment 11

[André Bargull [:anba]]

(In reply to Jason Orendorff [:jorendorff] from comment #9)

André: Right, this bug has been hijacked, but I think we should just go with it...

This patch is an improvement even if the current standard remains: it fixes some uses, just not all.

Yes, absolutely. The patch is definitely better than what we currently have!

I don't think it'd be super hard to implement the current standard in SM, but it's a bad idea to move away from a strong consensus behavior. I think the spec should change.

It's probably not hard to implement the standard for the revoke function, but for all functions it could be annoying because we'd have to change how bound function creation is optimised. (Simply setting JSFunction::RESOLVED_NAME for anonymous functions to avoid running the resolve hook would disable this bound function optimisation, so we'd need to change CodeGenerator::visitFinishBoundFunctionInit to somehow detect anonymous functions, for example by adding a new function flag...)

Comment 12

[André Bargull [:anba]]

And bug 1296237 for another anonymous function bug.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/68b3ab27cda5
The revoke function created by Proxy.revocable() should be anonymous. r=jwalden

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/68b3ab27cda5