HaxeFoundation / haxe

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

operator overloading – wrong function used (definition order-dependent) #9980

Open Antriel opened 3 years ago

Antriel commented 3 years ago
var pos = new Vec2(1, 1);
var t = pos;
pos *= 10; // Will call `mul` and will create a new instance.
assert(t == pos); // Fail.

@:forward abstract Vec2({x:Float, y:Float}) from {x:Float, y:Float} to {x:Float, y:Float} {
    public inline function new(x:Float, y:Float) this = { x: x, y: y };
    @:op(A * B) public inline function mul(f:Float) return new Vec2(this.x * f, this.y * f);
    @:op(A *= B) public inline function applyMul(f:Float) {
        this.x *= f;
        this.y *= f;
        return this;
    }
}

If the applyMul function is declared before mul, everything works as expected. Reproduced on 4.1, 3.4, and nightly.

Simn commented 3 years ago

This is working as designed. Whether or not that's good design is another question, but I'm not sure what we could even change at this point.

The argument could be made that this should be a declaration-level error because the applyMul function is for all intents and purposes shadowed as far as operator overloading is concerned.

Antriel commented 3 years ago

Error/warning on that would be better than nothing. But ultimately I do find that a bit odd, as there's no other place where declaration order matters (afaik). If the @:op(A *= B) is defined, it should be used. I would even go as far as to say that *= shouldn't be used on @:op(A * B) unless it's explicitly marked to allow it. I'm sure enough people will disagree with me on that though.

Ultimately this caused an issue in my code, and tracking it down was not trivial.

nadako commented 3 years ago

IMO selecting more specific overload like *= instead of * would be correct. Sure, changing this might break some potential insane and arguably already broken code that relies on * overshadowing *=, but I think we could live with that.

back2dos commented 3 years ago

as there's no other place where declaration order matters (afaik)

It matters everywhere, where there's potential ambiguity. For static extensions and imports, the last match is picked, for operators and implicit casts, the first match is taken.

Changing this now will create a huge mess. Might I suggest adding a warning for operators that are fully shadowed and discussing possible design changes separately?

RealyUniqueName commented 3 years ago

There was that idea of special methods for operator overloading. There's even a new keyword operator for that purpose. I think we can implement better resolution with that and keep current one as is.