apache / royale-compiler

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

[JS] super setter expressions do not resolve to the value being set #210

Closed greg-dove closed 2 years ago

greg-dove commented 2 years ago

an expression similar to :

override public function set mySetter(value:Boolean):void{
   this.something = super.mySetter = value;
}

(example is only for illustrative purposes) does not currently work correctly with the current javascript output for super setter calls.

for another example see: https://github.com/apache/royale-asjs/blob/1219425faa090aacf05fe940e88f6c68d93d206d/frameworks/projects/MXRoyale/src/main/royale/mx/controls/CheckBox.as#L255

joshtynjala commented 2 years ago

I suspect this applies to any getter/setter property, and not only ones accessed using super.

This seems to be how the JS code is generated:

this.com_example_MyClass_something = com.example.MyClass.superClass_.set__mySetter.apply(this, [ value] );

It fails because the setter doesn't return anything. So I think that undefined would be assigned to something, instead of the value.

The generated JS should probably be something like this instead:

com.example.MyClass.superClass_.set__mySetter.apply(this, [ value] )
this.com_example_MyClass_something = com.example.MyClass.superClass_.get__mySetter.apply(this);

However, I think that will be tricky to generate because I'm pretty sure that the emitter writes multiple assignments without looking ahead, and this.com_example_MyClass_something will already be be emitted before we even detect that there's a setter involved.

It might not look pretty, but I think that we can use the comma operator to make this work. Probably safest with parentheses around the full expression.

this.com_example_MyClass_something = (com.example.MyClass.superClass_.set__mySetter.apply(this, [ value]), com.example.MyClass.superClass_.get__mySetter.apply(this));
joshtynjala commented 2 years ago

I suspect this applies to any getter/setter property, and not only ones accessed using super.

Actually, it looks like I was wrong in that assumption. It appears that it fails with super specifically because it needs to call the setter directly. With this instead, it can use normal member access, so nothing special is needed there.

This:

this.something = super.mySetter = this.myOtherSetter = value;

Becomes this:

this.com_example_MyClass_something = com.example.MyClass.superClass_.set__mySetter.apply(this, [ this.myOtherSetter = value]);

That should make it easier to fix, actually, since we should already have a special case for super setters, and all tweaks should be able to stay in that one place.

joshtynjala commented 2 years ago

Since some conversation related to this issue is happening on the mailing list, here's a link to the thread so that anyone reading this on Github knows where to find it:

https://lists.apache.org/thread/p4o8kno69cg5nhwp1ptbopg0t83c5thq