352060 - "getter" keyword still decompiles as "(null)" in this case

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

The "setter" keyword still decompiles as "(null)" if the thing that precedes it is just an identifier:

js> function() { foo setter = function(){} }
function () {
    foo (null)= function () {};
}

The patch for bug 349616 fixed this for the case of "foo.bar setter":

js> function() { foo.bar setter = function(){} }
function () {
    foo.bar setter= function () {};
}

Comment 1

[Robert Sayre]

Attachment: check for getter/setter elsewhere

js> var x = function() { foo setter = function(){} }
js> x
function () {
    foo (null)= function () {};
}
js> dis(x)
flags: LAMBDA
main:
00000:  bindname "foo"
00003:  anonfunobj (function () {})
00006:  setter
00007:  setname "foo"
00010:  pop
00011:  stop

Source notes:
  0:     6 [   6] assignop
js> var x = function() { foo.bar setter = function(){} }
js> x
function () {
    foo.bar setter= function () {};
}
js> dis(x)
flags: LAMBDA
main:
00000:  name "foo"
00003:  anonfunobj (function () {})
00006:  setter
00007:  setprop "bar"
00010:  pop
00011:  stop

Source notes:
  0:     6 [   6] assignop
  1:     7 [   1] pcbase   offset 7

Comment 2

[Robert Sayre]

Comment on attachment 237638 [details] [diff] [review]
check for getter/setter elsewhere

this can happen on JSOP_SETELEM as well. 

js> var x = function(){ var y = new Array(); y[0] getter = function(){}; } 
js> x
function () {
    var y = new Array;
    y[0] (null)= function () {};
}
js> dis(x)
flags: LAMBDA
main:
00000:  name "Array"
00003:  pushobj
00004:  new 0
00007:  setvar 0
00010:  pop
00011:  getvar 0
00014:  zero
00015:  anonfunobj (function () {})
00018:  getter
00019:  setelem
00020:  pop
00021:  stop

Source notes:
  0:     4 [   4] pcbase   offset 4
  2:     7 [   3] decl     offset 0
  4:    18 [  11] xdelta  
  5:    18 [   0] assignop
  6:    19 [   1] pcbase   offset 8

Comment 3

[Robert Sayre]

Attachment: deal with getter/setter in arrays, xml, and plain identifiers

>              case JSOP_SETELEM:
>                rval = POP_STR();
>                op = JSOP_NOP;           /* turn off parens */
>                xval = POP_STR();
>                op = saveop;
>                lval = POP_STR();
>                if (*xval == '\0')
>                    goto do_setlval;
>                sn = js_GetSrcNote(jp->script, pc - 1);
>                todo = Sprint(&ss->sprinter,
>                              (js_CodeSpec[lastop].format & JOF_XMLNAME)
>                              ? "%s.%s %s= %s"
>                              : "%s[%s] %s= %s",

It's not clear to me where we're going to get the first format string here. Also, the introductory comment in jsopcode.tbl glosses over the fact that not all of JOF_ASSIGNING has a precedence of 3. 

>                              lval, xval,
>                              (sn && SN_TYPE(sn) == SRC_ASSIGNOP)
>                                ? (lastop == JSOP_GETTER)
>                                ? js_getter_str
>                                : (lastop == JSOP_SETTER)
>                                ? js_setter_str
>                                : js_CodeSpec[lastop].token
>                              : "",
>                              rval);

Comment 4

[Robert Sayre]

Are we looking for that first format string here?

js> var x = function(){ var foo = <foo bar="baz"/>; foo.@bar getter = function(){}; }
js> x
function () {
    var foo = <foo bar="baz"/>;
    foo[@bar] getter= function () {};
}

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #3)
> >                todo = Sprint(&ss->sprinter,
> >                              (js_CodeSpec[lastop].format & JOF_XMLNAME)
> >                              ? "%s.%s %s= %s"
> >                              : "%s[%s] %s= %s",
> 
> It's not clear to me where we're going to get the first format string here.

D'oh, lastop is wrong -- it's JSOP_GETTER or JSOP_SETTER of course.  We want ss->opcodes[ss->top+1] (xval's op, still on the stack after xval was POP_STR'ed). Can you give that a whirl?

> Also, the introductory comment in jsopcode.tbl glosses over the fact that not
> all of JOF_ASSIGNING has a precedence of 3.

No, the comment is right and I missed three cases:

$ grep JOF_ASSIGNING jsopcode.tbl|grep -v '3,  JOF_'
 *  3           =, +=, etc.             JSOP_SETNAME, etc. (all JOF_ASSIGNING)
OPDEF(JSOP_BINDNAME,  108,"bindname",   NULL,         3,  0,  1,  0,  JOF_CONST|JOF_NAME|JOF_SET|JOF_ASSIGNING)
OPDEF(JSOP_SETCALL,     132, "setcall",     NULL,     3, -1,  2, 17,  JOF_UINT16|JOF_SET|JOF_ASSIGNING)
OPDEF(JSOP_BINDXMLNAME,   170,"bindxmlname",NULL,     1,  1,  2,  0,  JOF_BYTE|JOF_SET|JOF_ASSIGNING)

Could you fix these in a new patch here?  Thanks.

/be

Comment 6

[Robert Sayre]

Attachment: fix format string, opcode table

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 237704 [details] [diff] [review]
fix format string, opcode table

Nice, thanks.

/be

Comment 8

[Robert Sayre]

Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.166; previous revision: 3.165
done
Checking in jsopcode.tbl;
/cvsroot/mozilla/js/src/jsopcode.tbl,v  <--  jsopcode.tbl
new revision: 3.72; previous revision: 3.71
done

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 237704 [details] [diff] [review]
fix format string, opcode table

a=beltzner on behalf of 181drivers

Comment 10

[Robert Sayre]

Checking in jsopcode.c;
/cvsroot/mozilla/js/src/jsopcode.c,v  <--  jsopcode.c
new revision: 3.89.2.45; previous revision: 3.89.2.44
done
Checking in jsopcode.tbl;
/cvsroot/mozilla/js/src/jsopcode.tbl,v  <--  jsopcode.tbl
new revision: 3.43.2.13; previous revision: 3.43.2.12
done

Comment 11

[Bob Clary [:bc] (inactive)]

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

Comment 12

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 20060914 windows/linux 1.9 20060914 windows/mac*/linux