HaxeFoundation / haxe

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

dependent on order of @:from function Haxe generates wrong code #4711

Open AdrianV opened 8 years ago

AdrianV commented 8 years ago

the following Haxe code generates wrong code (tested in JS and neko with 3.2++) depending on the position of the fromInt function. The assignment from an IDispatch to Bug takes the fromDate function, which is totally wrong. http://try.haxe.org/#68925

abstract Bug(Float) from Float to Float
{
#if false
    public inline function new(v: Float) this = v;
    @:to public inline function toInt(): Int { return Math.floor(this); }
    @:to public function toDate(): Date { 
        return new Date(Std.int(this), 1, 1, 0, 0, 0);
    }

    @:from static public inline function fromInt(v: Int): Bug return new Bug(v); 
    // if fromInt comes before fromDate the code is correct, but
    // the return value has to be casted - return v; does not compile

    @:from static public function fromDate(d: Date): Bug return d.getFullYear();

#else
    public inline function new(v: Float) {
        this = v;
    }

    @:to public inline function toInt(): Int { return Math.floor(this); }

    @:to public function toDate(): Date { 
        return new Date(Std.int(this), 1, 1, 0, 0, 0);
    }

    @:from public static function fromDate(d: Date): Bug {
        return d != null ? d.getFullYear() : 0.0;
    }

    @:from public static inline function fromInt(v: Int): Bug  return v; 
    // fromInt comes after fromDate - now the wrong code is generated

#end
}

abstract IDispatch(Dynamic) to(Dynamic) {
    public inline function new(v) this = v;
}

class Test {
    static function main() {
        var disp = new IDispatch(123);
        var dyn: Dynamic = 123;
        var t = new Bug(123);
        trace(t);
        t = disp;  // with an abstract wrapping a Dynamic not !
        t = dyn; // with a Dynamic the code is OK 
        trace(t);
    }
}
Simn commented 8 years ago

That's not exactly a bug, the order of cast fields is indeed relevant when multiple types match. The unification succeeds against whichever field because you have to Dynamic on IDispatch.

AdrianV commented 8 years ago

I see, but that is really nasty and produces hard to find bugs. It would be helpful if the compiler could produce warnings in this case (maybe a rule could be that "simple" casts should be before "complicated" casts). I am sure that almost nobody out there is aware of this problem. I hope I never forget about this trap.

Simn commented 8 years ago

Frankly, you bring this upon yourself by having to Dynamic. That's just terrible and you're going to lose your abstract type left and right.

AdrianV commented 8 years ago

I have already changed that ;)

Simn commented 8 years ago

I'm keeping this open because I think about only resolving cast fields on Dynamic if the cast type is explicitly Dynamic as well. That's what we do for static extensions, but I'm not sure if it's a good idea for casts.

AdrianV commented 8 years ago

I have thought about this during sleep that night and found, that eighter I don't understand the problem completely or there is something wrong:

abstract BoxInt(Int) to(Int) {
    public inline function new(v:Int) this =v;
}

abstract Container(Dynamic) {
    @:from static public function fromInt(v: Int): Container return cast v; 
    @:from static public function fromDynamic(v: Int): Container return Std.int(v); 
}

abstract Container2(Dynamic) from Int {
    @:from static public function fromDynamic(v: Int): Container2 return Std.int(v); 
}

class Test {
    static function main() {
        trace("Haxe is great!");
        var x = new BoxInt(123);
        var c: Container = x; // like expected fromInt is used
        trace(c);
        var c2 : Container2 = x; // here is fromDynamic uses instead the more obvious direct cast
        trace(c2);
    }
}

I would have expected, that Container2 behaves the same like Container. From the definition I thought that the direct implicit cast from Int is tried before the class field casts, but as shown in this example Haxe chooses the class field and not the direct cast.

Simn commented 8 years ago

There's no direct cast there, it requires a transitive cast BoxInt -> Int -> Container2. But yes, I agree it's strange that it picks the field in this case.

Simn commented 8 years ago

Actually it's not strange. We check field casts before we check unification because otherwise anything involving Dynamic would never go through a field (because it always unifies). Related issue #2685.