892903 - Remove Proxy.create and Proxy.createFunction

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[David Bruant]

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

Comment 1

[Nicolas B. Pierron [:nbp]]

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.

Comment 2

[David Bruant]

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

Comment 3

[Jeff Walden [:Waldo]]

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.

Comment 4

[Masatoshi Kimura [:emk]]

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

Comment 5

[Tom S [:evilpie]]

Shumway bug: https://github.com/mozilla/shumway/issues/1232

Comment 6

[Tom S [:evilpie]]

Attachment: v1

Let's first warn about this.

Comment 7

[Jeff Walden [:Waldo]]

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.

Comment 9

[Kohei Yoshino [:kohei]]

Posted the site compatibility doc: https://www.fxsitecompat.com/en-US/docs/2015/legacy-proxy-api-will-be-removed/

Comment 10

[Tom S [:evilpie]]

Comment on attachment 8398653 [details] [diff] [review]
v1

Moved to bug 1244442.

Comment 11

[Tom S [:evilpie]]

Attachment: Remove Proxy.create from jit-tests

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.

Comment 12

[Tom S [:evilpie]]

Attachment: Remove Proxy.create from jstests

Same disclaimer.

Comment 13

[Tom S [:evilpie]]

Attachment: Remove Proxy.create and Proxy.createFunction

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.

Comment 14

[Eric Faust [:efaust]]

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?

Comment 15

[Eric Faust [:efaust]]

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.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f999b1995dc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/346979205b0a
https://hg.mozilla.org/integration/mozilla-inbound/rev/0607c9f8df24

Comment 17

[Eric Faust [:efaust]]

And I will dance on its grave. \o/ \o/ \o/

Comment 18

[Jason Orendorff [:jorendorff]]

This is a great day.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/f999b1995dc6
https://hg.mozilla.org/mozilla-central/rev/346979205b0a
https://hg.mozilla.org/mozilla-central/rev/0607c9f8df24

Comment 20

[Kohei Yoshino [:kohei]]

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

Comment 21

[Florian Scholz (Open Web Docs)]

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)