Dynamic, side-effects, Javascript

[Simn]

I'm having a bit of a problem with Javascript and side-effects:

class Main {
    static var gl:Dynamic;

    static function main() {
        gl.call({
            terribleSideEffect();
            1;
        });
    }

    static function terribleSideEffect() {
        return 0;
    }
}

The main function is generated as this:

    var tmp = Main.gl.call;
    Main.terribleSideEffect();
    tmp(1);

In the general case this is necessary because terribleSideEffect could change Main.gl.call. With Main.gl being Dynamic the compiler can't make any assumptions regarding the field like it does in case of a Method MethNormal.

The problem is that this can lead to incorrect Javascript behavior because the this-context is lost or something like that. It's explained at http://stackoverflow.com/questions/9677985/uncaught-typeerror-illegal-invocation-in-chrome.

I'm not sure what to do with that right now. It might be necessary to bring back $this closures for this.

[nadako]

@RobDangerous That's actually the problem i got when trying use kha with latest Haxe (as we discussed on IRC). Technically, this is a Haxe regression, but if you didn't use Dynamic here https://github.com/KTXSoftware/Kha/blob/master/Backends/HTML5/kha/SystemImpl.hx#L30 in the first place, everything would be okay :) If I were you, I'd get rid of that Dynamic in favor of proper html/webgl externs present in modern haxe.

[nadako]

And even if we bring $this closures as a fix for this regression, it would still be much better to use properly typed externs instead of Dynamic, if we consider performance of the generated JS code.

[RobDangerous]

Okidoki, will do.

[Simn]

I tried addressing this by simply ignoring calls to Dynamic. However, I realized that this would bring back problems with side-effect order on C++/PHP. This is getting pretty complicated and I'm not sure how to proceed.

[nadako]

can't we detect in genjs that tmp from the example is a Dynamic function to be called and generate a context-binding js code?

[nadako]

it would be even cooler to somehow generate tmp.call(Main.gl, ...), but that requires some sorcery

[nadako]

Oh, that then should be actually var tmp = Main.gl, tmp2 = tmp.call; ...; tmp2.call(tmp, ...);, but doing that in analyzer just for js is pure evil.

[Simn]

I don't think we can address this in genjs, it would affect every Dynamic call.

[nadako]

I wonder if Type.texpr should get some extra field for analyzer-related metadata to be used in generators...

[Simn]

That's what TMeta is for. Also tvar has a meta field, so we could store some information there. However, I don't feel comfortable relying on this to produce valid code. Frankly the best course of action might be to just panic out of the analyzer if there's any Dynamic whatsoever.

... but that would still leave the C++ side-effect problem.

[ncannasse]

What's the specified order of evaluation for foo()(bar()) ? if foo() is evaluated before bar(), then this correctly follows the specification and is okay, regression or not.

[ncannasse]

Ah ok, reading it again, this is actually not correct and yes it should generate something different in that case.

[Simn]

What do you mean it's "not correct"?

[ncannasse]

class Main {

    static function foo() {
        trace("FOO");
        return function(_) trace("OK");
    }

    static function bar() {
        trace("BAR");
        return 0;
    }

    static function main() {
        foo()(bar());
    }

}

This trace BAR/FOO/OK using haxe -x Main.
So it means arguments are evaluated before function ?
In that case that should be generated as

Main.terribleSideEffect();
Main.gl.call(1);

And yes it's not correct to read a function and call it later on any platform that does automatically creates closures such as JS. You need to carry its context with it, for instance.

var gl = Main.gl;
var tmp = gl.call;
tmp.apply(gl,[Main.terribleSideEffect()]);

[Simn]

Wait what... I'm getting FOO/BAR/OK for that on JS. I was under the impression that our evaluation order corresponds to our syntax, which would make this a neko/macro bug.

[ncannasse]

I guess we are missing again some unit tests here. Good luck \o/

[Simn]

Already added, and yes it's only necro failing.

[nadako]

necro

i hardly could breath after loling at that

[ncannasse]

Ah hum... neko is entirely based on pushing the parameters on the stack before the field, I hardly see how we could change that without massively breaking performances. Unless we restrict the notion of "side effect" to something that is not a field read.

[Simn]

This is not strictly about fields, we get the same output when using local functions. I tried looking at the related neko sources but I think that's something I'm not ready for yet.

[ncannasse]

@Simn the problem is quite low level. When calling an object method, we need to have the following on the stack:

Arg1
Arg2
Arg3
Acc = Object
CALLMETHOD "field"

So that would require to push the object on the stack twice: before Arg1 so it gets saved after evaluation, then fetch it back before the callmethod. That's not a change I'm ready to make.

[Simn]

I'll detect it in the side-effect handler. It means having an extra temp var, but I don't think it affects too many cases.

[Simn]

Back to the original issue: I still don't fully understand the scope of this, in multiple senses. A famous target maintainer once said "if the optimizer can generate code that [target omitted] does not like, you can probably create similar code by hand". So what about this for example:

class Main {
    static var gl = {
        value: 12,
        call: function(i:Int) {
            return js.Lib.nativeThis.value;
        }
    }

    static function main() {
        var tmp = Main.gl.call;
        var x = tmp({
            terribleSideEffect();
            1;
        });
        trace(x);
    }

    static function terribleSideEffect() {
        gl.call = function(i) {
            return 0;
        }
    }
}

It fails with "TypeError: Cannot read property 'value' of undefined", which makes sense because there's no this to speak of. Is this just unspecified on our end?

[nadako]

In this concrete example it's quite obvious that you're messing with target-specific behaviour, using js.Lib.nativeThis, so the error makes sense, but with Dynamic as always it's really not clear, as the original problem with kha shows.

Personally though, I'm okay with this behaviour if we document it properly, but this is another limitation of Dynamic which again raises a question where it's a good feature to have at all. Speaking of which, it would be so cool to rework Dynamic to make it behave more like @back2dos's Any abstract some day.

[Simn]

Yes I'm using nativeThis to demonstrate the problem of losing the context. It would be silly if this worked behind Dynamic, but not when properly typed.

[nadako]

Yes, but the difference is that here you explicitly introduce a temp var, so this is to be expected with both Dynamic and proper typing, but in the original issue, it's the analyzer who does that.

[Simn]

Yes my question is if we should address only the cases introduced by the analyzer or all cases.

[ncannasse]

nativeThis entirely escape the Haxe semantics the way Dynamic does so I would not take any example using it into account

[Simn]

Coming back to this issue I still don't know what to do about it. I would like to clarify that this is not about nativeThis, I only used this to demonstrate why it's going wrong. In practice this is more likely to happen with JS externs that rely on this behavior.

I'm pretty sure we'll have to declare something as being unspecified, I'm just not sure how far this should go.

[Simn]

I'll see what happens with the RC and if I have to take any action here for 3.3.0 final.

[ncannasse]

Does the original issue still fails? If not, could you open a new issue and re-state the current problem?

[Simn]

The original issue still fails, but it's only a problem if you use Dynamic for JS-externs which, quite frankly, is going to lead to other problems as well. I cannot come up with an actual problem that doesn't use Dynamic or JS's native this. So let's see what happens.

[Simn]

Nothing to be done for the RC here.

[Simn]

We'll deal with the non-Dynamic case in #5082.