349616 - "getter" keyword decompiles as "(null)"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

> function() { window.foo getter = function() { return 5; }; alert(window.foo); };

function () { window.foo (null)= (function () {return 5;}); alert(window.foo); }

Comment 1

[Robert Sayre]

Attachment: skip JSOP_GETTER/JSOP_SETTER when checking for a token

feels hacky, but fixes the test case

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 235657 [details] [diff] [review]
skip JSOP_GETTER/JSOP_SETTER when checking for a token

Right fix is to actually populate the JSOP_GETTER and JSOP_SETTER cases in the big switch that handles all the non-terminal ops.

The test for decompiling terminal ops, if (cs->token) should stay that simple or get simpler.  My current plan to consolidate non-terminal code is to create a little language to drive the decompilation of non-terminals based on stack arity, input and output precedence, quoting flags, etc. This could be specified in cs->token too with a leading % or @, say.

In any event, the terminal case, which is generic, should stay generic and not test special op cases.

/be

Comment 3

[Brendan Eich [:brendan]]

"Terminal" vs "non-terminal" is not right, I should have written "regular" or "simple" instead of "terminal".

/be

Comment 4

[Brendan Eich [:brendan]]

jsopcode.tbl's old comment, which I wrote over 11 years ago, calls the token field of JSCodeSpec by its original name, image, and talks about null-initialized images for "ugly" bytecodes.  The regular or "pretty" ones are opcodes generated by most of the expression grammar, but a few operators in that grammar are "ugly".

Whatever we call things, the goal with the decompiler should be toward fewer cases and (I hope) a synthesis of the random logic in the big switch for the else clause of the if (cx->token) condition.

Thanks for diving into the decompiler again, it's the red-headed step-child among the interpreter and compiler (the other two bytecode-consuming and/or -producing modules).

/be

Comment 5

[Robert Sayre]

Attachment: put the test in JSOP_SETPROP

this is what Brendan said to do in IRC

Comment 6

[Brendan Eich [:brendan]]

Comment on attachment 235924 [details] [diff] [review]
put the test in JSOP_SETPROP

>Index: jsopcode.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsopcode.c,v
>retrieving revision 3.148
>diff -u -8 -p -r3.148 jsopcode.c
>--- jsopcode.c	27 Aug 2006 21:08:41 -0000	3.148
>+++ jsopcode.c	29 Aug 2006 16:42:38 -0000
>@@ -2287,18 +2287,20 @@ Decompile(SprintStack *ss, jsbytecode *p
>                  * Force precedence below the numeric literal opcodes, so that
>                  * 42..foo or 10000..toString(16), e.g., decompile with parens
>                  * around the left-hand side of dot.
>                  */
>                 op = JSOP_GETPROP;
>                 lval = POP_STR();
>                 sn = js_GetSrcNote(jp->script, pc - 1);
>                 todo = Sprint(&ss->sprinter, fmt, lval, xval,
>-                              (sn && SN_TYPE(sn) == SRC_ASSIGNOP)
>-                              ? js_CodeSpec[lastop].token
>+                              (sn && SN_TYPE(sn) == SRC_ASSIGNOP) ?
>+                              (lastop == JSOP_GETTER) ? js_getter_str :
>+                              (lastop == JSOP_SETTER) ? js_setter_str :
>+                              js_CodeSpec[lastop].token
>                               : "",

Cool -- style nit is to underhang ? and : as the existing code did.  JS house style puts && and || at the end, but not ?: when any part of the ternary takes more than one line (and if any does, then each ? and : part gets its own line or lines).

r=me with that picked, and a new patch to nominate for the branch.  Thanks,

/be

Comment 7

[Robert Sayre]

Attachment: test in JSOP_SETPROP, js house style (I think...)

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 237240 [details] [diff] [review]
test in JSOP_SETPROP, js house style (I think...)

>Index: jsopcode.c
>===================================================================
>RCS file: /cvsroot/mozilla/js/src/jsopcode.c,v
>retrieving revision 3.159
>diff -u -8 -p -r3.159 jsopcode.c
>--- jsopcode.c	7 Sep 2006 19:47:40 -0000	3.159
>+++ jsopcode.c	8 Sep 2006 00:29:36 -0000
>@@ -2364,17 +2364,21 @@ Decompile(SprintStack *ss, jsbytecode *p
>                  * 42..foo or 10000..toString(16), e.g., decompile with parens
>                  * around the left-hand side of dot.
>                  */
>                 op = JSOP_GETPROP;
>                 lval = POP_STR();
>                 sn = js_GetSrcNote(jp->script, pc - 1);
>                 todo = Sprint(&ss->sprinter, fmt, lval, xval,
>                               (sn && SN_TYPE(sn) == SRC_ASSIGNOP)
>-                              ? js_CodeSpec[lastop].token
>+                              ? (lastop == JSOP_GETTER)
>+                              ? js_getter_str
>+                              : (lastop == JSOP_SETTER)

Ok, don't hate me cuz of this, but you could get extra style points for indenting and parenthesizing the inner ?: expression.  House style chains with opening ( stacked over ? and : only for one ?: or -- and this is the key, if the final : term is itself a ?:.  Analogy is if-else-if chain that does not indent with each else-if.  What we have here is more if-if-else-else-if-else, which you wouldn't indent so that then and else parts were at only one level.

>+                              ? js_setter_str
>+                              : js_CodeSpec[lastop].token
>                               : "",
>                               rval);
>                 break;
> 
>               case JSOP_GETELEM2:
>                 op = JSOP_GETELEM;
>                 (void) PopOff(ss, lastop);
>                 /* FALL THROUGH */

Comment 9

[Robert Sayre]

Attachment: some indenting

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 237263 [details] [diff] [review]
some indenting

That's it!  Thanks.

Trivial patch, no-risk.  We know by static analysis and lots of other (some recent, some old) tests that lastop is valid. This just avoids printf'ing "(null)", preferring the correct tokens instead.

/be

Comment 11

[Mike Schroepfer]

Comment on attachment 237263 [details] [diff] [review]
some indenting

a=schrep for drivers.

Comment 12

[Robert Sayre]

(In reply to comment #11)
> (From update of attachment 237263 [details] [diff] [review] [edit])
> a=schrep for drivers.
> 

Brendan, you have to land this. I don't have the privs.

Comment 13

[Brendan Eich [:brendan]]

You are a member of the despot JavaScript partition now, so land at will :-).

/be

Comment 14

[Jesse Ruderman]

Robert checked in this fix on trunk a few hours ago.

I filed bug 352060 on a case where this still isn't fixed.

Comment 15

[Bob Clary [:bc] (inactive)]

Checking in regress-349616.js;
/cvsroot/mozilla/js/tests/js1_5/Regress/regress-349616.js,v  <--  regress-349616.js
initial revision: 1.1
done

do we care that getter = is decompiled as getter= ?

Comment 16

[Brendan Eich [:brendan]]

(In reply to comment #15)
> Checking in regress-349616.js;
> /cvsroot/mozilla/js/tests/js1_5/Regress/regress-349616.js,v  <-- 
> regress-349616.js
> initial revision: 1.1
> done
> 
> do we care that getter = is decompiled as getter= ?

No.  I prefer the latter.  None of this is being standardized via ECMA, so we can suit ourselves.

/be

Comment 17

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 1.9 2006091022 windows/mac*/linux