[cpp] Unable to modify Array<T> from inlined method

[MSGhero]

In the setup below, using an inlined method to add to an Array<T> doesn't do anything unless <T> is not an Int/Bool/Float/String (the ones I've tried). It works fine when the generic is removed and the array is given any type or when the push method is not inlined. It also works fine if I manually index into the array or if I manually reset the array.

class Test {

    public static function main() {

        var s0 = new Stack<Int>();
        s0.push(0);
        s0.push(1);
        Lib.trace(s0.arr); // expected: [0,1], actual: []
        // manual indexing
        s0.arr[0] = 7;
        s0.push(8);
        Lib.trace(s.arr); // expected: [7,1,8], actual: [7]

        // same results for String, Bool, and Float

        var s1 = new Stack<String>();
        // manually reset array
        s1.arr = []; // or s1.clear()
        s1.push("0");
        s1.push("1");
        Lib.trace(s1.arr); // expected: ["0","1"], actual: ["0","1"]
        s1.arr[0] = "7";
        s1.push("8");
        Lib.trace(s1.arr); // expected: ["7","1","8"], actual: ["7","1","8"]

        var s2 = new Stack<Dynamic>();
        // works as expected
        s2.push({});
        s2.push({});
        Lib.trace(s2.arr); // expected: [{},{}], actual: [{},{}]
        s2.arr[0] = {};
        Lib.trace(s2.arr); // expected: [{},{}], actual: [{},{}]
    }
}

class Stack<T> {

    public var arr:Array<T>;
    public var len(default, null):Int;

    public function new() {
        clear();
    }

    public inline function clear():Void {
        // I've tried this inlined and not, doesn't change things
        arr = [];
        len = 0;
    }

    public inline function push(t:T):Void{
        // inlining this causes issue
        arr[len++] = t;
    }
}

I'm testing this with openfl->windows. Works as expected on flash.

Latest master version of all libs, haxe 3.2.0-rc2.

[Simn]

class Main {
    public static function main() {
        var s0 = new Stack<Int>();
        s0.push(0);
        trace(s0.arr); // expected: [0,1], actual: []
    }
}

class Stack<T> {

    public var arr:Array<T>;
    public var len(default, null):Int;

    public function new() {
        clear();
    }

    public inline function clear():Void {
        // I've tried this inlined and not, doesn't change things
        arr = [];
        len = 0;
    }

    public inline function push(t:T):Void{
        // inlining this causes issue
        arr[len++] = t;
    }
}

Dump:

class Main{
    static public main(method) : Void -> Void

     = function() = {
        var tmp = new Stack();
        var s0 = tmp;
        {
            var tmp1 = s0.arr;
            var tmp2 = s0.len ++;
            tmp1[tmp2] = 0;
        };
        var tmp1 = s0.arr;
        var tmp2 = {fileName : "Main.hx",lineNumber : 5,className : "Main",methodName : "main"};
        haxe.Log.trace(tmp1,tmp2);
    }

}

Cpp:

Void Main_obj::main( ){
{
        HX_STACK_FRAME("Main","main",0xed0e206e,"Main.main","Main.hx",2,0x087e5c05)
        HX_STACK_LINE(3)
        ::Stack tmp = ::Stack_obj::__new();     HX_STACK_VAR(tmp,"tmp");
        HX_STACK_LINE(3)
        ::Stack s0 = tmp;       HX_STACK_VAR(s0,"s0");
        HX_STACK_LINE(4)
        {
            HX_STACK_LINE(4)
            Array< int > tmp1 = s0->arr;        HX_STACK_VAR(tmp1,"tmp1");
            HX_STACK_LINE(4)
            int tmp2 = (s0->len)++;     HX_STACK_VAR(tmp2,"tmp2");
            HX_STACK_LINE(4)
            tmp1[tmp2] = (int)0;
        }
        HX_STACK_LINE(5)
        Array< int > tmp1 = s0->arr;        HX_STACK_VAR(tmp1,"tmp1");
        HX_STACK_LINE(5)
        Dynamic tmp2 = hx::SourceInfo(HX_HCSTRING("Main.hx","\x05","\x5c","\x7e","\x08"),5,HX_HCSTRING("Main","\x59","\x64","\x2f","\x33"),HX_HCSTRING("main","\x39","\x38","\x56","\x48"));        HX_STACK_VAR(tmp2,"tmp2");
        HX_STACK_LINE(5)
        ::haxe::Log_obj::trace(tmp1,tmp2);
    }
return null();
}

@hughsando: Your problem or my problem?

[hughsando]

Have not looked at this too closely, but my guess is that this: Array< int

tmp1 = s0->arr;
is creating and then modifying a copy, since s0->arr is implemented as
array, not Array
So I guess it depends on the type of s0->arr (is there any hint that it is
a type-param?) and this would point the fickle finger of blame.

On Thu, Apr 30, 2015 at 3:21 PM, Simon Krajewski notifications@github.com
wrote:

class Main {
public static function main() {
var s0 = new Stack();
s0.push(0);
trace(s0.arr); // expected: [0,1], actual: []
}
}

