hxcpp inconsistency on dynamic property access

[Simn]

class Main {

    static var x(get, null):String;

    static function get_x() return "foo";

    static public function main() {
        var m:Dynamic = Main;
        trace(m.x); // foo
    }
}

This traces foo on the C++ target and null on all other targets I tested. The documentation states that this should not respect the getter: http://haxe.org/ref/properties#important-remarks

@ncannasse: Do you confirm that this is a bug?

[hughsando]

My understanding is: getProperty - always property (get)field - never property dynamic. - target specific This is quite useful - especially with the nme event handler object (event.target) where most of the fields are properties. I thought flash worked this way? In fact, I think neko should be changed to work this way too. I would have thought that not using the getter would only be for code that really knows exactly what it is doing (eg, serializer style code), not your day-to-day usage which should not really even know there is a getter or not. The library code should be able to add a getter, and the users code should still "just work", not have to worry about calling "Reflect.getProperty" every time they have a dynamic object.

On Sat, Apr 5, 2014 at 5:23 AM, Simon Krajewski notifications@github.comwrote:

class Main {

static var x(get, null):String;

static function get_x() return "foo";

static public function main() {
    var m:Dynamic = Main;
    trace(m.x); // foo
}

}

This traces foo on the C++ target and null on all other targets I tested. The documentation states that this should not respect the getter: http://haxe.org/ref/properties#important-remarks

@ncannasse https://github.com/ncannasse: Do you confirm that this is a bug?

Reply to this email directly or view it on GitHubhttps://github.com/HaxeFoundation/haxe/issues/2852 .

[Simn]

I'm not saying that your view doesn't have any merit and it could certainly be discussed, but I have to keep the global picture in mind and there having a target inconsistency like this strikes me as a really bad idea.

People are now writing code which relies on this behavior, and then when it breaks on a different target Joshua tells them how broken that target is with regards to Dynamic access. If you guys want to change the specification, this has to be brought up for discussion (before it is changed).

[ncannasse]

I agree this is not good.

I purposely avoided to use native flash properties to ensure a cross platform behavior. Not all the platforms have native properties support (neko, js), so to have a cross platform behavior with property support on Dynamic would require to compile all Dynamic accesses to get/setProperty, which is not feasible since Dynamic is also used to implement "unsafe" parts of the standard library that needs direct field access.

I can see two possiibilties here:

• change Dynamic definition to specify that properties are handled, then add a new type "Internal" (?) that will behave like the old Dynamic, this would require rewrite a lot of things and complexify the whole code I think.
• enforce that Dynamic only supports native properties but not user-defined ones

In both cases, this is a behavior change, and quite a rough one since it will go unnoticed at compilation. I'm in favor of waiting for a major release to do the change.

If we choose the second way, we could still tag @:nativeProperty some fields that are user-defined but should behave like native (for instance when emulating flash Sprite.x/y), but that would not work for JS (or at least not in all browsers since it would require Object.defineProperty support http://kangax.github.io/es5-compat-table/#Object.defineProperty)

[Simn]

My concern is that the longer we keep this inconsistency, the more users might rely on it, making it harder to change it later.

[hughsando]

It seems odd then to have the difference native flash properties and haxe flash properties - and obviously makes the flash api impossible to mimic. Since neko can be changed to behave any way you wish, we are then only talking about js as the only target - but this can be done now too? https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/get So a better outcome would be to support it on all targets.

On Sat, Apr 5, 2014 at 4:56 PM, Simon Krajewski notifications@github.comwrote:

My concern is that the longer we keep this inconsistency, the more users might rely on it, making it harder to change it later.

Reply to this email directly or view it on GitHubhttps://github.com/HaxeFoundation/haxe/issues/2852#issuecomment-39632741 .

[Simn]

I don't want to support this on all targets because it would mean piping every single dynamic access through Reflect.set/getProperty on some of them.

[hughsando]

Well, on js, as3 and neko it can be done natively (native getter, @:getter, vm 3.0). Not sure how it works on java/cs? But I assume something is happening like it is in cpp where you are already doing "get/set Field" in the this case, and can just as easily be "get/set property" since something must be done here anyhow. Same for the c target I presume. I understand you do not want to (and shouldn't) add runtime overhead for targets that do not support it - but what are we actually talking about here?

On Fri, May 9, 2014 at 7:16 PM, Simon Krajewski notifications@github.comwrote:

I don't want to support this on all targets because it would mean piping every single dynamic access through Reflect.set/getProperty on some of them.

— Reply to this email directly or view it on GitHubhttps://github.com/HaxeFoundation/haxe/issues/2852#issuecomment-42655666 .

[Simn]

I have yet to see a compatible implementation for this on JS. We were discussing it here: https://github.com/HaxeFoundation/haxe/issues/2469 It would break Reflect.field usage, which is a problem we already have on the As3 target.

For neko it would have to be supported first. For PHP I have no idea. For other target it would require a lot of work and potential runtime overhead.

In summary, this is something I can't see us supporting before Haxe 4, even if we wanted to. I really don't like how the Cpp target is doing its own thing here, even more so if you guys promote it as the superior approach and label other targets buggy because they don't support it.

[waneck]

While I agree with you, it's a little tricky to do that without breaking a lot of code that relies on it. It's even more problematic due to the fact that NME/OpenFL use haxe properties for all their getter/setters, while of course is implemented using native properties on Flash - so breaking this would also introduce inconsistencies between Flash and C++.

[Simn]

Yes, I already realized that we can't change it in either direction at this point. I'm just ranting about the approach that was taken here.

[ncannasse]

What about the solution I propose ? We make sure HxCpp does follow Haxe specification, but still allows @:nativeProperty tagging which allows current behavior and will enable Flash/OpenFL compatibility

[ncannasse]

(but only on the properties that are marked as such)

[hughsando]

I'm not really sure how @:nativeProperty solves the problem - I go though and add this to every property I have ever written and two days later, we are back to where we started. Unless of course we implement it on other targets.

The refelct-field vs reflect-property for AS I think can be worked out like this: The "normal" way is to use getProperty, and this the the trivial AS3 function when native getters/setters are used. Everyone who is not a library writer and not doing serializer/cloning should be using getProperty anyhow. (get)field is for "I really know what I'm doing and I've deliberately circumvented the getter", and if this requires a few extra lines on as3 in Reflect.field to deliberately avoid the getter, then I think this is ok.

And maybe I have not handled this the best, but I'm not sure that this haxe standard is gospel. This feature was added to hxcpp in 2009: https://github.com/HaxeFoundation/haxe/commit/b27b35f0d2db4f9c6b1f48d45829180da4de03cb and then 2 yeas ago, the difference between getProperty and (get)field was was made explicit in the reflection case, but there was discussion and it was not changed in the dynamic case: https://github.com/HaxeFoundation/haxe/blob/b08585cf424a202a9bb8ef9f0a583d85f17d468c/gencpp.ml So it has always seemed to me that this feature is target defined, and I am surprised to hear such opposition to this very old feature. And if it is target defined, then I am really trying to encourage you to use native properties where ever they are available - my over-enthusiastic championing of this feature (for which i apologize) is in reaction to what I think is an over-dogmatic adherence to a standard that has not been followed in practice for over 4 years.

On Fri, May 9, 2014 at 9:54 PM, Nicolas Cannasse notifications@github.comwrote:

(but only on the properties that are marked as such)

— Reply to this email directly or view it on GitHubhttps://github.com/HaxeFoundation/haxe/issues/2852#issuecomment-42668217 .

[J-d-H]

While reading this all, I just realized, that I have to go through all my code to check for platform specific missbehaviour ;/

[ncannasse]

@hughsando : at least these will be explicitly tagged as not respecting the Haxe specification, and this will not affect other parts of the end-user code.

We could actually support @:nativeProperty in Flash and JS (would require IE9+, since IE8 restricts it to DOM objects http://kangax.github.io/compat-table/es5/), and for other platforms that supports it, but that should not be mandatory

The fact that it has gone this way for such long time might simply be because we didn't add an unit test for it, it does not imply an agreement that it's a good thing to have such cross platform inconsistencies, especially when then can be avoided.

[hughsando]

There is still the fundamental inconsistency that neko (all dynamic targets?) will set the field and then retrieve it later dynamically, even if it is not part of the class, where cpp will not: var x:Dynamic = new Point(1,2); x.X = 20; // wrong caps trace(x.X); // 20 on neko, but point is still (1,2) Now this is kind of a feature with dynamic targets, which is ok, but is a pretty big gottcha when the programmer is expecting a property, since it appears to work but it does not. So c++ can throw an error and neko can carry on as if everything is ok. This is no more consistent - just a different kind of inconsistent. So I don't think we can make things consistent - we should look at making things as nice for developers as possible. And as nice as possible is, i think:

1. Work as intended if possible (cpp,swf , ie9+)
2. Help identify trouble areas. In a gaming situation, many developers will not use other targets - except neko. And if we could get a patch together to adapt neko, then this is the last they will see of the problem. Now point 2 can't be done at compile time - otherwise we would just fix it there. It also can't be done at runtime on targets that can not tell the difference between a property access and a random variable access, but it can be identified on the targets that can tell the difference. So we could have a command line arg to specify the behaviour: --dynamic-propery-access=allow/warn/throw. The gaming guys can have allow (build into the openfl tool), and the database/web guys can throw and the default can be warn. Note that this is better than the alternatives that ignore the side-effects intended by the property access but otherwise appear to work. Also, in the --dynamic-propery-access=throw case, might it be possible to put undefined "poison" values in place of the getter to generate a runtime error when accidental dynamic property access is used? Or maybe in a debug case add additional runtime checks with --dynamic-propery-access=throw for JS where appropriate.

On Sun, May 11, 2014 at 12:33 AM, Nicolas Cannasse <notifications@github.com

wrote:

@hughsando https://github.com/hughsando : at least these will be explicitly tagged as not respecting the Haxe specification, and this will not affect other parts of the end-user code.

We could actually support @:nativeProperty in Flash and JS (would require IE9+, since IE8 restricts it to DOM objects http://kangax.github.io/compat-table/es5/), and for other platforms that supports it, but that should not be mandatory

The fact that it has gone this way for such long time might simply be because we didn't add an unit test for it, it does not imply an agreement that it's a good thing to have such cross platform inconsistencies, especially when then can be avoided.

— Reply to this email directly or view it on GitHubhttps://github.com/HaxeFoundation/haxe/issues/2852#issuecomment-42746754 .

[ncannasse]

@hughsando we don't want to adapt Neko since JS can't be adapted either without removing WinXP/IE8 support, and since we have zero guarantees that modifying the spec will allow us to run on the next platform we want to support.

Dynamic access IS of course platform dependent. But when we can enforce some rules about things we control we should do it. The whole specification of Haxe properties is based on the fact that their accessors are replaced at compile time, not at runtime. We can add explicit metadata to toggle that, but please respect the language spec default behavior.

We definitely don't want to have a compiler parameter for this, since as soon as one library you want to use needs it, and another not, you're prevented from using the two together.

Trust me on this, @:nativeProperty is the best option we have here, and can then be supported on other targets that have native property support.

[hughsando]

As I said, I can go though and add @:nativeProperty to all my properties, and then not have dynamic properties work on other peoples code - and therefore pass all the tests. Lets say this is done. Now what happens when someone mistakenly uses dynamic access? Neko, they get a hard to diagnose problem - may not even know it exists - it just behaves oddly sometimes. Or, they use my code (with @:nativeProperty) and it happily works for years and then they go to neko and something mysterious happens occasionally. There are both very hard to diagnose bugs, neither of which is helped by the meta data. The only way the meta helps, is if someones has been relying on the behaviour obj.x === Reflect.field(obj,x) for dynamic objects as a way to avoid a getter/setter, but I do not think this is a great language feature, since it will mysteriously change when generics are used. A command-line option would allow dynamic code to be instrumented to find these issues.

So as I said, with or without @:nativeProperty, I think control over the behaviour is still desirable. Get a warning that it will not work on winxp, or carry on because you are only targeting IOS or whatever. I guess nativeProperty also could be added with a macro to emulate the command-line option.

On Sun, May 11, 2014 at 5:17 PM, Nicolas Cannasse notifications@github.comwrote:

@hughsando https://github.com/hughsando we don't want to adapt Neko since JS can't be adapted either without removing WinXP/IE8 support, and since we have zero guarantees that modifying the spec will allow us to run on the next platform we want to support.

Dynamic access IS of course platform dependent. But when we can enforce some rules about things we control we should do it. The whole specification of Haxe properties is based on the fact that their accessors are replaced at compile time, not at runtime. We can add explicit metadata to toggle that, but please respect the language spec default behavior.

We definitely don't want to have a compiler parameter for this, since as soon as one library you want to use needs it, and another not, you're prevented from using the two together.

Trust me on this, @:nativeProperty is the best option we have here, and can then be supported on other targets that have native property support.

— Reply to this email directly or view it on GitHubhttps://github.com/HaxeFoundation/haxe/issues/2852#issuecomment-42765860 .

[ncannasse]

I'm not sure what to tell me here... Yes there are cross platform issues, but we are doing our best to have the most uniform behavior. Having normal properties not getting getter on dynamic access / Reflect.field will make it more uniform. Then the choice to use @:nativeProperty is left to the end user depending if he prefer Flash or Neko compatibility. At least there's a choice with this, and the default for user-defined classes is the the same on all platforms.

[Simn]

@ncannasse: I don't understand your @:nativeProperty idea. The problem at hand comes from accessing fields through Dynamic, so we never see any metadata while generating the field access.

[hughsando]

This will be ok for c++ because it will be implemented in the class definition. I guess for as3 it will also be similar.

On Mon, May 12, 2014 at 3:42 PM, Simon Krajewski notifications@github.comwrote:

@ncannasse https://github.com/ncannasse: I don't understand your @:nativeProperty idea. The problem at hand comes from accessing fields through Dynamic, so we never see any metadata while generating the field access.

— Reply to this email directly or view it on GitHubhttps://github.com/HaxeFoundation/haxe/issues/2852#issuecomment-42803749 .

[frabbit]

i guess it's more about the metadata which becomes nothing more than just "decoration".

[ncannasse]

@Simn the idea is to have C++ follow Haxe spec by not having Dynamic accesses call getter/setter functions. In case one might still want to have such behavior (for instance to keep Flash compatibility), adding @:nativeProperty before the property declaration will allow Dynamic access.

The problem is that HxCpp has a per-class Dynamic accessor which accesses to getter/setter because he knows they are declared as properties. I only want to keep this behavior if @:nativeAccess have been defined.

We could also support @:nativeProperty for SWF/AS3/JS, although for JS that would mean it can only be used with IE9+.

[waneck]

Both Java and C# as they are now can support it as well.

[Simn]

I still don't get it...

var d:Dynamic = somethingThatHasNativeProperties;
d.someField;

The real field is never seen, so the metadata is never found.

[waneck]

It will depend on the target's runtime to be able to achieve this - so some targets won't be able to respect this metadata at all.

[frabbit]

in other words @:nativeProperty should tell the generator that it should generate the required code so that dynamic access on this property works at runtime. I dislike that the user of such properties have to be aware of this in every single case.

[ncannasse]

@Simn that's not a compile time behavior but a runtime one. Look at the way hxcpp handles Dynamic access in generated code

[Simn]

Okay, got it. I guess that means there's nothing to do on my end.

[ncannasse]

@hughsando could you confirm that hxcpp now follow the spec ? @Simn could you add unit test for it and close the issue if it passes ?

[hughsando]

I have not done this yet. I think you should give fair warning that all library maintainers need to add this meta to all their properties, otherwise you will break a lot of code for anyone who uses a git version of haxe. Particularly, you need to get openfl to add this property everywhere.

[Simn]

I think we should:

1. Implement it.
2. Hide it behind a -D 3_2_property_change flag so people can adapt their code.
3. Communicate that properly.
4. Make it the default shortly before 3.2 release.

[ncannasse]

I prefer the "Break early, break often" :)

More seriously : if we hide that behind a flag, we will only get people to adapt this once we release, which is not good. I prefer we update right now the GIT version, so people using GIT will complain to the library maintainer which will then fix it (or fix it himself if he uses GIT).

That's IMHO the best way to ensure most of the libraries are working when we release 3.2

[andyli]

What about:

• Default to the breaking change, and
• Have a flag "-D deprecated_property" which hide the change

such that library authors know that there is a need to update their code, and people can temporarily use the flag to workaround until there is a proper fix to the libraries.