224334 - Implement __defineGetter__ and __defineSetter__ in Rhino

Status: RESOLVED FIXED

(Rhino Graveyard :: Core, enhancement)

Description

[Igor Bukanov]

It would be nice if Rhino would support at least some parts of SpiderMonkey
getter/setter functionality.

Comment 1

[Igor Bukanov]

Attachment: Pass methodName as the last parameter

The patch changes the current glue code to pass the name of the called
interface method as the last parameter to JS function.

This is useful for debugging, for example, even without extending the
conversion to interfaces with multiple methods with the same signature.

Comment 2

[Igor Bukanov]

Comment on attachment 144563 [details] [diff] [review]
Pass methodName as the last parameter

Attachment for the wrong bug...

Comment 3

[Hannes Wallnoefer]

Attachment: implementation for __defineGetter__ and __defineSetter__

This patch implements __defineGetter__ and __defineSetter__ functions in the
Object prototype. Implementation in ScriptableObject is analogous to Java
getter/setter methods, but with a ScriptedGetterSlot class that uses
o.m.j.Function as getter/setter instead of o.m.j.MemberBox.

Defining getters/setters in object initializers isn't implemented by this patch
as I'm a bit scared of messing with the Rhino parsing code.

Comment 4

[Igor Bukanov]

Comment on attachment 198032 [details] [diff] [review]
implementation for __defineGetter__ and __defineSetter__

Thanks for the patch, it is quite resonable besides few hotspots and the need
to replace Function by Callable. In Rhino 1.6R2 object can be called if it
implements just Callable, not Function interface. In this way host objects that
want to allow shortcuts like matrixInstance(i, j) spared from implementing
Function.constructior.

And here are hotspots:

>+++ src/org/mozilla/javascript/NativeObject.java	2005-09-30 21:04:34.417942264 +0200
>@@ -78,6 +78,8 @@
>@@ -182,6 +184,12 @@
>+          case Id_defineGetter:
>+              defineGetter((String) args[0], (Function) args[1], 0);
>+              return Undefined.instance;
>+          case Id_defineSetter:
>+              defineSetter((String) args[0], (Function) args[1], 0);
>+              return Undefined.instance;

It is always necessary to check that args[0] exist and has necessary type to
report proper execption. ScriptRuntime.notAFunction is your friend here.

>@@ -205,6 +213,10 @@
>                 if (c=='h') { X="hasOwnProperty";id=Id_hasOwnProperty; }
>                 else if (c=='t') { X="toLocaleString";id=Id_toLocaleString; }
>                 break L;
>+            case 16: c=s.charAt(8);
>+                if (c=='G') { X="__defineGetter__";id=Id_defineGetter; }
>+                else if (c=='S') { X="__defineSetter__";id=Id_defineSetter; }
>+                break L;
>             case 20: X="propertyIsEnumerable";id=Id_propertyIsEnumerable; break L;
>             }
>             if (X!=null && X!=s && !X.equals(s)) id = 0;

Use toolsrc/org/mozilla/javascript/tools/idswitch/Main.java to generate the
string switch code, see
http://lxr.mozilla.org/mozilla/source/js/rhino/toolsrc/org/mozilla/javascript/t
ools/idswitch/README. (I should definitely mention that in comments to
IdScriptableObject)

>+    public void defineGetter(String propertyName, Function getter, int attributes)
>+    {
...
>+        addSlot(propertyName, propertyName.hashCode(), gslot);

>+    }
>+
>+    public void defineSetter(String propertyName, Function setter, int attributes)
>+    {
....
>+        addSlot(propertyName, propertyName.hashCode(), gslot);
>+    }
>+
Check that addSlot would return gslot and report error otherwise: it can happen
if another thread modifies the object.

Comment 5

[Hannes Wallnoefer]

> Thanks for the patch, it is quite resonable besides few hotspots and the need
> to replace Function by Callable. 

Thanks for the feedback. I knew the patch was sloppy in parts, but wanted to
know if the general direction was ok. I'll upload a revised patch shortly.

Comment 6

[Hannes Wallnoefer]

Attachment: Revised implementation of __defineGetter__ and __defineSetter__

This patch incorporates Igor's suggestions. 

One remaining issue: Spidermonkey throws an error when setting a property that
only has a getter but no setter, while this code silently ignores the setting.
Should we emulate spidermonkey behaviour here?

Comment 7

[Igor Bukanov]

Attachment: Implementation to follow SpiderMonkey as close as possible.

In SpiderMonkey one can call __defineGetter__ and __defineSetter__
arbitrary number of times in arbitrary order. In addition the
functions can be applied to already created properties. Rhino should
follow that and here is a patch to support it.

The patch unifies native and JS getters and setters and use casts to
check if getter/setter is MemeberBox or Callable. With the patch one
can apply native or JS getters/setters in arbitrary order and even mix
them. That required to move optional delegate parameter for native
getter/setters to MemberBox.

As a consequence of the changes the usage of native
getters/setters is more strict. In particular the patch always passes
thisObj as "this" for native methods without delegation where the
current code tries to search the prototype chain. But that usage is
undocumented and was a leftover from times when Rhino used reflection
to implement the runtime library and that search was necessary due to
hacks in other places. With the patch the rules for all
getters/setters are the same.

Another undocumented feature removed by the patch is an option for setter to
declare non-void return type which marks it as one-time initialization
method. The current code uses it to implement lazy initialization of
global properties.