class Stack {

public var arr:Array<T>;
public var len(default, null):Int;

public function new() {
    clear();
}

public inline function clear():Void {
    // I've tried this inlined and not, doesn't change things
    arr = [];
    len = 0;
}

public inline function push(t:T):Void{
    // inlining this causes issue
    arr[len++] = t;
}

}

Dump:

class Main{
static public main(method) : Void -> Void

 = function() = {
    var tmp = new Stack();
    var s0 = tmp;
    {
        var tmp1 = s0.arr;
        var tmp2 = s0.len ++;
        tmp1[tmp2] = 0;
    };
    var tmp1 = s0.arr;
    var tmp2 = {fileName : "Main.hx",lineNumber : 5,className : "Main",methodName : "main"};
    haxe.Log.trace(tmp1,tmp2);
}

}

Cpp:

Void Main_obj::main( ){
{
HX_STACK_FRAME("Main","main",0xed0e206e,"Main.main","Main.hx",2,0x087e5c05)
HX_STACK_LINE(3)
::Stack tmp = ::Stack_obj::__new(); HX_STACK_VAR(tmp,"tmp");
HX_STACK_LINE(3)
::Stack s0 = tmp; HX_STACK_VAR(s0,"s0");
HX_STACK_LINE(4)
{
HX_STACK_LINE(4)
Array< int > tmp1 = s0->arr; HX_STACK_VAR(tmp1,"tmp1");
HX_STACK_LINE(4)
int tmp2 = (s0->len)++; HX_STACK_VAR(tmp2,"tmp2");
HX_STACK_LINE(4)
tmp1[tmp2] = (int)0;
}
HX_STACK_LINE(5)
Array< int > tmp1 = s0->arr; HX_STACK_VAR(tmp1,"tmp1");
HX_STACK_LINE(5)
Dynamic tmp2 = hx::SourceInfo(HX_HCSTRING("Main.hx","\x05","\x5c","\x7e","\x08"),5,HX_HCSTRING("Main","\x59","\x64","\x2f","\x33"),HX_HCSTRING("main","\x39","\x38","\x56","\x48")); HX_STACK_VAR(tmp2,"tmp2");
HX_STACK_LINE(5)
::haxe::Log_obj::trace(tmp1,tmp2);
}
return null();
}

@hughsando https://github.com/hughsando: Your problem or my problem?

—
Reply to this email directly or view it on GitHub
#4187 (comment)
.

[Simn]

The type of s0->arr should match TInst({cl_path = [],"Array"}, [{ cl_kind = KTypeParameter _}]) if that's what you're asking.

[hughsando]

Yes, I think it is - in which case, it looks like it is my problem, and
hxcpp should be typing it as ArrayBase. Possibly one too many "follows".

On Thu, Apr 30, 2015 at 3:55 PM, Simon Krajewski notifications@github.com
wrote:

The type of s0->arr should match TInst({cl_path = [],"Array"}, [{ cl_kind
= KTypeParameter _}]) if that's what you're asking.

—
Reply to this email directly or view it on GitHub
#4187 (comment)
.

[Simn]

Can we fix this today? I'm not sure I can make a release tomorrow if this is indeed a regression.

[hughsando]

I have been looking at it, but I think the problem is coming from the typing. Looking at the -D dump, it gets [Var tmp1(155):Array<Int>] but I think it should be
[Var tmp1(155):Array<Stack.T>] or maybe tag the "Int" with KTypeParameter - but this is not possible since "Int" is an abstract, and there is no a_kind field.

Still looking though...

[Simn]

Actually Array<Int> looks correct here because we know the concrete type. Why do you need the KTypeParameter flag? The fact that Int is used as a type parameter should come from the context, i.e. it appearing in the type parameter list of TInst.

[hughsando]

The Array<Stack.T> is not actually stored as an Array<Int> - it is an ArrayBase since the one representation must be used for all types. Eg, on a 64 bit machine, if you have Stack<Int> and Stack<Main> sharing the same implementation, and sizeof(int) is not the same as sizeof(MAIN *), so what "byte stride" can be used when accessing the elements?
Then, when the tmp = is used, it must do a copy, or else maybe throw some exception (this issue is discussed elsewhere).

So the real problem might be : arr = []; which is from the "new". This is a hidden version of the "no new in generic type" rule, since the "[]" should be strongly typed (ie, an actual Array<Int>). If you pass in a real array form code that is strongly typed, rather than calling "clear", it should work.

[hughsando]

In fact, if you call the inline clear function from outside the constructor, it works.

[hughsando]

I think it comes down to arrays being special, and the only way to solve it would be to have an extra level of indirection (virtual functions), which would slow things down.

class Generic<T>
{
   public var t:T;
   public var arr:Array<T>;
   public var list:List<T>;
   public function new()
   {
      //t = new T(); // 'Only generic type parameters can be constructed'
      list = new List<T>(); // Ok
      arr = new Array<T>(); // Not really Ok on cpp
   }
}

