410192 - E4X: incorrect ([e4x.@x]).toSource()

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Biju]

var a = <a x="blah" />;
([a.@x]).toSource()

gives    ==>  [blah]

expected ==> ["blah"]


===== also ===
var a = <a>
            <a x="blah" />
            <a x="blah" />
            <a x="blah" />
        </a>;

([a..@x]).toSource()

gives    ==>
[blah
blah
blah]

expected  ==> ? (I dont know !!!)

may be    ==> ["blah\nblah\nblah"]
or may be ==> ["blahblahblah"]

Comment 1

[Brendan Eich [:brendan]]

Simplest testcase:

js> uneval(<x a="v"/>.@a)
v

Biju, is there anything else like an attribute value that should be quoted by uneval/toSource, but is not quoted by toXMLString? This bug is the result of making toSource an alias for toXMLString. Patch next.

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix, plus a few unrelated cleanups

Comment 3

[Biju]

(In reply to comment #1)
> Biju, is there anything else like an attribute value that should be quoted by
> uneval/toSource, but is not quoted by toXMLString? 

//with
  XML.ignoreComments = false;
  XML.ignoreProcessingInstructions = false;
  XML.ignoreWhitespace = false;

//following also gives invalid toSource
  [<>df</>].toSource();
  [<><![CDATA[df]]></>].toSource();
  [<><!--df--></>].toSource();
  [<><?mypi sds ?></>].toSource();
  [XML('df')].toSource();
  [XML('<![CDATA[df]]>')].toSource();
  [XML('<!--df-->')].toSource();
  [XML('<?mypi sds ?>')].toSource();

//along with
  [<><a>1</a><a>2</a></>].toSource();

Comment 4

[Biju]

// two more test cases

({a:(<x a="a"/>).@b}).toSource();     // XML Query result is null,
({a:XML('')}).toSource();             // blank XML
({a:XML()}).toSource();

Comment 5

[Biju]

also see bug 410263 ...
  [(<a a="&#xA;<>"/>).@a].toSource()  ==> [&#xA;&lt;>]

Comment 6

[Brendan Eich [:brendan]]

Biju: thanks for your help getting E4X bugs tracked down. One suggestion here: please simplify tests of the form |[expr].toSource() ==> bad| to avoid the array initialiser and toSource call: |uneval(expr) ==> bad|.

I'm not going to linger fixing E4X bugs, given higher priorities, so I will commit the patch mrbkap reviewed shortly. Remaining problems go in new bugs. Thanks!

/be 

Comment 7

[Brendan Eich [:brendan]]

Last E4X non-crasher I'll fix for 1.9, I swear!

/be

Comment 8

[Brendan Eich [:brendan]]

Fixed.

/be

Comment 9

[Biju]

Here is it....

With
   XML.ignoreComments = true;
   XML.ignoreProcessingInstructions = true;
   XML.ignoreWhitespace = true;

   |uneval(<>asdf</>)                ==> asdf|
   |uneval(<><![CDATA[asdf]]></>)    ==> asdf|
   |uneval(<><!--asdf--></>)         ==> |
   |uneval(<><?mypi asdf ?></>)      ==> |
   |uneval(<></>)                    ==> |
   |uneval(<x a="asdf"/>.@b)         ==> |
   |uneval(XML('asdf'))              ==> asdf|
   |uneval(XML('<![CDATA[asdf]]>'))  ==> asdf|
   |uneval(XML('<!--asdf-->'))       ==> |
   |uneval(XML('<?mypi asdf ?>'))    ==> |
   |uneval(XML(''))                  ==> |
   |uneval(XML())                    ==> |

also if needed
   |uneval({a:(<x a="asdf"/>).@b})   ==> ({a:})|
   |uneval({a:<></>})                ==> ({a:})|
   |uneval({a:<>asdf</>})            ==> ({a:asdf})|
   |uneval({a:XML('')})              ==> ({a:})|
   |uneval({a:XML()})                ==> ({a:})|


My bad actually following are GOOD, but I dont know what fix has done, 
so I am adding them also
   XML.ignoreComments = false;
   XML.ignoreProcessingInstructions = false;
   XML.ignoreWhitespace = false;

   |uneval(<><!--asdf--></>)         ==> <!--asdf-->|
   |uneval(<><?mypi asdf ?></>)      ==> <?mypi asdf ?>|
   |uneval(XML('<!--asdf-->'))       ==> <!--asdf-->|
   |uneval(XML('<?mypi asdf ?>'))    ==> <?mypi asdf ?>|


(In reply to comment #6)
> please simplify tests of the form |[expr].toSource() ==> bad| to avoid the
> array initialiser and toSource call: |uneval(expr) ==> bad|.

Why uneval is prefered over toSource?

Comment 10

[Brendan Eich [:brendan]]

uneval works for all values, toSource does not for null and undefined (general reason, doesn't apply here of course). Also, uneval is two characters shorter ;-).

Not wrapping in an array initialiser avoids confounding variable to-do with array init and toSource.

Please file followups for uncovered buggy cases. Thanks,

/be

Comment 11

[Biju]

(In reply to comment #10)
> Please file followups for uncovered buggy cases. Thanks,

Sure, thanks...

Comment 12

[Bob Clary [:bc] (inactive)]

/cvsroot/mozilla/js/tests/public-failures.txt
new revision: 1.20; previous revision: 1.19

/cvsroot/mozilla/js/tests/e4x/extensions/regress-410192.js
initial revision: 1.1

Comment 13

[Bob Clary [:bc] (inactive)]

v