321547 - E4X: toString, toSource of a function containing E4X gives incorrect output

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Biju]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051219 Firefox/1.6a1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051219 Firefox/1.6a1

toString, toSource of a function containing E4X syintax ".." gives incorrect output by putting the next nodename in ""

Reproducible: Always

Steps to Reproduce:
do..

function a(){
  var x=<a><b><c>value c</c></b></a>;
  return x..c;
}

alert(a.toString());
Actual Results:  
function a() {
    var x = <a><b><c>value c</c></b></a>;
    return x.."c";
}

Expected Results:  
function a() {
    var x = <a><b><c>value c</c></b></a>;
    return x..c;
}

Comment 1

[Biju]

Attachment: test cases e4x_toSourceErr.html

Comment 2

[Brendan Eich [:brendan]]

The .. operator added by E4X is not like other binary operators, where each operand is evaluated before the operator is applied.  Instead, it implicitly quotes its right operand.

This asymmetry is a shame, because it makes .. unusable for a Perl-ish "range" (a..b for two integer-valued variables a and b) notation.  The bytecode emitted for c on the right of .. in the testcase is JSOP_STRING, which reflects this implicit quoting, so of course SpiderMonkey decompiles the right operand in quotes.

Fixable with a source note; SRC_LABEL will do.

/be

Comment 3

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/e4x/Regress/regress-321547.js,v  <--  regress-321547.js

Comment 4

[Brendan Eich [:brendan]]

mrbkap, you game to take this?  An easy decompiler bug, and we know how much you love the decompiler! ;-)

/be

Comment 5

[Blake Kaplan (:mrbkap) (inactive)]

Sure, I'll take this.

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v1

This seems to do the trick, though it seems mildly wasteful, since SRC_LABEL has arity 1, which is unneeded. Am I missing something?

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 215976 [details] [diff] [review]
patch v1

>     SRC_RESERVED0   = 10,       /* reserved for future use */

Hmm, maybe we should claim this, and call it SRC_UNQUOTE.

>+    JSBool ok, quoteString;

No single-use variables, in general.

>               BEGIN_LITOPX_CASE(JSOP_STRING)
>+                sn = js_GetSrcNote(jp->script, pc);
>+                quoteString = !inXML && (!sn || SN_TYPE(sn) != SRC_LABEL);
>                 rval = QuoteString(&ss->sprinter, ATOM_TO_STRING(atom),
>-                                   (jschar)(inXML ? 0 : '"'));
>+                                   (jschar)(quoteString ? '"' : 0));

In particular, 

                                   (jschar)((inXML ||
                                             (sn && SN_TYPE(sn) == SRC_UNQUOTE))
                                            ? 0 : '"'));

fits in 80 columns, avoids logical negations (confusing and unsightly), and does not seem hard to read to me.

/be

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: patch v2

Much better.

Comment 9

[Brendan Eich [:brendan]]

Comment on attachment 215979 [details] [diff] [review]
patch v2

Yeah!

/be

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 11

[Bob Clary [:bc] (inactive)]

verified fixed 20060401 win/mac/linux

Comment 12

[Bob Clary [:bc] (inactive)]

fixed by Bug 336373 on the 1.8.1 branch. 
verified fixed 1.8.1 with windows/macppc/linux 20060707