HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.13k stars 653 forks source link

extra closure created when accessing members of an inlined function result value #2338

Closed nadako closed 10 years ago

nadako commented 10 years ago

Subj. Haxe example:

class A
{
    public var v:Int;

    public function new(v = 0)
    {
        this.v = v;
    }

    public inline function clone()
    {
        return new A(v);
    }
}

class Sample
{
    static function main()
    {
        var v = new A().clone().v;
    }
}

Generated JS:

Sample.main = function() {
    var v = ((function($this) {
        var $r;
        var _this = new A();
        $r = new A(_this.v);
        return $r;
    }(this))).v;
};

AST:

static main(method) : Void -> Void

 = function() = {
    var v = ({
        var _this = new A();
        new A(_this.v);
    }).v;
};
nadako commented 10 years ago

Probably this is the case when inlining should not occur but still does. I got another one here:

class Sample
{
    static function main()
    {
        var o:Dynamic = {f: true};
        if (Reflect.field(o, "f"))
        {
            trace(o.f);
        }
    }
}

js:

Sample.main = function() {
    var o = { f : true};
    if((function($this) {
        var $r;
        var v = null;
        try {
            v = o.f;
        } catch( e ) {
        }
        $r = v;
        return $r;
    }(this))) console.log(o.f);
};
ncannasse commented 10 years ago

We should not mark as inline Reflect.field if it contains a try/catch

nadako commented 10 years ago

Yeah, but shouldn't the inliner be smart here and disallow inlining in this case of usage?

Simn commented 10 years ago

We removed the JS-specific inlining restrictions recently, but I agree Reflect.field should not be inline.

@waneck-mode I have a filter for genc which removes "statements" in some places: https://github.com/waneck/haxe-genc/blob/genc/filters.ml#L111

The generated JS output with that would look like this:

Main.main = function() {
    var o = { f : true};
    var _g8;
    var v = null;
    try {
        v = o.f;
    } catch( e ) {
    }
    _g8 = v;
    if(_g8) o.f;
};

It still has too many locals, but that's a job for a separate filter.

Simn commented 10 years ago

Since the JS inlining restrictions are back in place this issue is "resolved".