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

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

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

kLabz
Copy link
Contributor

@kLabz kLabz commented Aug 30, 2024

This is a regression introduced in 4.3.0 with 02a7f98

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 kLabz added this to the 4.3 Hotfix milestone Aug 30, 2024
@kLabz kLabz changed the title [regression] Monomorph vs Null<T> inference [typer] Monomorph vs Null<T> inference Aug 30, 2024
@kLabz
Copy link
Contributor Author

kLabz commented Aug 30, 2024

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

Simn commented Aug 31, 2024

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

@kLabz
Copy link
Contributor Author

kLabz commented Sep 4, 2024

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 :/

@skial skial mentioned this pull request Sep 4, 2024
1 task
@Simn
Copy link
Member

Simn commented Sep 10, 2024

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

kLabz commented Sep 10, 2024

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

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

Successfully merging this pull request may close these issues.

2 participants