JS array.copy() being compiled to null()

[andyli]

class Test {
    static function main():Void {
        var a = ["a", "b"];
        var b = a.copy();
    }
}

compile with:

-main Test
-js Test.js

output:

(function (console) { "use strict";
var Test = function() { };
Test.main = function() {
    var a_0 = "a";
    var a_1 = "b";
    var b = null();
};
Test.main();
})(typeof console != "undefined" ? console : {log:function(){}});

[Simn]

IMO this should be fixed in Array.copy by using untyped __js__("{0}.slice()", this). Doing (untyped this).slice() is quite bad because it generates an anonymous structure field access. The inliner doesn't cancel here because the field of that access defaults to Var instead of Method.

[ncannasse]

I think that's a huge problem if the analyzer breaks on "untyped this"

[ncannasse]

Ah, reading again I understand a bit better.

[Simn]

It's not the analyzer, it's the constructor inlining logic.

[ncannasse]

Actually I don't understand how it breaks exactly, but I think we should find a way to fix it without changing Array.copy implementation since users might have similar cases in their code

[Simn]

I have clarified this somewhere before but I can do it again: The problem with (untyped this).field is that (untyped this) is typed as a monomorph and then turned into an open structure upon field access.

[insert long train of thought why we may want to consider disallowing field access on unknown types]

The problem is related to this match in inline_constructors in some way: | TField(e1, (FInstance(_, _, {cf_kind = Var _; cf_name = s}) | FAnon({cf_kind = Var _; cf_name = s}))) ->. On sanely typed code this would always cancel because the cf_kind would be Method, but here due to the open structure thing the field access is Var and the inliner thinks that we are just accessing some field on some structure.

[Simn]

... plus the fact that you originally designed inlined constructors to default to null upon unknown field access.

[ncannasse]

But then you can get the same result by doing (perfectly typed):

var _this : { var method(default,never) : Void -> Void; } = this;
_this.method();

[Simn]

Yes, that's probably a problem with the constructor inlining implementation since the get go. So we indeed have to do something about that (though I still think the untyped this code is bad).

[ncannasse]

class Test {
    static function main():Void {
        var a = ["a", "b"];
        (untyped a).slice();        
    }
}

SInce a is used as a value, we should cancel constructor inlining here.

[Simn]

I have fixed the original issue, but the sub-typing one remains for now.

[Simn]

We sidestep the issue now by not inlining constructors if the variable type is different. This is good enough for 3.2, but we can improve the distinction of constructor/structure inlines after that.

[Simn]

I looked at this again and think that my type-based fix does exactly the right thing. If we do new C() but assign it to a variable that's typed differently canceling inlining is the right thing to do. I cannot reproduce any other problem, so I'll consider this closed.