With multiple defineGetter/defineSetter the semantics of the feature
is no longer clear. For example, what should happen if one changes
getter for the property? Or what should happen if another thread
change the setter when initializer runs? Not to answer such questions I
simply removed the feature and instead added explicit support for
LazilyLoadedCtor to ScriptableObject.

Comment 8

[Igor Bukanov]

Attachment: No more slot replaces

The previous version of the patch uses replacement Slot->GetterSlot to get room
to store getter/setter. Although it works for this particular case, it would
prevent for subclasses to implement their own slot search algorithms. The
latter is essential if we want to allow to use __defineGetter__ with array
indexes and properties managed through IdScriptableObject. For example,
currently the following would not define getter for array[0] and Math.sin since
the corresponding values are managed outside of ScriptableObject:

array = [];
array.__defineGetter__(0, function() { return  0; });
Math.__defineGetter__('sin', function() { return  0; });

To support this my current idea is to allow ScriptableObject subclasses to
supply there own Slot implementations which ScriptableObject would use to
properly store getters/setters. But that requires for Slot class to store
getter/setter itself and this is what this version of the patch does. 

To avoid wasting memory for unused getter/setter the patch adds SlotExtra class
which stores attributes together with getter and setter. When getter/setter are
null, the attributes objects are shared, so there is no memory increase
compared with the previous version and the extra object is created only when
one wants getter/setter.

Comment 9

[Jon Stritar]

This patch doesn't include changes to allow Rhino to parse JavaScript files with getter and setter methods, correct? I am working on a project right now, and I'm trying to modify Rhino to not throw syntax errors when someone uses the "get propertyName() { }," syntax in JS files. I just wanted to make sure this doesn't already do it (which it doesn't look like it does).

Comment 10

[Igor Bukanov]

Reassigning to [e-mail] pending resolution of bug 288433

Comment 11

[David Caldwell]

The most current patch causes at least one regression when applied to 1.6R4.  I haven't been able to figure out exactly where or why yet but it causes the following:

var a = [];
a.push("foo");

print(a.length); // prints 1
print(a[0]); // prints undefined
print(typeof(a[0]) == "undefined"); // prints true

Comment 12

[David Caldwell]

The second-to-last patch dated 4-Oct-2005 does not contain the same regressions as the last patch.  The last patch has all the SlotExtra business that is complex enough that I was unable to get to the bottom of the issue.

I can confirm that having run the full test suite against both an unpatched 1.6R4 and a patched version using the 4-Oct-2005 patch that several extra tests pass, including two of the js1_5/GetSet tests and a few miscellaneous tests that rely on the __defineXetter__ methods to be present (notably related to garbage collection).

Thus I would recommend committing the 4-Oct patch on the trunk.

Comment 13

[Igor Bukanov]

(In reply to comment #12)
> Thus I would recommend committing the 4-Oct patch on the trunk.

Well, I am surprised that the patch did not cause any regressions. As far as I can remember I had more trust in the last version. In any case, it seems my desire to spend a weekend or two on hacking  Rhino to close bugs like this would not realize. There are too much other things to do. So feel free to commit whatever that works. 

Comment 14

[David Caldwell]

I tried to change the title of this bug to limit it to __defineGetter__ and __defineSetter__, which are implemented by the patch discussed.  There are other things that are not (changes to the parser to support "get" and "set," __lookupGetter__ and __lookupSetter__) but this is a partial start.  So we could close this issue, commit the patch, and open new issues for the other aspects of this topic.  Or not; that's just my suggestion. :)  Anyway, I have insufficient privileges to change the title.

Comment 15

[David Caldwell]

I have committed a fix based on the second-to-last patch (attachment #198444 [details] [diff] [review]).  (The patch was modified in part to deal with bug #359411.)  I've tested it using my version of the test suite (see bug #353501) and gotten the following results:

Fixed (13): e4x/Regress/regress-354151-01.js, e4x/Regress/regress-354151-02.js, js1_5/Array/regress-315509-02.js, js1_5/GC/regress-311792-01.js, js1_5/GC/regress-311792-02.js, js1_5/GC/regress-312278.js, js1_5/GC/regress-313763.js, js1_5/GC/regress-325269.js, js1_5/GetSet/getset-004.js, js1_5/GetSet/getset-005.js, js1_5/GetSet/regress-366396.js, js1_5/Regress/regress-366288.js, js1_5/Regress/regress-366292.js

Regressions (7): ecma/Expressions/11.10-2.js, ecma/Expressions/11.10-3.js, js1_5/Array/regress-99120-02.js, js1_5/GC/regress-203278-3.js, js1_5/Regress/regress-256501.js, js1_5/String/regress-314890.js, js1_5/String/regress-322772.js

However, all 7 "regressions" went away when increasing the timeout-per-test to 10 seconds from 2 seconds, and results in previous runs varied, so I am satisfied this patch is working.

I would like to change the title of this bug to "implement __defineGetter__ and __defineSetter__" and open a new bug regarding __lookupGetter__ and __lookupSetter__ and perhaps a third bug with the get and set keywords, but I lack the privileges to do that.  So I will leave this bug open, but it's partially fixed. :)

Comment 16

[David Caldwell]

I am closing this bug and have opened bug 368453 for __lookupGetter__ and __lookupSetter and bug 368455 for the "get" and "set" forms of this in object initializers.  If you're interested in this bug you might also be interested in bug 368452 (implement __noSuchMethod__).

Comment 17

[Norris Boyd]

Adding target milestone of 1.6R6 based on the date this bug was resolved FIXED.