Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[flash] native property support rework #8241

Merged
merged 28 commits into from
May 7, 2019
Merged

[flash] native property support rework #8241

merged 28 commits into from
May 7, 2019

Conversation

nadako
Copy link
Member

@nadako nadako commented May 2, 2019

This is a PR that reworks native property support for the Flash target. We need this at @innogames, because the current situation is rather sad:

Currently the extern loader loads native properties as they were normal var fields, which causes a lot of issues when inheritance and interface implementation is involved:

  • to override native accessors you have to add additional @:getter/@:setter methods and it gets worse when you want to also compile to e.g. JS with HTML5, where the properties are actually implemented in Haxe, so you have to write something like:
    #if !flash override #end function set_x(value:Float):Float {...}
    #if flash @:setter(x) function _set_x(value:Float):Void set_x(value);
    This is polluting the code a lot. Yes, this could be done by a build macro (that was also our first solution), but then it takes a lot of time to macro-process the code because we have to make the macro global so there's less possibilities for game programmers to make mistakes.
  • implementing a property from native interface reliably is actually impossible, because haxe loads the property as a normal var as well, so you cannot provide a get/set property in the implementation.

This PR reworks it so the loader loads native properties as actual Haxe get/set properties and also generates fake get_/set_ methods that an user can override/implement. Then the generator takes care about them as well: it rewrites extern get_x()/set_x() method access into property access and it generates native getters and setters for non-extern overrides/implementations. Currently this requires marking the properties with @:flash.property (one can also use this for generating native properties for non-extern classes).

@nadako
Copy link
Member Author

nadako commented May 2, 2019

This is technically a breaking change, but I think it's fine, because with normal field usage nothing should change and inheritance was already kind of broken anyway :-)

@nadako nadako added the platform-flash Everything related to Flash / SWF label May 2, 2019
@back2dos
Copy link
Member

back2dos commented May 2, 2019

Can't speak for the implementation, but design wise it seems neat :)

The breaking part would be the loader creating the externs with different access and @:flash.proptery in it? Perhaps that behavior could be made configurable? OTOH Haxe 3 is a perfectly fine language to create swfs with :D

@nadako
Copy link
Member Author

nadako commented May 3, 2019

one edge-case found in our code base that I still have to deal with:
if we implement a non-extern interface with a property by inheriting an extern class with that property, we have to generate a real get_/set_ method that simply forwards to the extern properties.

e.g.

interface I {
  var x(get,set):Float;
}

class C extends Sprite implements I {
 // haxe thinks that get_x/set_x is present because that's how we load Sprite extern,
 // but these methods are not real, so we have to detect that and generate
 // `function get_x() return super.x` so the method becomes "real"
}

@nadako
Copy link
Member Author

nadako commented May 3, 2019

So, in the current state, no metadata is used/required: we just treat non-physical properties as native. That means two things:

  • every extern non-physical property accessor call will be rewritten to a native property access, i.e.
    extern interface I {
      var x(get,set):Int;
    }
    var i:I = getI();
    i.x = i.x + 1; // haxe transforms this to `i.set_x(i.get_x() + 1)`,
                   // but it's rewritten back to `i.x = i.x + 1` in the generator
  • every non-extern non-physical property will be generated as native property:
    class C {
      var x(get,set):Int;
      function get_x() return 10;
      function set_x(value) return value;
    }
    will be generated as this (AS3)
    class C {
      function get_x():int { return 10; }
      function get x():int { return get_x(); }
      function set_x(value:int):int { return value; }
      function set x(value:int):void { set_x(value); }
    }

I think the only thing that this can break is that the non-physical property will now appear in reflection results, but I hope that's acceptable, given that we already have some #if !as3 guards in tests, because as3 apparently already behaves similarly. A nice side-effect of always generating the native properties we found in our code base is that native libraries that rely on reflection will just work with haxe-defined properties. In our case this would be the GreenSock tweener.

@nadako
Copy link
Member Author

nadako commented May 3, 2019

Is anyone interested in reviewing this? This is kind of important for us so I'd like to have it merged soon.

(also I just realized that I have to adapt genas3 for the new flash externs, so some refactorings will come)

@RealyUniqueName
Copy link
Member

I have no knowledge about abc, but I can review for the generic logic errors.

@RealyUniqueName
Copy link
Member

Also, what will happen with an existing Haxe code which uses @:setter / @:getter ?

@nadako
Copy link
Member Author

nadako commented May 4, 2019

Also, what will happen with an existing Haxe code which uses @:setter / @:getter ?

I think that will work but it won't generate overload flag where needed anymore, I think I can fix it.

UPD: fixed

@back2dos
Copy link
Member

back2dos commented May 5, 2019

I think the only thing that this can break is that the non-physical property will now appear in reflection results, but I hope that's acceptable

There's a distinct chance that this'll mess up haxe serialization in subtle ways (e.g. serializes non-physical field, that when unserialized on another static platform causes runtime crash, or when unserialized on flash calls the setter before the object is ready). Probably not relevant for your use case or in fact anybody anymore ever. Still, it feels like the kind of thing that nobody runs into, except for the one guy who will have to spend half a week figuring out what's wrong. Why did you abandon the metadata after all?

@nadako
Copy link
Member Author

nadako commented May 6, 2019

Why did you abandon the metadata after all?

It was more annoying to deal with it than not :) Mostly because of interface property implementation (we gotta check that if the interface requires flash property, the implementation provides it). But I could add back it, I guess, it doesn't matter too much for us. Will look into it.

@nadako nadako force-pushed the flash_property branch 4 times, most recently from 68a8bce to c936f3f Compare May 6, 2019 14:18
@nadako nadako marked this pull request as ready for review May 7, 2019 20:04
@nadako nadako merged commit 8fec5fc into development May 7, 2019
@nadako nadako deleted the flash_property branch May 7, 2019 20:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform-flash Everything related to Flash / SWF
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants