Remove Proxy.create and Proxy.createFunction

RESOLVED FIXED in Firefox 48

Status

()

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: bruant.d, Assigned: evilpie)

Tracking

(Blocks 1 bug, {addon-compat, dev-doc-complete, site-compat})

unspecified
mozilla48
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox48 fixed)

Details

(Whiteboard: [DocArea=JS])

Attachments

(3 attachments, 1 obsolete attachment)

Reporter

Description

6 years ago
They won't be part of ES6 (replaced by direct proxies). People use them while they shouldn't. Chrome hiding them behind a flag is the only thing that doesn't make them de facto standards (which would be an very unfortunate thing).
FYI, it is also used in the test suite to cause deterministic bailouts in IonMonkey.

Ideally, we should replaced them by a js-shell primitive functions which will be replaced by a bailout.
Reporter

Comment 2

6 years ago
(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> FYI, it is also used in the test suite to cause deterministic bailouts in
> IonMonkey.
> 
> Ideally, we should replaced them by a js-shell primitive functions which
> will be replaced by a bailout.
By any chance, would the use of direct proxies (new Proxy(target, handler)) make the trick in the short term if a primitive bailout function takes to much time to add?
We should start simpler than just trying to remove these: make them emit a deprecation warning when called, linking to a page talking about how to migrate to |new Proxy|.  Once that's out, that'll cut down a lot on the seeming burden of existing uses.
Reporter

Updated

6 years ago
Blocks: es6
(In reply to David Bruant from comment #0)
> They won't be part of ES6 (replaced by direct proxies). People use them
> while they shouldn't. Chrome hiding them behind a flag is the only thing
> that doesn't make them de facto standards (which would be an very
> unfortunate thing).

I'm glad the Web hasn't been filled with full of mozProxy, webkitProxy, msProxy, oProxy, blinkProxy, servoProxy, smProxy, v8Proxy, jscProxy, chakraProxy, carakanProxy, etc. Experimental flags certainly work.
(Of course, we should not have shipped Proxy.create without flags from the start.)
Assignee

Updated

5 years ago
Depends on: 989426
Assignee

Comment 6

5 years ago
Posted patch v1 (obsolete) — Splinter Review
Let's first warn about this.
Attachment #8398653 - Flags: review?(jwalden+bmo)
Comment on attachment 8398653 [details] [diff] [review]
v1

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

::: js/src/js.msg
@@ +226,5 @@
>  MSG_DEF(JSMSG_NOT_A_CODEPOINT,        173, 1, JSEXN_RANGEERR, "{0} is not a valid code point")
>  MSG_DEF(JSMSG_BRACKET_AFTER_ARRAY_COMPREHENSION, 174, 0, JSEXN_SYNTAXERR, "missing ] after array comprehension")
>  MSG_DEF(JSMSG_NESTING_GENERATOR,      175, 0, JSEXN_TYPEERR, "already executing generator")
>  MSG_DEF(JSMSG_PAREN_AFTER_FOR_OF_ITERABLE, 176, 0, JSEXN_SYNTAXERR, "missing ) after for-of iterable")
> +MSG_DEF(JSMSG_DEPRECATED_PROXY_CREATE,177, 0, JSEXN_TYPEERR, "Proxy.create and Proxy.createFunction are deprecated use new Proxy instead")

Add a semicolon after "deprecated".

::: js/src/vm/GlobalObject.h
@@ +110,5 @@
>      static const unsigned REGEXP_STATICS          = DATE_TIME_FORMAT_PROTO + 1;
>      static const unsigned WARNED_WATCH_DEPRECATED = REGEXP_STATICS + 1;
>      static const unsigned WARNED_PROTO_SETTING_SLOW = WARNED_WATCH_DEPRECATED + 1;
> +    static const unsigned WARNED_PROXY_CREATE_DEPRECATED = WARNED_PROTO_SETTING_SLOW + 1;
> +    static const unsigned RUNTIME_CODEGEN_ENABLED = WARNED_PROXY_CREATE_DEPRECATED + 1;

We used to have a FLAGS slot that held checks and stuff like this.  At some point it, er, grew to hold only a single flag, and then someone made it a boolean.  But now we're back to having a bunch of these.  Might be worth going back to having a single FLAGS slot in a separate patch.
Attachment #8398653 - Flags: review?(jwalden+bmo) → review+
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Assignee: general → nobody
No longer blocks: es6
Reporter

Updated

5 years ago
Depends on: 1110457
Assignee

Updated

5 years ago
Blocks: 1103158
Duplicate of this bug: 942569
Assignee

Updated

4 years ago
Depends on: 1243805
Assignee

Updated

4 years ago
Assignee: nobody → evilpies
Assignee

Updated

3 years ago
Depends on: 1244442
Assignee

Comment 10

3 years ago
Comment on attachment 8398653 [details] [diff] [review]
v1

Moved to bug 1244442.
Attachment #8398653 - Attachment is obsolete: true
Assignee

Updated

3 years ago
Depends on: 1245141
Assignee

Comment 11

3 years ago
I tried updating most handwritten tests where it made sense. However I just deleted a lot of fuzz tests, because making them fail or succeed in the same way was usually just not worth it.
Attachment #8721562 - Flags: review?(efaustbmo)
Assignee

Comment 12

3 years ago
Same disclaimer.
Attachment #8721563 - Flags: review?(efaustbmo)
Assignee

Comment 13

3 years ago
I am actually surprised how self-contained this code seems to be.

We can't quite land this yet, because there are still uses in the addon-sdk and in crash tests. I will probably have to fix those myself.
Attachment #8721564 - Flags: review?(efaustbmo)
Comment on attachment 8721562 [details] [diff] [review]
Remove Proxy.create from jit-tests

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

::: js/src/jit-test/tests/baseline/bug881461.js
@@ +1,1 @@
> +z = new Proxy({__proto__: (function(){})}, {});

why can't we just use the function directly, here, like the last test seemed to?

::: js/src/jit-test/tests/basic/bug1085464.js
@@ +1,2 @@
>  function *f() {
> +    var o = new Proxy({}, {

I guess this test doesn't rely on [[Call]] behavior.

::: js/src/jit-test/tests/basic/bug1113980.js
@@ +3,5 @@
>          return {value: 1, configurable: true, writable: true};
>      },
>      defineProperty: function() {
>      }
>  }, null);

what does this null do, in the new world? It's just ignored, right?

::: js/src/jit-test/tests/basic/bug787847.js
@@ +7,3 @@
>  }
>  
> +var p = new Proxy({}, { get: get } );

