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

Haxe's Generated Neko Source Unable to Utilize Inlined Getter Methods on Dynamic Objects #7368

Closed
Cleod9 opened this issue Aug 28, 2018 · 8 comments
Labels
platform-neko Everything related to Neko

Comments

@Cleod9
Copy link

Cleod9 commented Aug 28, 2018

Minimal Test Case:
https://github.com/Cleod9/WeirdOpenFLNekoBug

Original source thread here:
http://community.openfl.org/t/strange-neko-bug-when-inlining-getters/10888/6

All details from the original thread have been added to the repo's readme, but the gist of it is as follows:

A simple inline expression such as obj1.func(obj2.get_my()) will fail if obj1 is dynamic because Haxe's Neko output expands the expression by assigning get_my into a temporary variable. This results in get_my becoming an anonymous function, which will have lost its context.

My recommendations are to do one of the following:

  1. Have haxe's neko output assign only objects to a tmp and not functions (when possible to infer)
  2. Have haxe's neko output always remain inlined altogether instead of expanding into additional tmp vars
  3. Have haxe's neko output use $call on all functions assigned from Dynamic vars that can be assumed to be functions (e.g. $call(tmp, this.stage,$array(this.stage));)

However if none of the above is possible, I think a build warning would be extremely helpful for others who run into this.

@Simn
Copy link
Member

Simn commented Aug 28, 2018

Unfortunately, this is necessary because neko has an evaluation order that doesn't match Haxe's when it comes to calls. See #4787 (comment)

@Simn Simn closed this as completed Aug 28, 2018
@Cleod9
Copy link
Author

Cleod9 commented Aug 28, 2018

@Simn There is something I don't understand about my issue in relation to that thread.

How could the evaluation order differ between any of Haxe's supported platforms in my code? (JS, HXCPP, etc.). Unlike that other thread I am not writing things like a()(b(), c()), but rather a(b(), c())

I'm ok with avoiding Dynamic in most situations, but because of this problem it becomes impossible to wrap a Dynamic object in a class structure at runtime for Neko (and Neko only). An example case where this might be useful is if you wanted to abstract the rendering process of a game so that you could replace the underlying implementation with a few compile time flags (e.g. toggling from Stage to Stage3D)

If modifying Haxe's output is too risky then I understand. But I strongly feel that this should be surfaced during compilation time as a warning if nothing else. This could be caught when an unknown type gets assigned to a temp, or any Dynamic field containing a ( ) can be disallowed altogether. It seems all too easy to fall into this trap in my opinion, and it is not very easy to debug unless you're already aware of this gotcha

@Simn
Copy link
Member

Simn commented Aug 28, 2018

Maybe I misunderstood your example. Can you reduce it to something that doesn't involve OpenFL?

@Simn Simn reopened this Aug 28, 2018
@Cleod9
Copy link
Author

Cleod9 commented Aug 28, 2018

Sure, I've pushed a new branch that just uses raw Haxe code:

https://github.com/Cleod9/WeirdOpenFLNekoBug/tree/without-openfl

The problem remains the same, where a function call gets assigned to a temp variable when it was unnecessary since there are no evaluation order gotchas.

@markknol markknol added the platform-neko Everything related to Neko label Sep 11, 2018
@Simn
Copy link
Member

Simn commented Sep 25, 2018

I took another look and what I originally said is correct after all. You have this code in ObjOuterWrapper.hx:

inner_wrapper.a(value.get_instance());

With neko's evaluation order being backwards, it evaluates value.getInstance() before it evaluates inner_wrapper.a. Because the former has a side-effect (because it's a call) and the latter could be affected by a side-effect (because it's a Dynamic field access), Haxe has to play it safe and bind the latter to a temp var.

@Simn Simn closed this as completed Sep 25, 2018
@Cleod9
Copy link
Author

Cleod9 commented Sep 25, 2018

Hi @Simn , thanks for taking the time to investigate further. Just to clarify for my piece of mind-

  1. By "side effect" are you saying that value.get_instance() can possibly have an effect on what function inner_wrapper.a is pointing to? If so, is it not possible to create a flag to ignore that assumption and leave the code inlined?

  2. value.get_instance() being evaulated before inner_wrapper.a in Neko sounds like what one would expect in other languages. For example in JavaScript:

var a = function () {
  console.log("inside a");
}
var b = function () {
  console.log("inside b");
}
a(b())

Prints

> inside b
> inside a

The JS version of my example keeps the code inlined while Neko does not. If the JS evaluation order in this case is the same as Neko and Neko is known to be "backwards", wouldn't that mean one of the two outputs by Haxe is inconsistent?

@Simn
Copy link
Member

Simn commented Sep 25, 2018

That is not the correct example, you will need something like a()(b()) to see the difference. I know that in your case the a() part is not a call but a field access, but that has no effect on the order of evaluation.

As for a flag, we don't want any global flags that influence semantics like that. You can easily solve this locally through one of the workarounds you came across. And in general, try avoiding Dynamic as best you can.

@Cleod9
Copy link
Author

Cleod9 commented Sep 25, 2018

Alright got it, I'll leave it at that 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
platform-neko Everything related to Neko
Projects
None yet
Development

No branches or pull requests

3 participants