make handles_assign_ops for cpp always true

[mikaib]

Previously += would not be handled by the analyzer.
This issue/fix was found by @Apprentice-Alchemist but I decided to make a PR before it gets forgotten😅

Given:

class Float2 {
    public var x:Float = 0.0;
    public var y:Float = 0.0;

    public function new(_x:Float, _y:Float) {
        x = _x;
        y = _y;
    }

    public function toString():String {
        return 'Float2($x, $y)';
    }
}

class Test {
    static var v:Float2;

    static function main() {
        v = new Float2(10.0, 20.0);

        trace(v.toString());
        doSomething();
        trace(v.toString());
    }

    static function doSomething():Void {
        v.x += 30.0;
        v.y *= 2.0;
    }
}

It would generate

void Test_obj::doSomething(){
                HX_STACKFRAME(&_hx_pos_df13cafffbed243f_26_doSomething)
                HXLINE(  27)         ::Float2 fh = ::Test_obj::v;
                HXDLIN(  27)        fh->x = (fh->x + ((Float)30.0));
                HXLINE(  28)         ::Float2 fh1 = ::Test_obj::v;
                HXDLIN(  28)        fh1->y = (fh1->y * ((Float)2.0));
}

While with this change, or by doing v.x = v.x + 30.0 instead of v.x += 30.0 it would generate:

void Test_obj::doSomething(){
                HX_STACKFRAME(&_hx_pos_df13cafffbed243f_26_doSomething)
                HXDLIN(  27)        ::Test_obj::v->x = (::Test_obj::v->x + ((Float)30.0));
                HXDLIN(  28)        ::Test_obj::v->y = (::Test_obj::v->y * ((Float)2.0));
}

I believe this change is rather crucial as without this change there is a difference in behaviour regarding x += y and x = x + y when using cpp.Struct<T> (or any type with copy semantics...)

You can follow the conversation inside the Haxe discord server here

[tobil4sk]

I believe this change is rather crucial as without this change there is a difference in behaviour regarding x += y and x = x + y when using cpp.Struct (or any type with copy semantics...)

It would be good to add a test for the behaviour that this fixes, and also we should determine why this was made false for cpp originally.

EDIT: looks like the reason is #5545

[mikaib]

Hm, that makes this issue more difficult than I initially suspected. I'll try to investigate... In the meantime I'll also create a test based on #5545 so that issues regarding OpAssignOp can be more easily caught in the future.

[mikaib]

Considering this example:

    public static var vg: Val;

    static function testVec() {
        var vl = new Val();
        vg = new Val();

        vg.x += 10;
        vl.x += 10;

        trace(vg.x, vl.x);
    }

Here Val is a struct (represented as cpp.Struct<T>) with x as a Float. The trace in the code above gives us:

    OriginalTest.hx:34: 0,10

The offending code here is:

    HXLINE(  31)		cpp::Struct<  ::_OriginalTest::_Val > fh = ::OriginalTest_obj::vg;
HXDLIN(  31)		fh->x = (fh->x + 10);

My first idea for a fix was something like the following:

    HXLINE(  31)		cpp::Struct<  ::_OriginalTest::_Val > fh = ::OriginalTest_obj::vg;
HXDLIN(  31)		 ::OriginalTest_obj::vg->x = (fh->x + 10);

But this could obviously break easily if you had something like funcWithSideEffects().x += 5;, below is the output for that (which is the desired/correct output):

HXLINE(  34)		cpp::Struct<  ::_OriginalTest::_Val > fh1 = ::OriginalTest_obj::funcWithSideEffects();
HXDLIN(  34)		fh1->x = (fh1->x + 5);

If my idea above was used, this would call ::OriginalTest_obj::funcWithSideEffects() twice.

So considering this, here is my "revised" idea:

• If we are doing OpAssignOp on a variable we should be able to safely do ::OriginalTest_obj::vg->x = (fh->x + 10);, meaning we read it it into a temporary and write back to the original.
• If we are doing OpAssignOp on something that would trigger copy semantics anyway (like a function call, getter/setter, etc), we put it in a temporary and write back to the temporary (as it shouldn't impact the behaviour in any way, this is also the current behaviour).
• Perhaps we could even lock this workaround behind a specific meta, or behind a specific semantic like @:semantic(value) to ensure this workaround doesn't cause unexpected issues later on.
• Or maybe there is a different, possibly better way of fixing this entirely,

The main question I have is how feasible it is to track what would trigger copy semantics or not...

I'm still very inexperienced when it comes to the Haxe compiler internals, so I'm very sorry if I've said something that makes no sense at all (which could be likely).

[tobil4sk]

I wonder if we could apply this to c++ ffa72bc. This way if the rhs expression doesn't have side effects then the assign op can be generated. It also seems strange to me that it checks for side effects on php but for other targets it just returns true without checking.

The issue with cpp.Struct being copied might be a related but separate issue, because it's more about the temporary variables. I think it would make sense to add @:semantics(value) to cpp.Struct and ensure that assignments don't generate temporary variables if the type has value semantics, even if cases where the assign op can't be generated.

[mikaib]

Oh hah, I completely read over that- I think that would work quite well actually... Only one way to find out 😄
Regarding your second point, I think that would be quite a good change to make aswell, I'll create another PR for that I suppose...