class Test
{
    public function new() {}
    public static function main()
    {
       var t = new Generic<Int>();
       t.arr.push(0);
       trace(t.arr);

       var tmp = t.arr;
       tmp.push(1);
       trace(t.arr);
    }
}

I think the regression is coming from the tmp variables - I have a feeling the same bug can be shown if explicit tmp variables are used in the old code.

[Simn]

I suspected as much. I think we can a hack-fix for this in the simplifier for 3.2. I'm having some trouble formalizing the problem though, could you add the relevant case(s) here: https://github.com/HaxeFoundation/haxe/blob/development/analyzer.ml#L218

[hughsando]

I think it is based on etype, rather than the eexpr. An etype of Array<x> when x has KTypeParameter.
This is just to fix the regression, not the whole problem right?

[Simn]

Yes

Check a bit below where we check for Void. If I'm understanding it correctly we could just add Array there and maybe restrict it to the cpp target.

[hughsando]

I tried | TInst ({ cl_path = [],"Array" }, [ TInst({ cl_kind = KTypeParameter _ }, _ ) ]) -> true but I think that the "follow" may need to be handled better so it does not replace KTypeParameter too early.

Just using "Array" on cpp would solve the problem - not sure if it would introduce other problems that the original code solved. But maybe this is a good solution on short notice.

[hughsando]

Ok, I have made the change - does not seem like it will cause too many issues. You can close this issue if you are happy with this solution.

[hughsando]

(There was not point checking for KTypeParameter, since I think it has been replaced already)

[Simn]

Thanks!

But well, this is really just a hack. I don't entirely understand why this behavior occurs only when transforming the AST like this. That is, why does it work if I manually store s0.arr in a variable?

[hughsando]

Still breaks - see my "Generic" test above - gives "[0]" (correct), but then with the explicit tmp, "[0]" again, which is wrong.

[hughsando]

@Simn do you think a possible solution is to disable inlining of functions that use "[]" (of type Array<T>) or "new Array<T>()" , or call functions that call these functions?

Another option would be to not follow the type all the way to a concrete type when inlining, and leave this last step up to the target. This would bring the inlining closer to reproducing exactly what the non-inline version does, rather then being a little bit of "pseudo generic" thing.

[Simn]

I don't understand why inlining would be relevant here. We can reproduce the same faulty behavior when "inlining by hand":

class Main {
    public static function main() {
        var s0 = new Stack<Int>();
        var tmp1:Array<Int> = s0.arr;
        var tmp2:Int = s0.len++;
        tmp1[tmp2] = 0;
        trace(s0.arr);
    }
}

class Stack<T> {
    public var arr:Array<T>;
    public var len:Int;

    public function new() {
        arr = [];
        len = 0;
    }
}

[hughsando]

Sure, but you do not get a bug report that starts with "When I inline...."

On Mon, May 18, 2015 at 2:33 PM, Simon Krajewski notifications@github.com
wrote:

I don't understand why inlining would be relevant here. We can reproduce
the same faulty behavior when "inlining by hand":

class Main {
public static function main() {
var s0 = new Stack();
var tmp1:Array = s0.arr;
var tmp2:Int = s0.len++;
tmp1[tmp2] = 0;
trace(s0.arr);
}
}

class Stack {
public var arr:Array;
public var len:Int;

public function new() {
    arr = [];
    len = 0;
}

}

—
Reply to this email directly or view it on GitHub
#4187 (comment)
.

[hughsando]

I guess it depends what kind of a fix you are looking for - quick or good,
choose one.
I have some ideas about wrapping the underlying representation like I
currently do with the Map classes, and only fix a certain type in when a
cast is done. This has potential to fix this problem without a change to
the inlining - and maybe the issues elsewhere with deserialization etc.
But might require a bit of effort.

On Mon, May 18, 2015 at 4:50 PM, Hugh Sanderson gamehaxe@gmail.com wrote:

Sure, but you do not get a bug report that starts with "When I inline...."

On Mon, May 18, 2015 at 2:33 PM, Simon Krajewski <notifications@github.com

wrote:

I don't understand why inlining would be relevant here. We can reproduce
the same faulty behavior when "inlining by hand":

class Main {
public static function main() {
var s0 = new Stack();
var tmp1:Array = s0.arr;
var tmp2:Int = s0.len++;
tmp1[tmp2] = 0;
trace(s0.arr);
}
}

class Stack {
public var arr:Array;
public var len:Int;

public function new() {
    arr = [];
    len = 0;
}

}

—
Reply to this email directly or view it on GitHub
#4187 (comment)
.

[Simn]

We just made a release so let's go for a good fix.

It makes no sense to me to even talk about inlining when the problem can easily be reproduced without inlining.

[hughsando]

Great, I'll think about it some more.

On Mon, May 18, 2015 at 5:09 PM, Simon Krajewski notifications@github.com
wrote:

We just made a release so let's go for a good fix.

It makes no sense to me to even talk about inlining when the problem can
easily be reproduced without inlining.

—
Reply to this email directly or view it on GitHub
#4187 (comment)
.