517580 - Make JS_HAS_GETTER_SETTER always 1, remove support for old-style getters and setters

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

ES5 is codifying getters and setters in the standard; it doesn't quite seem worth the overhead of maintaining the #ifdef.  (I haven't even thought of how I might do that for Object.defineProperty, and it's not immediately obvious how the algorithm might be modified to respect it, although admittedly this is only first-glance thinking.)  One exception: require JS_HAS_GETTER_SETTER in order to include the __(lookup|define)[GS]etter__ methods in Object.prototype; people who may have wanted to avoid non-standard methods shouldn't be provided them, while people who had it enabled before won't want them to go away without having any choice in the matter.

Comment 1

[Brendan Eich [:brendan]]

Please do -- we keep trimming jsversion.h (jsconfig.h formerly) -- you should have seen it in the old days (cvs.mozilla.org).

/be

Comment 2

[Jim Blandy :jimb]

Strip 'em out!

Comment 3

[Jason Orendorff [:jorendorff]]

(In reply to comment #0)
> One exception: require JS_HAS_GETTER_SETTER in order
> to include the __(lookup|define)[GS]etter__ methods in Object.prototype; people
> who may have wanted to avoid non-standard methods shouldn't be provided them,
> while people who had it enabled before won't want them to go away without
> having any choice in the matter.

It can also control the deprecated getter= syntax (better yet, delete that too).

Comment 4

[Jeff Walden [:Waldo]]

(In reply to comment #3)
> It can also control the deprecated getter= syntax (better yet, delete that
> too).

Good call -- also time to axe the { x getter: ... } syntax as the better { get x() { ... } } syntax is now sanctioned by ECMA.  (This one's trickier due to complexity in obj_toSource that must necessarily be cleaned up to address use of Object.defineProperty with named function expressions.)

Comment 5

[Jeff Walden [:Waldo]]

Hm, axing x getter: requires supporting |{ get <PropertyName>() { } }|, where PropertyName includes strings and numbers (any non-negative fraction, that is).  Filed bug 520696 to add that.

Comment 6

[Jeff Walden [:Waldo]]

Attachment: Getting there getting there

This has whatever I'd finished last time I looked at this bug, plus what I finished now, which I *think* may just be everything but changes necessary for uneval and toSource.

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Gettinger there gettinger there

After much staring I finally disentangled old-style getters and setters from object decompilation.  I think.  The code is very complicated, and there's still a residual needOldStyleGetterSetter that, I think, guards now-dead code.  More investigation to do, certainly.  Also, there's a gazillion tests that use the old-school syntax that need to be updated.  Somehow.  They can't just use ES5 because they're segregated by JS version (yet another reason I'm not a fan of organizing tests by language version), and __defineGetter__ and friends are more of the same old badness.  Anyway, I'll think about that after I get this milestone uploaded for backup purposes.

Comment 8

[Jeff Walden [:Waldo]]

Attachment: Full patch

This patch:

* turns on JS_HAS_GETTER_SETTER unconditionally
* getter/setter support, in syntax and programmatically, is always enabled
* changes the meaning of JS_HAS_GETTER_SETTER to enabling/disabling support
  for Object.prototype.__{define,lookup}{G,S}etter__
* removes old-style getter/setter syntax, which comes in at least these flavors
  (going from memory, might have missed/misremembered one or two):

varname getter= ...; varname setter= ...;
obj.prop getter= ...; obj.prop setter= ...;
var o = { propname getter: ..., propname setter: ... };
var o = { get propname getterName() { }, set propname setterName(v) { } };
getter function prop() { } // getter on global object
setter function prop(v) { } // setter on global object

Lots of code removal, lots of code simplification, excellent opportunity for code improvements.

Comment 9

[Brendan Eich [:brendan]]

> var o = { get propname getterName() { }, set propname setterName(v) { } };

is not old-style, it was the "new" form favored by Waldemar for JS2/ES4 "classic" back circa y2k. And of course it's in ES5 now.

Suggest not using JS_HAS_GETTER_SETTER for a different purpose -- better to use a new name, although even better to avoid #ifdefs that won't be used for a while, so are likely to rot. This goes for other jsversion.h macros, too (separate bugs).

/be

Comment 10

[Jeff Walden [:Waldo]]

(In reply to comment #9)
> > var o = { get propname getterName() { }, set propname setterName(v) { } };
> 
> is not old-style, it was the "new" form favored by Waldemar for JS2/ES4
> "classic" back circa y2k. And of course it's in ES5 now.

You misread the example.

var o = { get propname() { }, set propname(v) { } };

is in ES5, but

var o = { get propname getterName() { }, set propname setterName(v) { } };

is not.  In the latter, the function used as the getter or setter is not anonymous: it has a name associated with it.  This does not appear to have been intentional.  Namedness here is only a side effect of how the syntax used to be parsed: after eating up get/set and then the property name, a function expression context was faked up by pretending the last token seen was TOK_FUNCTION.  Thus if a name was seen next, it'd be treated exactly as if we were at this point in the following source text:

function foo(v) { }
         ^
         |

If a name occurred, it'd be the name of the function, but if it didn't (the far more common case), it'd just be a function expression without a name:

function (v) { }
         ^
         |

(Note that for bug 554616 we're going to have to parse getters and setters in object literals in a more principled manner anyway, which will be a great opportunity to decompose function parsing in general into something like function/name?/arguments/body -- a non-trivial task due to name-analysis code, but one which will greatly help readability.)

The JS test suite includes many tests for ES5 getter/setter syntax.  This removal breaks none of them because it's not ES5 syntax.


> Suggest not using JS_HAS_GETTER_SETTER for a different purpose

Not a new purpose (the ifdefs protected this before), just a narrower one.

> better to use a new name, although even better to avoid #ifdefs that won't
> be used for a while, so are likely to rot.

The #ifdefs are rare and not at all pervasive: one around the methods and property strings, one in jsobj.h for where those property strings are extern'd, one around the property entries in a JSPropertySpec array, and one in jsapi.cpp in JS_ResolveStandardClasses.  None of these locations are in complex code paths subject to change.  Some #ifdefs bite hard, but ones which affect little more than the methods found on standard objects don't.

As far as a different name goes, I wouldn't be bothered to change the name personally, but it doesn't really matter.  Maybe OLD_GETTER_SETTER_METHODS?  The name has some similarity to the name for the #ifdef that guarded forcing old-school getter/setter style (OLD_GETTER_SETTER) with a slightly more precise name.

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> (In reply to comment #9)
> > > var o = { get propname getterName() { }, set propname setterName(v) { } };
> > 
> > is not old-style, it was the "new" form favored by Waldemar for JS2/ES4
> > "classic" back circa y2k. And of course it's in ES5 now.
> 
> You misread the example.
> 
> var o = { get propname() { }, set propname(v) { } };
> 
> is in ES5, but
> 
> var o = { get propname getterName() { }, set propname setterName(v) { } };
> 
> is not.

Oops, I did misread -- thanks. This is the infamous intrinsic function name != property name "feature". It came up on es*-discuss, I talked about it here:

https://mail.mozilla.org/pipermail/es-discuss/2009-March/008959.html

It was (I'm almost positive) not intended.

> > Suggest not using JS_HAS_GETTER_SETTER for a different purpose
> 
> Not a new purpose (the ifdefs protected this before), just a narrower one.

"different" != "new"

> > better to use a new name, although even better to avoid #ifdefs that won't
> > be used for a while, so are likely to rot.
> 
> The #ifdefs are rare and not at all pervasive....

The number of ifdefs was not the issue I raised. Rotting if we turn off the old __{define,lookup}{G,S}etter__ API is the issue, but we won't turn them off soon. Ahead of that, the ifdefs' controlling macro is always defined, and we test these old APIs.

So the only point of the ifdefs is to delineate the non-standard stuff. It's not just a feel-good measure but it adds a little source complexity with the potential for rot on the "turned off" side that we do not test.

In general my experience with ifdef'ing stuff in SpiderMonkey, especially to draw lines around things, has been negative on balance. It clutters the source and makes the testing matrix explode, but of course we only test what we ship, so the rot grows everywhere the tests do not cover.

> As far as a different name goes, I wouldn't be bothered to change the name
> personally, but it doesn't really matter.  Maybe OLD_GETTER_SETTER_METHODS? 
> The name has some similarity to the name for the #ifdef that guarded forcing
> old-school getter/setter style (OLD_GETTER_SETTER) with a slightly more precise
> name.

Yeah, something like OLD_GETTER_SETTER_FOO would be better for delineating the trash you want to take out (or is it trash? ES5's default attribute values make the ES5-standard alternative verbose, you pointed out on IRC the other day). I suspect __{define,lookup}{G,S}etter__ will be with us for a looong time.

/be

Comment 12

[Jeff Walden [:Waldo]]

(In reply to comment #11)
> "different" != "new"

Oops, I should have said different.  I still think there's a distinction between narrowed scope and completely altered scope.

> (or is it trash? ES5's default attribute values make the ES5-standard
> alternative verbose, you pointed out on IRC the other day)

Verbose, certainly.  I think the ship sailed when ES5 introduced the one true way to do all this.  People who dislike it enough can write all the helper methods they want; it shouldn't be our duty to provide them in superficial cases like this.

> I suspect __{define,lookup}{G,S}etter__ will be with us for a looong time.

Just call me William James when it comes to removing implementation baggage.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> > (or is it trash? ES5's default attribute values make the ES5-standard
> > alternative verbose, you pointed out on IRC the other day)
> 
> Verbose, certainly.  I think the ship sailed when ES5 introduced the one true
> way to do all this.

No, ES5 is new and not the sole arbiter of truth. This is upside-down, and intensely non-William-James-pragmatic.

> People who dislike it enough can write all the helper
> methods they want; it shouldn't be our duty to provide them in superficial
> cases like this.

You are again reversing cause and effect.

/be

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Tweaked for comments

Comment 15

[Luke Wagner [:luke]]

Mmm, getters and sharps.  Early question, concerning this bit in obj_toSource:

            /*
             * It's not possible to serialize objects containing cyclic getters
             * or setters using ECMA syntax; censor those parts of properties
             * which can't be serialized.
             */
            if (vchars[0] == '#' && gsop[j])
                continue;

I can't see how the result of js_ValueToSource of a getter/setter is a sharped value: the getter/setter value is either undefined or a function and js_ValueToSource for a function just decompiles the function without any sharp logic.

Now, before the patch, sharping of getter/setter methods happened a few lines later, but you've added a !gsop[j] conjunct, so it won't happen there either.  Thus, it seems, rather than 'censoring' cyclic getters/setters, they are necessarily clone.  E.g., with the patch:

js> function f() { return 3 }
js> o = {}
js> Object.defineProperty(o, 'a', { enumerable:true, get:f })
({get a () {return 3;}})
js> Object.defineProperty(o, 'b', { enumerable:true, get:f }) 
({get a () {return 3;}, get b () {return 3;}})

Is this the desired behavior?

Comment 16

[Luke Wagner [:luke]]

Oh wait, regarding my first question: user-defined Function.prototype.toSource seems to do it.  So the idea is that they might be returning a sharped string and we want to say "well, that used to work, but not anymore" ?

Comment 17

[Jeff Walden [:Waldo]]

I dropped the idea of censoring in favor of duplication at some point, yes -- I think the problem was we were streaming out the returned source and had already dumped out the property name, or something like that, and it was easier to duplicate than to do anything else.

The worry for that particular |continue| wasn't user-defined sourcification (except to the extent of not having memory/safety errors that's SEP); I'll look back and work out what the reason was.

Comment 18

[Luke Wagner [:luke]]

>             /*
>              * Remove '(function ' from the beginning of valstr and ')' from the
>              * end so that we can put "get" in front of the function definition.
>              */
>-            if (gsop[j] && VALUE_IS_FUNCTION(cx, val[j]) &&
>-                !needOldStyleGetterSetter) {
>+            if (gsop[j] && VALUE_IS_FUNCTION(cx, val[j])) {

Independent of the user-defined toSource, it seems here that gsop[j] implies VALUE_IS_FUNCTION(cx, val[j]).  Is that right?

>-#if JS_HAS_GETTER_SETTER
>-    if (tt == TOK_NAME) {
>-        tt = CheckGetterOrSetter(context, &tokenStream, TOK_FUNCTION);
>-        if (tt == TOK_ERROR)
>-            return NULL;
>-    }
>-#endif

Wow, I had no idea how many places 'getter' and 'setter' could show up!  I was hoping that taking all these tests and calls would show a speedup on Chris's parsemark, but it said 'Meh'.

Comment 19

[Jeff Walden [:Waldo]]

I think the |continue| is a vestigial remnant from the very first adjustment I made in that code, back when you had this line in the code:

  if (!JSVAL_IS_PRIMITIVE(val[j]) && vchars[0] != '#') {

rather than this one as in the patch:

  if (!gsop[j] && !JSVAL_IS_PRIMITIVE(val[j]) && vchars[0] != '#') {

I think those two lines can be safely removed now; nice catch.

gsop[j] doesn't imply function because the test for getter/setter-correctness when not undefined is js_IsCallable.

js> Object.defineProperty({}, "foo", { get: /a/, enumerable: true })
({get foo /a/})

Embedders could also define other classes/objects which would be callable.  We should be slightly careful here if we can help it, because all of that defensive character-pointer-twiddling is very fragile, as comments note, in the presence of user-defined toSource properties.

Comment 20

[Jeff Walden [:Waldo]]

I should note that { get foo /a/ } is not especially desirable, but it's not entirely undesirable: it's almost an analog to function foo() { [native code] }.

Comment 21

[Luke Wagner [:luke]]

Comment on attachment 437945 [details] [diff] [review]
Tweaked for comments

Well, that's all my questions, nice work.

Comment 22

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/e47d2506e0ad

I'll write up a blog post on this in the next few days or so, just in case anyone was actually using any of this, and it needs to be in the what's-new docs as well.

Comment 23

[Andreas Gal :gal]

Awesome cleanup Waldo. Cool.

Comment 24

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/e47d2506e0ad

Comment 25

[Eric Shepherd [:sheppy]]

For reference when documenting: http://whereswalden.com/2010/04/16/more-spidermonkey-changes-ancient-esoteric-very-rarely-used-syntax-for-creating-getters-and-setters-is-being-removed/

Comment 26

[Eric Shepherd [:sheppy]]

This is now mentioned in the docs; not a lot of detail, but a link is included to the blog post for people's reference if they really need to know specifics.