apache / royale-compiler

Apache Royale Compiler
https://royale.apache.org/
Apache License 2.0
95 stars 49 forks source link

XML.toXMLString() not recognized by the compiler #203

Open Harbs opened 2 years ago

Harbs commented 2 years ago

The following application:

<?xml version="1.0" encoding="utf-8"?>
<js:Application xmlns:fx="http://ns.adobe.com/mxml/2009"
                xmlns:js="library://ns.apache.org/royale/basic"
                applicationComplete="onComplete()">
    <fx:Script>
        <![CDATA[
            private function onComplete():void{
                var xml:XML = <foo name="baz"/>;
                console.log(xml.toXMLString());
            }
        ]]>
    </fx:Script>
    <js:valuesImpl>
        <js:SimpleCSSValuesImpl />
    </js:valuesImpl>
    <js:initialView>
        <js:View>
            <js:Label text="ID" id="id1"/>
        </js:View>
    </js:initialView>
</js:Application>

compiled using:

{
    "config": "royale",
    "compilerOptions": {
        "debug": true,
        "targets": ["JSRoyale"],
        "source-map": true,
        "remove-circulars": true
    },
    "additionalOptions": "-js-output-optimization=skipAsCoercions -js-dynamic-access-unknown-members=true -allow-abstract-classes=true -js-vector-emulation-class=Array -js-vector-index-checks=false -js-complex-implicit-coercions=false -export-public-symbols=false -prevent-rename-protected-symbols=false -prevent-rename-public-static-methods=false -prevent-rename-public-instance-methods=false -prevent-rename-public-static-variables=false -prevent-rename-public-instance-variables=false -prevent-rename-public-instance-accessors=false",
    "files":
    [
        "src/test_project.mxml"
    ]
}

Will get an error: TypeError: a.toXMLString is not a function when run in release mode.

There's a (probably) related issue that toXMLString is usually wrapped in Language.string call when used when expecting a string.

Harbs commented 2 years ago

Probably related as well:

There's (usually?) a double require for XML when used:

goog.require('XML');
goog.require('XML');

and: /* Royale Dependency List: org.apache.royale.core.SimpleCSSValuesImpl,org.apache.royale.core.View,org.apache.royale.html.Label,org.apache.royale.events.Event,XML,XML,org.apache.royale.html.beads.GroupView,org.apache.royale.html.beads.layouts.BasicLayout*/

joshtynjala commented 2 years ago

With -js-dynamic-access-unknown-members=true, toXMLString() is being incorrectly generated as dynamic access, for some reason.

console.log(xml["toXMLString"]());

As a temporary workaround, you can remove either -js-dynamic-access-unknown-members=true or -prevent-rename-public-instance-methods=false. I'll try to figure out why toXMLString() isn't being recognized as a known member.

joshtynjala commented 2 years ago

So I discovered that members of the XML and XMLList classes always fail to resolve in the compiler. Even if a field is absolutely defined on the class, the definition still resolves to null. This is due to the dynamicness of these classes, where setting properties changes an internal representation of the XML data, instead of storing fields on the object. With that in mind, I decided that if the class where the member is being accessed resolves to XML or XMLList, the compiler can safely ignore -js-dynamic-access-unknown-members=true.

Harbs commented 2 years ago

Much improved, but I found another case where it's not being resolved:

if(objXMLList.length() == 1){// no contents. Keep the background as a top level object
    xml.Spread.setChildren(objXMLList);
} else {
    if(xml.Spread.Group.length() == 0){// make sure the Group actually exists (i.e. started off with objects)
        xml.Spread.appendChild(<Group/>);
    }
    xml.Spread.Group.setChildren(objXMLList);
}

is being compiled to:

  if (objXMLList.length() == 1) {
    xml.child('Spread')["setChildren"](objXMLList);
  } else {
    if (xml.child('Spread').child('Group')["length"]() == 0) {
      xml.child('Spread')["appendChild"](new XML( '<Group/>'));
    }
    xml.child('Spread').child('Group')["setChildren"](objXMLList);
  }

xml.child() should be determined to be XML as well.

Harbs commented 2 years ago

The issue is like you stated that none of the XML methods resolves. That causes the following as well:

var xml:XML = <foo name="baz"/>;
var logStr:String = xml.toXMLString();
console.log(logStr);

compiles to:

var /** @type {XML} */ xml = new XML( '<foo name="baz"/>');
var /** @type {string} */ logStr = org.apache.royale.utils.Language.string(xml.toXMLString());
console.log(logStr);

So while that will work, there's lots of extra calls to Language.string. It would be better if we can avoid that.

Harbs commented 2 years ago

What defines XML as "dynamic"?

FWIW, xml[i] (or xmlList[i]) can always be assumed to be XML.

joshtynjala commented 2 years ago

What defines XML as "dynamic"?

When I think of something being dynamic in AS3, I think of accessing arbitrary properties that are not known by the compiler. With XML and XMLList in AS3, you you can do stuff like this: xml.someRandomName.anythingYouWant. The names someRandomName and anythingYouWant are not known by the compiler. You are allowed to use any name you want, a lot like the Object or * types.

So while that will work, there's lots of extra calls to Language.string. It would be better if we can avoid that.

