Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
AdrianV opened this issue Dec 8, 2015 · 8 comments
Open

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

AdrianV opened this issue Dec 8, 2015 · 8 comments
Assignees
Milestone

Comments

@AdrianV
Copy link
Contributor

AdrianV commented Dec 8, 2015

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
Copy link
Member

Simn commented Dec 8, 2015

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.

@Simn Simn self-assigned this Dec 8, 2015
@AdrianV
Copy link
Contributor Author

AdrianV commented Dec 8, 2015

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
Copy link
Member

Simn commented Dec 8, 2015

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
Copy link
Contributor Author

AdrianV commented Dec 8, 2015

I have already changed that ;)

@Simn
Copy link
Member

Simn commented Dec 9, 2015

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
Copy link
Contributor Author

AdrianV commented Dec 10, 2015

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
Copy link
Member

Simn commented Jan 5, 2016

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
Copy link
Member

Simn commented Jan 5, 2016

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.

@Simn Simn modified the milestone: 3.3.0-rc1 Feb 23, 2016
@Simn Simn modified the milestones: 3.4, 3.3.0-rc1 Mar 16, 2016
@Simn Simn modified the milestones: 3.4, 4.0 Jan 9, 2017
@Simn Simn modified the milestones: Release 4.0, Bugs Apr 17, 2018
@Simn Simn modified the milestones: Bugs, Later Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants