546532 - make narcissus use ES5 defineProperty

Status: RESOLVED FIXED

(Other Applications Graveyard :: Narcissus, defect)

Comment 1

[Andreas Gal :gal]

Attachment: patch

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 427224 [details] [diff] [review]
patch

>@@ -41,33 +44,21 @@
>  *      the global object (singleton)
>  *      eval
>  *      function objects, Function
>  *
>  * SpiderMonkey extensions used:
>  *      catch guards
>  *      const declarations
>  *      get and set functions in object initialisers
>- *      Object.prototype.__defineGetter__
>- *      Object.prototype.__defineSetter__
>- *      Object.prototype.__defineProperty__

Please do note ES5 dependency, however -- people porting to other browsers will have to cope until those browsers implement ES5. Nightly WebKit support is good.

/be

Comment 3

[Jeff Walden [:Waldo]]

I don't think WebKit supports catch guards, so portability seems absent regardless.  Now, if you wanted to kill the uses of catch guards to provide that, maybe const as well since it's not ES5, to provide portability, that'd be cool, but worrying about portability without those changes seems premature.

You can remove get/set in initializers as well while you're here, since that's in ES5 too.

Comment 4

[Andreas Gal :gal]

Yeah, I will try to work through 3. The intermediate step is to make this work without #define NARCISSUS.

Comment 5

[Brendan Eich [:brendan]]

Waldo, you've got me all wrong :-P. I don't give a rat's behind about Narcissus portability, having written it to use const and catch guards originally.

/be

Comment 6

[Jeff Walden [:Waldo]]

Guess I do!  I don't much care about portability here either, except to the extent that the fewer SpiderMonkey-isms we have the more approachable the code becomes.  But that's a tangential concern, really.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 427224 [details] [diff] [review]
patch

Clearing this request until the meta-circular problems you were seeing have been debugged.

Comment 10

[Tom Austin]

With Andreas's patch, the workaround for __applyConstructor__ (bug #573792), and a couple of tweaks to the narcissus shell (#572879), narcissus almost passes all of the same unit tests in the standard build as it does in the special build:

  $ python jstests.py -d -j 4 ../narcShell.py -m narcissus.list -x narcFailuresFull.txt
  [1096|  52| 150] 100% ===============================================>|  473.1s

Comment 11

[Dave Herman [:dherman]]

Tom--

If you wouldn't mind posting patches as you make progress, I would love to be able to start trying them out. Don't worry about them being incomplete, just leave off any request for review until you're ready.

Dave, itching to start prototyping Harmony features

Comment 12

[Tom Austin]

Here are the links.
*"Shell" for Narcissus--
    https://bug572879.bugzilla.mozilla.org/attachment.cgi?id=452937
*Patch to work around __applyConstructor__
    https://bug573792.bugzilla.mozilla.org/attachment.cgi?id=453226
*Some adjustments to the testing setup for Narcissus
    https://bug572879.bugzilla.mozilla.org/attachment.cgi?id=453596

I've got some more updates to the shell to make it work without browser extensions.  I'll get a patch for that together and upload shortly.

Comment 13

[Tom Austin]

Attachment: Patch using Proxy.createFunction to enable __call__ to work

With this and the previous patches, Narcissus without special extensions is down to a single (additional) failing unit test:

  $ python jstests.py -d -j 4 ../narcShell.py -m narcissus.list -x narcFailuresFull.txt
  [1147|   1| 150] 100% ===============================================>|  508.5s
  FIXES
      narcissus/../js1_8_5/regress/regress-555246-1.js
  REGRESSIONS
      narcissus/../ecma_3/Function/regress-313570.js
  FAIL

Comment 14

[Andreas Gal :gal]

Comment on attachment 455609 [details] [diff] [review]
Patch using Proxy.createFunction to enable __call__ to work

>diff -r 7654745d9944 js/narcissus/jsexec.js
>--- a/js/narcissus/jsexec.js	Fri Jun 25 12:05:19 2010 -0700
>+++ b/js/narcissus/jsexec.js	Thu Jul 01 18:57:37 2010 -0700
>@@ -104,17 +104,17 @@ var global = {
>         // XXX We want to pass a good file and line to the tokenizer.
>         // Note the anonymous name to maintain parity with Spidermonkey.
>         var t = new Tokenizer("anonymous(" + p + ") {" + b + "}");
> 
>         // NB: Use the STATEMENT_FORM constant since we don't want to push this
>         // function onto the null compilation context.
>         var f = FunctionDefinition(t, null, false, STATEMENT_FORM);
>         var s = {object: global, parent: null};
>-        return new FunctionObject(f, s);
>+        return makeFunction(f,{scope:s});
>     },
>     Array: function (dummy) {
>         // Array when called as a function acts as a constructor.
>         return Array.apply(this, arguments);
>     },
>     String: function String(s) {
>         // Called as function or constructor: convert argument to string type.
>         s = arguments.length ? "" + s : "";
>@@ -239,38 +239,38 @@ function toObject(v, r, rn) {
> 
> function execute(n, x) {
>     var a, f, i, j, r, s, t, u, v;
> 
>     switch (n.type) {
>       case FUNCTION:
>         if (n.functionForm != DECLARED_FORM) {
>             if (!n.name || n.functionForm == STATEMENT_FORM) {
>-                v = new FunctionObject(n, x.scope);
>+                v = makeFunction(n, x);
>                 if (n.functionForm == STATEMENT_FORM)
>                     defineProperty(x.scope.object, n.name, v, true);
>             } else {
>                 t = new Object;
>                 x.scope = {object: t, parent: x.scope};
>                 try {
>-                    v = new FunctionObject(n, x.scope);
>+                    v = makeFunction(n, x);
>                     defineProperty(t, n.name, v, true, true);
>                 } finally {
>                     x.scope = x.scope.parent;
>                 }
>             }
>         }
>         break;
> 
>       case SCRIPT:
>         t = x.scope.object;
>         a = n.funDecls;
>         for (i = 0, j = a.length; i < j; i++) {
>             s = a[i].name;
>-            f = new FunctionObject(a[i], x.scope);
>+            f = makeFunction(a[i], x);
>             defineProperty(t, s, f, x.type != EVAL_CODE);
>         }
>         a = n.varDecls;
>         for (i = 0, j = a.length; i < j; i++) {
>             u = a[i];
>             s = u.name;
>             if (u.readOnly && hasDirectProperty(t, s)) {
>                 throw new TypeError("Redeclaration of const " + s,
>@@ -714,17 +714,17 @@ function execute(n, x) {
> 
>       case OBJECT_INIT:
>         v = {};
>         for (i = 0, j = n.length; i < j; i++) {
>             t = n[i];
>             if (t.type == PROPERTY_INIT) {
>                 v[t[0].value] = getValue(execute(t[1], x));
>             } else {
>-                f = new FunctionObject(t, x.scope);
>+                f = makeFunction(t, x);
>                 u = (t.type == GETTER) ? '__defineGetter__'
>                                        : '__defineSetter__';
>                 v[u](t.name, thunk(f, x));
>             }
>         }
>         break;
> 
>       case NULL:
>@@ -785,16 +785,69 @@ function FunctionObject(node, scope) {
>     this.node = node;
>     this.scope = scope;
>     defineProperty(this, "length", node.params.length, true, true, true);
>     var proto = {};
>     defineProperty(this, "prototype", proto, true);
>     defineProperty(proto, "constructor", this, false, false, true);
> }
> 
>+// Returns a new function wrapped with a Proxy.
>+function makeFunction(n,x) {
>+    var f = new FunctionObject(n, x.scope);
>+    var p = Proxy.createFunction(handlerMaker(f),
>+            function() { return f.__call__(this, arguments, x); },
>+            function() { return f.__construct__(arguments, x); });
>+    return p;
>+}
>+

Just make this a lambda function in makeFunction() above. And maybe call it "newFunction()".

>+//Create handler for object to use with proxies
>+function handlerMaker(obj) {
>+  return {
>+   getOwnPropertyDescriptor: function(name) {
>+     var desc = Object.getOwnPropertyDescriptor(obj);

Newline.

>+     // a trapping proxy's properties must always be configurable
>+     desc.configurable = true;
>+     return desc;
>+   },
>+   getPropertyDescriptor:  function(name) {
>+     var desc = Object.getPropertyDescriptor(obj); // assumed

Newline.

>+     // a trapping proxy's properties must always be configurable
>+     desc.configurable = true;
>+     return desc;
>+   },
>+   getOwnPropertyNames: function() {
>+     return Object.getOwnPropertyNames(obj);
>+   },
>+   defineProperty: function(name, desc) {
>+     Object.defineProperty(obj, name, desc);
>+   },
>+   delete:       function(name) { return delete obj[name]; },   
>+   fix:          function() {
>+     if (Object.isFrozen(obj)) {
>+       return Object.getOwnProperties(obj); // assumed
>+     }

Newline.

>+     // As long as obj is not frozen, the proxy won't allow itself to be fixed

Period.

>+     return undefined; // will cause a TypeError to be thrown
>+   },
>+ 
>+   has:          function(name) { return name in obj; },
>+   hasOwn:       function(name) { return ({}).hasOwnProperty.call(obj, name); },
>+   get:          function(receiver, name) { return obj[name]; },
>+   set:          function(receiver, name, val) { obj[name] = val; return true; }, // bad behavior when set fails in non-strict mode
>+   enumerate:    function() {
>+     var result = [];
>+     for (name in obj) { result.push(name); };
>+     return result;
>+   },
>+   enumerateOwn: function() { return Object.keys(obj); }
>+ 
>+  };
>+}
>+
> var FOp = FunctionObject.prototype = {
>     // Internal methods.
>     __call__: function (t, a, x) {
>         var x2 = new ExecutionContext(FUNCTION_CODE);
>         x2.thisObject = t || global;
>         x2.caller = x;
>         x2.callee = this;
>         defineProperty(a, "callee", this, false, false, true);
>diff -r 7654745d9944 js/src/tests/ecma/Array/15.4.4.5-3.js
>--- a/js/src/tests/ecma/Array/15.4.4.5-3.js	Fri Jun 25 12:05:19 2010 -0700
>+++ b/js/src/tests/ecma/Array/15.4.4.5-3.js	Thu Jul 01 18:57:37 2010 -0700
>@@ -41,17 +41,17 @@ gTestfile = '15.4.4.5-3.js';
> /**
>    File Name:          15.4.4.5-3.js
>    ECMA Section:       Array.prototype.sort(comparefn)
>    Description:
> 
>    This is a regression test for
>    http://scopus/bugsplat/show_bug.cgi?id=117144
> 
>-   Verify that sort is successfull, even if the sort compare function returns

Not sure you have to fix this.

>+   Verify that sort is successful, even if the sort compare function returns
>    a very large negative or positive value.
> 
>    Author:             [e-mail]
>    Date:               12 november 1997
> */
> 
> 
> var SECTION = "15.4.4.5-3";

Comment 15

[Andreas Gal :gal]

Please move your patch to a new bug. Once dherman reviewed this, you can push it. You can make me appear as author using --author="Andreas Gal <[e-mail]>".

Comment 16

[Dave Herman [:dherman]]

Comment on attachment 427224 [details] [diff] [review]
patch

It occurs to me that it would be good style to wrap everything in the module pattern, so that Narcissus makes minimal changes to the global object. But that can be left for a separate patch.

>             if (!n.name || n.functionForm == STATEMENT_FORM) {
>                 v = new FunctionObject(n, x.scope);
>                 if (n.functionForm == STATEMENT_FORM)

This isn't your fault, but these constant checks should really be using === instead of ==. Harmless, but better style to use the least powerful operation needed.

>-            t.__defineProperty__(s, f, x.type != EVAL_CODE);
>+            defineProperty(t, s, f, x.type != EVAL_CODE);

Similarly, this should really be !== instead of !=.

>-                t.__defineProperty__(s, undefined, x.type != EVAL_CODE,
>-                                     u.readOnly);
>+                defineProperty(t, s, undefined, x.type != EVAL_CODE, u.readOnly);

Likewise.

>             if (n.type == CONST)
>-                s.object.__defineProperty__(t, u, x.type != EVAL_CODE, true);
>+                defineProperty(s.object, t, u, x.type != EVAL_CODE, true);

Here too.

>         if (n.type == NEW) {

And here.

>             // XXX check for a non-arguments object

We should eventually eliminate all the XXX comments. Feel free to destroy or replace with FIXME whenever you're in there.

Looks good. Up to you whether you want to address any of the above nits, which are all from the existing code.

Dave

Comment 17

[Dave Herman [:dherman]]

PS Better still, FIXME's should be accompanied with bug ID's.

Comment 18

[Tom Austin]

Submitted changelist: http://hg.mozilla.org/tracemonkey/rev/0b2cafe8667b

I left out the changes from == to ===, but they seem like a good idea.  I'll open another bug for this issue.

Comment 19

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/0b2cafe8667b