Have you tried using the -strict-xml=true compiler option? The default behavior matches the classic Flex SDK compiler (there's a comment in the Royale compiler that says that Adobe did this on purpose), but I think that Alex added this new option to allow someone to opt into stricter XML typing. The description notes that you might get extra warnings, though. I haven't personally tried it, except as a quick experiment to see if toXMLString() would resolve (and it did). This option may be worth trying to see if you can get the behavior that you personally are looking for.

joshtynjala commented 2 years ago

xml.Spread.setChildren(objXMLList); xml.child('Spread')["setChildren"](objXMLList);

My latest commit should fix this issue with nested XML member access.

Harbs commented 2 years ago

Have you tried using the -strict-xml=true compiler option?

Bingo.

It fixes both issues, but creates other ones:

var childName:String = XMLChild.name();

Causes an error: Error: Implicit coercion of a value of type QName to an unrelated type String. Elsewhere I get a warning: Warning: Comparison between a value of type QName and an unrelated type String.

Good info!

joshtynjala commented 2 years ago

It sounds like you'll need to do some manual string conversions that would normally happen automatically at runtime, in order to make the compiler happy using the stricter API. I suspect that toString() will work in most cases.

var childName:String = XMLChild.name().toString();
Harbs commented 2 years ago

Yup. Basically there's a tradeoff here. It's good to know the details. I'll have to write this up in the docs before I forget... πŸ˜‰

Harbs commented 2 years ago

It looks like attributes need to be handled separately:

The following:

if(shape.rawXML.@MyAttribute.length()){

}

compiles to:

if(this.shape.rawXML.attribute('MyAttribute')["length"](){

}
Harbs commented 2 years ago

Another one:

var childrenList:XMLList = range.children();
var childLen:int = childrenList.length();
for(j=0;j<childLen;j++){
    var name:QName = childrenList[j].name();
}

compiles to:

var /** @type {XMLList} */ childrenList = range.children();
var /** @type {number} */ childLen = childrenList.length();
for (j = 0; j < childLen; j++) {
    var /** @type {QName} */ name = childrenList[j]["name"]();
}

(name should not be in quotes)

Harbs commented 2 years ago

For the record: var childName:String = XMLChild.name().toString(); is a bad idea because if XMLChild is a text node, name() is null. A better solution is "" + XMLChild.name() or String(XMLChild.name()).

joshtynjala commented 2 years ago

It looks like attributes need to be handled separately:

I'm not able to reproduce this one. Here's my code:

public var shape:XML;

//later, in a method
if(shape.rawXML.@MyAttribute.length()){
}

I get this JS:

if (this.shape.child('rawXML').attribute('MyAttribute').length()) {
}

Are you sure that shape is correctly typed as XML or XMLList in your code?

Another one: childrenList[j].name();

I can reproduce this one. The solution will be very similar. I'll commit the fix today.

Harbs commented 2 years ago

In my case rawXML was typed as XML. shape was a class. I'll check if your change fixes it later.

joshtynjala commented 2 years ago

Oh, okay! I think that I need to make a small tweak to fix that. Thanks!

Harbs commented 2 years ago

πŸ’£ πŸ’₯ πŸ₯‡

Perfect! This makes finding the legitimate cases of missing types sooo much easier. 🀟 You da best! 😎

Harbs commented 2 years ago

This was a vast improvement. I did find some more cases of unnecessary quoting:

  1. toString and indexOf:
    if(!pg.runtimeQualifiers.(text().toString().indexOf("image_name") == 0).length()){
    name = "";
    }

compiles to:

  if (!pg.runtimeQualifiers.filter(function(/** @type {XML} */ node){return (node.text()["toString"]()["indexOf"]("image_name") == 0)}).length()) {
    name = "";
  }

pg is a class. runtimeQualifiers is an XMLList.

  1. Another one, that's not XML related at all (hasOwnProperty):
    if(value[i].hasOwnProperty("selected")){
    item.selected = value[i]["selected"];
    }

compiles to:

if (value[i]["hasOwnProperty"]("selected")) {
    item.selected = !!(value[i]["selected"]);
}

I don't think hasOwnProperty ever needs to be quoted. Here value is an array of objects, but value in this case is actually types as an Object instead of an Array. I don't know if that makes a difference.

joshtynjala commented 2 years ago

toString and indexOf:

I'll look into these.

I don't think hasOwnProperty ever needs to be quoted. Here value is an array of objects, but value in this case is actually types as an Object instead of an Array. I don't know if that makes a difference.

Usually, when [] square bracket syntax is used, the returned value is considered to be of type * by the AS3 compiler. This applies to using square brackets with both arrays and objects. I think that Vector is the one exception when the type should be known at compile time when using square brackets. So technically, the compiler is behaving as expected here.

Assigning to a variable of type Object should be enough to let the compiler know that hasOwnProperty() and other Object methods are available:

var currentValue:Object = value[i];
if(currentValue.hasOwnProperty("selected")){

I'm wary to upgrade * to Object automatically when resolving dynamic-access-unknown-members. It should be safe most of the time, but there may be edge cases that I haven't remembered yet.

Harbs commented 2 years ago

OK. Definitely better safe than sorry! πŸ˜‰