Bug 6041 – Support property getters and setters

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Anders Carlsson]

Property getters and setters is a Firefox extension. It's described at http://developer.mozilla.org/en/docs/
Core_JavaScript_1.5_Guide:Working_with_Objects#Defining_Getters_and_Setters

[Anders Carlsson]

Created attachment 5027 [details]
Implement getters and setters

Note that this does not implement the old deprecated way of defining getters
and setters (which unfortunately is used in the jsc test suite)

[Anders Carlsson]

Comment on attachment 5027 [details]
Implement getters and setters

Ah, some tests actually use __defineGetter__ and __defineSetter__ but still
fail, I'll investigate why

[Geoffrey Garen]

+    if (params)
+        func = new FuncExprNode(Identifier::null(), params, body);
+    else
+        func = new FuncExprNode(Identifier::null(), body);

This code seemed a little bizarre to me at first read. Since params is an optional argument, why isn't it 
the last argument?

I've attached a manual patch to show you what I mean. (CVS is down for me right now.) It has the added 
advantage of collapsing the above lines to one:

     func = new FuncExprNode(Identifier::null(), body, params);

[Geoffrey Garen]

Created attachment 5034 [details]
Make params an optional, last argument

[Geoffrey Garen]

+JSValue *GetterSetterImp::toPrimitive(ExecState *, Type) const
+{
+    assert(false);
+    return 0;
+}

+JSObject *GetterSetterImp::toObject(ExecState *exec) const
+{
+    assert(false);
+    return 0;
+}

JavaScriptCore is not tolerant of null pointers, so these return values for deployment builds will almost 
certainly cause a crash if they're ever hit.

I would suggest
     return jsNull().toPrimitive(exec);
and
     return jsNull().toObject(exec);

[Geoffrey Garen]

Same comment here:
+JSValue *PropertyNode::evaluate(ExecState *exec)
+{
+  assert(false);
+  return 0;
+}

[Anders Carlsson]

Created attachment 5055 [details]
Address comments

Here's a new patch that addresses the comments made, and also passes
getset-00[456].js in the mozilla test suite.

I had to do some changes to PropertySlot which makes the patch big, but Maciej
thought it was the right approach (and so do I)

[Darin Adler]

Comment on attachment 5055 [details]
Address comments

In PropertyListNode::evaluate, defineGetter and defineSetter are called on the
value from the assign node without checking that it's a JSObject. That's
dangerous and could crash if the thing is not an object. The code needs a check
if it's an object somewhere rather than just calling JSObject member functions
on a JSValue.

I notice this line in the parser is no longer lined up with the code above it:

+  | '{' PropertyList '}'    { $$ = new ObjectLiteralNode($2); }

I know that Geoff suggested returning jsNull()->toPrimitive(exec, type), but
that's exactly the same as jsNull(), so there's no reason to include the extra
function call. I also think that the logical equivalent of assert(false) is to
call throwError so we get a JavaScript exception in those cases. But since
these values should never get out of properties into expressions, there's very
little reason to worry about exactly what code is there after the assertion,
never to be executed.

Is PropertyNode::streamTo really all that hard to write for getter and setter?

I like to see member functions that are virtual re-declared as virtual, so I'd
like to see that in GetterSetterImp's declaration.

The PropertyListNode::evaluate seems to be a serious one, so marking review-,
otherwise I'd say review+.

[Anders Carlsson]

Created attachment 5066 [details]
Address more comments

(In reply to comment #8)
> (From update of attachment 5055 [details] [edit])
> In PropertyListNode::evaluate, defineGetter and defineSetter are called on
the
> value from the assign node without checking that it's a JSObject. That's
> dangerous and could crash if the thing is not an object. The code needs a
check
> if it's an object somewhere rather than just calling JSObject member
functions
> on a JSValue.
> 
The reason I skipped this check is that we know that the node to be evaluated
always is a FuncExprNode which will always be evaluated to a JSObject. Anyway,
I've added an isObject check for both the getter and setter.

> I notice this line in the parser is no longer lined up with the code above
it:
> 
> +  | '{' PropertyList '}'    { $$ = new ObjectLiteralNode($2); }
> 
> I know that Geoff suggested returning jsNull()->toPrimitive(exec, type), but
> that's exactly the same as jsNull(), so there's no reason to include the
extra
> function call. I also think that the logical equivalent of assert(false) is
to
> call throwError so we get a JavaScript exception in those cases. But since
> these values should never get out of properties into expressions, there's
very
> little reason to worry about exactly what code is there after the assertion,
> never to be executed.
> 
Both of these have been fixed.

> Is PropertyNode::streamTo really all that hard to write for getter and
setter?

It's not hard to write, but the end result will look ugly because there will be
newlines everywhere. I added an implementation for this that will generate
ugly, yet syntactically correct, text.
> 
> I like to see member functions that are virtual re-declared as virtual, so
I'd
> like to see that in GetterSetterImp's declaration.
> 
Fixed.

> The PropertyListNode::evaluate seems to be a serious one, so marking review-,

> otherwise I'd say review+. 
>

[Anders Carlsson]

Comment on attachment 5066 [details]
Address more comments

I noticed that the JSC ChangeLog is messed up; I've fixed that in my local
copy.

[Darin Adler]

Comment on attachment 5066 [details]
Address more comments

As far as PropertyListNode::evaluate is concerned -- if we really do know the
value is an object, then it can be an assert rather than an if.

r=me