HaxeFoundation / haxe

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

[typer] Monomorph vs Null<T> inference #11753

Open kLabz opened 2 months ago

kLabz commented 2 months ago

This is a regression introduced in 4.3.0 with https://github.com/HaxeFoundation/haxe/commit/02a7f9834b1327507eb9b6f646697960a0ec657f

This can also lead to Fatal error: exception Invalid_argument("List.map2") in genhl with a slightly different example. Also allows this kind of things https://try.haxe.org/#05A1D90d

4.2.5

Main.hx:12: characters 9-12 : Warning : Unknown<0> : { doWithBar : () -> Unknown<1> }
Main.hx:13: characters 9-22 : Warning : () -> Unknown<0>
Main.hx:16: characters 9-12 : Warning : Unknown<0> : { doWithBar : () -> Unknown<1> }
Main.hx:17: characters 9-22 : Warning : () -> Unknown<0>
Main.hx:6: characters 35-38 : error: (?bar : Null<Bar>) -> Void should be () -> Unknown<0>
Main.hx:6: characters 35-38 : ... have: { doWithBar: (?...) -> ... }
Main.hx:6: characters 35-38 : ... want: { doWithBar: () -> ... }
Main.hx:6: characters 35-38 : ... For function argument 'foo'

Since 4.3.0

Notice how we lose the doWithBar typing:

Main.hx:12: characters 9-12 : Warning : Unknown<0> : { doWithBar : () -> Unknown<1> }
Main.hx:13: characters 9-22 : Warning : () -> Unknown<0>
Main.hx:16: characters 9-12 : Warning : Null<Unknown<0>>
Main.hx:17: characters 9-22 : Warning : Unknown<0>
Main.hx:11: characters 3-16 : Don't know how to cast (Bar):void to ():dyn

With this PR

[WARNING] Main.hx:12: characters 9-12

 12 |   $type(foo);
    |         ^^^
    | Unknown<0> : { doWithBar : () -> Unknown<1> }

[WARNING] Main.hx:13: characters 9-22

 13 |   $type(foo.doWithBar);
    |         ^^^^^^^^^^^^^
    | () -> Unknown<0>

[WARNING] Main.hx:16: characters 9-12

 16 |   $type(foo);
    |         ^^^
    | Null<{ doWithBar : () -> Unknown<0> }>

[WARNING] Main.hx:17: characters 9-22

 17 |   $type(foo.doWithBar);
    |         ^^^^^^^^^^^^^
    | () -> Unknown<0>

[ERROR] Main.hx:6: characters 35-38

  6 |   doThings = (foo -> doThingsImpl(foo));
    |                                   ^^^
    | error: (?bar : Null<Bar>) -> Void should be () -> Unknown<0>
    | have: { doWithBar: (?...) -> ... }
    | want: { doWithBar: () -> ... }
kLabz commented 2 months ago

This is still wrong.. monomorph is closed, which breaks the following:

    static function doThingsImpl(foo) {
        $type(foo); // Unknown<0>
        foo.doWithBar();
        $type(foo); // Unknown<0> : { doWithBar : () -> Unknown<1> }
        $type(foo.doWithBar); () -> Unknown<0>
        if (foo != null) trace(foo);
        $type(foo); // Null<{ doWithBar : () -> Unknown<0> }>
        $type(foo.doWithBar); // () -> Unknown<0>
        foo.test(); // Null<{ doWithBar : () -> Unknown<0> }> has no field test
    }

With

class Foo {
    public function new() {}
    public function test() {}
    public function doWithBar(?bar:Bar) {
        trace(bar);
    }
}
Simn commented 2 months ago

My intuition here would be to only do this Null<T> binding if the expected type is an unconstrained monomorph.

kLabz commented 2 months ago

My intuition here would be to only do this Null<T> binding if the expected type is an unconstrained monomorph.

Sounds like it would only cover a few use cases (but at least it wouldn't be broken). But also it seems a bit hard to make it work in the general case without reworking the whole "nullable" concept :/

Simn commented 2 months ago

I agree, ideally the nullability of a monomorph would be a property of the monomorph itself, not some wrapper type which easily gets lost. I'm not sure how complicated such a change would be, but in theory we can decorate monomorphs with any number of flags. I vaguely recall wanting to have some sort of "allows array access" flag on them too which would work in a similar fashion.

kLabz commented 2 months ago

Yeah, that sounds like a necessary rabbit hole (but I guess other constraints could be added later if needed rather than all at once).

I started a POC adding that nullability flag; things were not quite working but I could revive that and look into it a bit more since we seem to agree on the theory there

Simn commented 1 month ago

I'd be interested in seeing that POC!