do we need to change this trap? There's still a getOwnProperty, or something, right?

::: js/src/jit-test/tests/basic/splice-check-steps.js
@@ +21,2 @@
>      },
> +    // derived traps

there's no longer any such thing as a "derived trap". Do we need this comment?

::: js/src/jit-test/tests/basic/testCrossCompartmentTransparency.js
@@ +8,4 @@
>  "    defineProperty: () =>assertEq(true,false),            "+
> +"    deleteProperty: () =>assertEq(true,false),            "+
> +"    get: () =>assertEq(true,false),                       "+
> +"    set: () =>assertEq(true,false),                       "+

"fix" is preventExtensions, right? Do we want do add that here, also?

::: js/src/jit-test/tests/basic/testProxyDefinePropertyWithMissingSetter.js
@@ +9,4 @@
>        Object.defineProperty(x, name, desc)
>      },
> +});
> +

is the target here not eval? Shouldn't we maintain that?

::: js/src/jit-test/tests/debug/Object-callable.js
@@ +13,5 @@
>  g.eval("f({}, false);");
>  g.eval("f(Function.prototype, true);");
>  g.eval("f(f, true);");
> +g.eval("f(new Proxy({}, {}), false);");
> +g.eval("f(new Proxy(f, {}), true);");

I assume that omitting f here (and below) as the handler is sensible, since it has no trap properties. If so, no objections.

::: js/src/jit-test/tests/debug/Object-name-02.js
@@ +11,5 @@
>  
>  g.eval("f({});");
>  g.eval("f(/a*/);");
>  g.eval("f({name: 'bad'});");
> +g.eval("f(new Proxy({}, {}));");

sure, though I mildly prefer sticking with the function target

::: js/src/jit-test/tests/for-of/semantics-11.js
@@ +5,5 @@
>  var s = '';
>  
>  var i = 0;
> +var next_fn = new Proxy(function() {}, {
> +    apply() {

why is it better here (and below) to have he trap instead of just putting this body inside the target function?
Attachment #8721562 - Flags: review?(efaustbmo) → review+
Attachment #8721563 - Flags: review?(efaustbmo) → review+
Comment on attachment 8721564 [details] [diff] [review]
Remove Proxy.create and Proxy.createFunction

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

And I'll dance on its grave. r=me with prejudice.
Attachment #8721564 - Flags: review?(efaustbmo) → review+
Assignee

Updated

3 years ago
Depends on: 1253866
And I will dance on its grave. \o/ \o/ \o/
This is a great day.
Assignee

Updated

3 years ago
Keywords: addon-compat
Developer release notes:
https://developer.mozilla.org/en-US/Firefox/Releases/48#JavaScript
(there are no real docs for the legacy Proxy API anymore, standard Proxy is documented, though)
Duplicate of this bug: 586168
You need to log in before you can comment on or make changes to this bug.