Closed Bug 1296236 Opened 4 years ago Closed 10 months ago

Proxy revocation function should be anonymous

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox51 --- wontfix
firefox68 --- fixed

People

(Reporter: anba, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Blocks: test262
Duplicate of this bug: 1427058
Priority: -- → P3

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, "");

(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.

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

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

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

(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.

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.

Attachment #9050531 - Attachment description: Bug 1296236 - Don't include name in revoke construction r?jorendorff → Bug 1296236 - The revoke function created by Proxy.revocable() should be anonymous. r=jorendorff

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".

(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...)

And bug 1296237 for another anonymous function bug.

Assignee: nobody → jorendorff
Pushed by jorendorff@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/68b3ab27cda5
The revoke function created by Proxy.revocable() should be anonymous. r=jwalden
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68
You need to log in before you can comment on or make changes to this bug.