Need XML.toJSON to return something better than {}. Current plan is to do "XML" (We could do even better than that, but seems better to let the user write their own toJSON.)

Ugh this is harder than I thought. Just adding a toJSON method to the XML.prototype, the same way I did for Dictionary and ByteArray, does not seem to work; stringification continues to return {} rather than "XML". Current hypothesis as to why: XML objects override the behavior of getMultinameProperty (and thus Toplevel::getproperty), and the overriding behavior, as far as I can tell, does *not* ascend the prototype chain, but instead look immediately in the XML object to get E4X semantics to lookup attributes and tagged XML substructure.

(In reply to comment #1) > Current hypothesis as to why: XML objects override the behavior of > getMultinameProperty (and thus Toplevel::getproperty), and the overriding > behavior, as far as I can tell, does *not* ascend the prototype chain, but > instead look immediately in the XML object to get E4X semantics to lookup > attributes and tagged XML substructure. This can't be 100% correct, since direct manipulation of the XML object in AS3 code like x.toJSON("key") and x["toJSON"]("key") does do the prototype lookup. Still investigating.

Correction: x.toJSON("key") does do the prototype lookup. x["toJSON"]("key") signals a type error: Error #100: value is not a function.

Is this a bug in the behavior of our XML objects, or a feature: dynamic class Foo { }; Foo.prototype.bar = function () { return "baz"; } var f = new Foo(); print(' f.bar (): '+ f.bar ()); print('(f.bar)(): '+ (f.bar)()); XML.prototype.bar = function () { return "baz"; } var x = <a><b c="d" e="f">g<h/>i</b><j k="l" m="n">o</j></a> print(' x.bar ("k"): ' + x.bar ("k")); print('about to do (x.bar)("k"): ') print('(x.bar)("k"): ' + (x.bar)("k")); yields the following output in the shell: f.bar (): baz (f.bar)(): baz x.bar ("k"): baz about to do (x.bar)("k"): TypeError: Error #1006: value is not a function. at global$init() It matters because the spec for JSON says that it first looks up the value for the property named "toJSON", and asks it if it is callable. But it seems like we do the lookup differently when the property for an XML object appears directly in a call operator context x.bar(..) as opposed to doing the lookup separately from the call, as in (x.bar)(..)

(Maybe the best short term solution is to special-case the handling of XML in the JSON code. But, *bleah*.)

(In reply to comment #4) Ed suggested I look at Spidermonkey. (Lars also suggested I look at Mozilla's approach, but I think he is referring to Firefox; I'll check that out in a bit.) For reference, here is my experience adapting the example from comment 4 to Spidermonkey's REPL: fklockii-iMac:spidermonkey-objdir fklockii$ ./js js> function Foo () { }; js> Foo.prototype.bar = function () { return "baz"; } (function () {return "baz";}) js> var f = new Foo(); js> print(' f.bar (): '+ f.bar ()); f.bar (): baz js> print('(f.bar)(): '+(f.bar)()); (f.bar)(): baz js> XML.prototype.bar = function () { return "baz"; } (function () {return "baz";}) js> var x = <a><b c="d" e="f">g<h/>i</b><j k="l" m="n">o</j></a> js> print(' x.bar ("k"): ' + x.bar ("k")); typein:8: TypeError: x.bar is not a function js> print('about to do (x.bar)("k") though probably pointless given above.') about to do (x.bar)("k") though probably pointless given above. js> print('(x.bar)("k"): ' + (x.bar)("k")); typein:11: TypeError: x.bar is not a function js> print('and some last second exploration of behavior.') and some last second exploration of behavior. js> x.a js> x.b <b c="d" e="f"> g <h/> i </b> js> x.b.@c "d" js> x.j <j k="l" m="n">o</j>

oh, here's another goodie: checkout the JSON.stringify(x) result below. (Now I don't feel so bad about our goal to return "XML" from stringify.) js> var a = {}; js> a.b = 1 1 js> a.c = 2 2 js> JSON.stringify(a) "{\"b\":1,\"c\":2}" js> JSON.stringify(x) js> x <a> <b c="d" e="f"> g <h/> i </b> <j k="l" m="n">o</j> </a> js>

A few more notes from the Spidermonkey REPL: js> Foo function Foo() {} js> JSON.stringify(new Foo()) "{}" js> Foo.prototype ({bar:(function () {return "baz";})}) js> Foo.prototype.toJSON = function (k) { return "Foo"; } (function (k) {return "Foo";}) js> JSON.stringify(new Foo()) "\"Foo\"" js> JSON.stringify(x) js> JSON.stringify(XML(x)) I conclude that in the context of Spidermonkey, E4X and JSON do not play well together. (I might be missing something, of course.) I'm going to start looking at what Firefox does now. Then I might start asking questions in #js or #jsapi (or #jslang).

From inspecting the json.cpp source code in mozilla-central/js/src (which I believe corresponds to the Spidermonkey JSON implementation), it appears that XML objects are filtered out from handling in a fairly deep way, much like how Functions are filtered out. So I'm not terribly worried about compatibility with Javascript at this point, since it looks like no one has come up with a nice solution for interoperation between E4X and JSON. But the real question is if I can whip something together today.

It would be nice if this got into TR in time for the merge with the player. But if it doesn't, I'll carry the load to try to get it merged separately (assuming that it does end up passing muster in the review process). Added Jeff for feedback because he might have an opinion about how to deal with the clear conflict between E4X getprop semantics and the ES5 toJSON semantics. (Please don't just say "do what Mozilla does" without first reading the comments on Bug 652200 that survey the landscape between Spidermonkey and Firefox.)

Comment on attachment 528445 [details] [diff] [review] adds special case for XML.prototype.toJSON Review of attachment 528445 [details] [diff] [review]: ::: core/JSONClass.cpp @@ +1280,5 @@ + // control-path could follow the same flow in both the + // XML- and non-XML-cases. + if (AvmCore::isObject(value) && + (obj = AvmCore::atomToScriptObject(value), + // toJSON invocations even if the client has assigned using the comma operator in this way makes my teeth hurt, and will probably generate warnings on some compilers. IMHO a clearer and less warning-prone construct would be if (AvmCore::isObject(value) && (obj = AvmCore::atomToScriptObject(value)) != NULL && isXMLInstance(obj))) even better might be to use the recent changes in https://bugzilla.mozilla.org/show_bug.cgi?id=638956 and rewrite as if (toplevel->builtinClasses()->get_XMLClass()->isType(value) || toplevel->builtinClasses()->get_XMLListClass()->isType(value))

This is meant to supplement attachment 528445 [details] [diff] [review]. It is an Actionscript program that shows the way that one can leverage changing toJSON in XML and XMLList to get improved (but application-specific) rendering of the relevant structure in an XML object. (This code was very seat-of-my-pants; I've never tried decomposing XML structures before now. The code is merely an illustration; it is probably incomplete and won't do quite-the-right-thing on some inputs. That's why I'm pawning it off as an application-specific toJSON and not one we'd try to provide officially.) After applying attachment 528445 [details] [diff] [review], running the attached Actionscript yields the following output from avmshell: <ems> <em id="0"> <n>Jim</n> <a>25</a> </em> <em id="1"> <n>Joe</n> <a>20</a> </em> </ems> Run stringify with current non-existent XML.prototype.toJSON {} Run stringify with planned future XML.prototype.toJSON "XML" Run stringify with user-written ``semi-smart'' XML.prototype.toJSON {"ems":[{"@id":"0","em":[{"n":["Jim"]},{"a":["25"]}]},{"@id":"1","em":[{"n":["Joe"]},{"a":["20"]}]}]}

incorporated Steven's suggestion. Using isType(Atom) seems to work. (I think I've acquired some over-anxious over-optimization behavior)

(In reply to comment #8) > I conclude that in the context of Spidermonkey, E4X and JSON do not play well > together. (I might be missing something, of course.) When I posted that comment, I omitted the portion of the transcript that illustrated how Spidermonkey essentially ignored XML.prototype.toJSON. Here's a relevant repl interaction (including an illustration of how overriding the prototype.toJSON works on types other than XML): js> var x = <hello>World</hello> js> JSON.stringify(x) js> JSON.stringify(x) == undefined true js> XML.prototype.toJSON = function (k) { return "XML"; } (function (k) {return "XML";}) js> JSON.stringify(x) js> function Foo() { } js> Foo.prototype.toJSON = function (k) { return "Foo"; } (function (k) {return "Foo";}) js> var y = new Foo(); js> JSON.stringify(y); "\"Foo\"" js> To be honest, there might be deeper problems here with Spidermonkey. Consider this: js> var x = <hello>World</hello> js> x.foo() typein:2: TypeError: x.foo is not a function js> XML.prototype.foo = function () { return "bar"; } (function () {return "bar";}) js> x.foo() typein:4: TypeError: x.foo is not a function And compare against what avmshell does: avmplus interactive shell Type '?' for help > var x = <hello>World</hello> World > x.foo() TypeError: Error #1006: value is not a function. at global$init()[:1] > XML.prototype.foo = function () { return "bar"; } function Function() {} > x.foo() bar So, here be Tygers...

changeset: 6221:6eabde14897f user: Felix S Klock II <fklockii@adobe.com> summary: Bug 652200: add special-case for XML*.prototype.toJSON (r=stejohns). http://hg.mozilla.org/tamarin-redux/rev/6eabde14897f

changeset: 6223:c36107842e7b user: Felix S Klock II <fklockii@adobe.com> summary: Bug 652200: add trivial toJSON() implementation to XML and XMLList (r=fklockii). http://hg.mozilla.org/tamarin-redux/rev/c36107842e7b

How about this: 1/Define toJSON in a special namespace (AS3 might work). 2/From the JSON code that uses getprop, use the equivalent of xml.AS3::toJSON. The namespace AS3 causes the the XML instance properties to be bypassed in the lookup. 3/Define AS3::toJSON in the XML prototype so that it delegates to the user assignable XML.prototype.toJSON. 4/Do the same for the other types that support user overriding of toJSON.

(In reply to comment #19) > How about this: > > 1/Define toJSON in a special namespace (AS3 might work). > > 2/From the JSON code that uses getprop, use the equivalent of xml.AS3::toJSON. > The namespace AS3 causes the the XML instance properties to be bypassed in the > lookup. > > 3/Define AS3::toJSON in the XML prototype so that it delegates to the user > assignable XML.prototype.toJSON. > > 4/Do the same for the other types that support user overriding of toJSON. I like this idea, I'll spend some time hacking and see if I can use this idea to come up with something cleaner than rev 6223.

Here's a comment that evolved as I did development. With Jeff's solution, there's no need to leave it in the source code, but I think the case analysis may be useful for others inspecting the code, so I'm transcribing it here: // Bug 652200: We can't use getprop to lookup the // (potentially overridden) toJSON property on XML // objects, because it has special behavior on XML // objects. We have four options: // 1. Don't support toJSON on XML/XMLList, // 2. Use callprop directly on .toJSON for the XML/XMLList, or // 3. Use getprop on delegate (prototype) for the XML/XMLList. // 4. Add another level of indirection // // Option 1 will mean that we're stuck with whatever // behavior we get from JSON's default handling of XML // objects: returning the rendered JSON object {}. // (The only way around producing {} is to put // special-case code in JSON for XML, and if we're // going to do that, we might as well go with options // 2 or 3 above.) // // Option 2 will mean that we will attempt to perform // toJSON invocations even if the client has assigned // a non-function to the toJSON property. This is // is probably better than option 1. // // Option 3 will mean that we won't attempt a lookup // on an individual XML object xmlobj, but rather go // straight to its prototype. But a property lookup // on an individual object xmlobj cannot yield a // function without delegating to its prototype // anyway, so we would never miss a toJSON the user // was intending us to call. // // Option 3 is probably better than option 2 in that // it is a bit more faithful to what the JSON // specification dictates (in that it will not attempt // to invoke toJSON properties that resolve to // non-functions), and it is a less invasive change to // the JSON code. // // Option 4 means we don't always jump straight to // doing a lookup for toJSON. Instead we start by // looking up a property named AS3::toJSON, and only // fall back on toJSON if the first lookup fails. // // Option 4 is probably the best option of all, since // it decouples XML and JSON. JSON will still have // a special case code, but only for classes that // define a AS3::toJSON method (which cannot be // dynamically added anyway) // // "All problems in computer science can be solved by // another level of indirection" Thanks to Jeff Dyer // (and Butler Lampson)

This adds the approach Jeff Dyer outlined in Comment 20. (It also fixes a small bug I introduced in changeset - 6223:c36107842e7b where I left out the key parameter in the toJSON methods I added.)

Re your step 4, since AS3::toJSON is a fixed method it will always be present on an instance where it might be present (based on a known type). I take it that there will not be such a method on Object, then.

Comment on attachment 528658 [details] [diff] [review] adds special case for AS3::toJSON to JSON and usage in XML Review of attachment 528658 [details] [diff] [review]: okey dokey

changeset: 6236:bd426b64ca73 user: Felix S Klock II <fklockii@adobe.com> summary: Bug 652200: special case AS3::toJSON in JSON and use it in XML (r=stejohns). http://hg.mozilla.org/tamarin-redux/rev/bd426b64ca73

(In reply to comment #19) > How about this: > > ... > > 4/Do the same for the other types that support user overriding of toJSON. (In reply to comment #23) > Re your step 4, since AS3::toJSON is a fixed method it will always be present > on an instance where it might be present (based on a known type). I take it > that there will not be such a method on Object, then. Oh yeah, I meant to comment on that step 4 when I talked with Jeff. I see the addition of AS3::toJSON as a work-around for E4X, not something that every type needs to fundamentally provide. So I was not planning to putting AS3::toJSON into the other classes, unless there are similar semantic issues in other classes that motivate it. (Arguably I should do a survey of how getprop is overridden to try to find such cases ahead of time to avoid similar surprises in the future.)