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.

Sign up for GitHub

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

Jump to bottom

Unclosed monomorphs in function signature #11381

Closed
Simn opened this issue Nov 16, 2023 · 5 comments
Closed

Unclosed monomorphs in function signature #11381

Simn opened this issue Nov 16, 2023 · 5 comments

Comments

@Simn
Copy link
Member

Simn commented Nov 16, 2023

I randomly found this problem today in the test for #2685:

function main() {
	var a:Int = func1(1);
	$type(func1); // (a : Dynamic) -> Int
	var s:String = func1("foo"); // Int should be String
}

function func1(a:Dynamic) {
	return a;
}

The problem is that the return type of func1 remains a monomorph because those are not bound to Dynamic. It is instead unified with the type of var a:Int and thus the function itself ends up as (a : Dynamic) -> Int, which is just wrong.

Very related to #3033 and I still maintain that functions should not expose their monomorphs. I"ll try to see what happens if we bind leftover monomorphs to Dynamic (and maybe Void for some return types?), but I expect some friction here.
@skial skial mentioned this issue Nov 17, 2023
Closed
1 task
@Simn
Copy link
Member Author

Simn commented Dec 19, 2024

I thought that this might work since fa2faa1, but it doesn"t. I suspect that we create a monomorph via setup_args_ret before we actually setup ctx.e, which means it is added to a ctx.e.monomorphs which is then forgotten about.

@Simn Simn closed this as completed in f2638f6 Dec 19, 2024
@kLabz
Copy link
Contributor

kLabz commented Dec 19, 2024

Nice!

Slightly related, from the updated test:

 6 |   p.then(x -> 10, e -> "");
   |                   ^^^^^^^
   | (e : Dynamic) -> String should be Null<js.lib.PromiseHandler<Dynamic, Int>>

Would Any work instead of Dynamic?
As we want to [slowly] move away from Dynamic, this would be nice

@Simn
Copy link
Member Author

Simn commented Dec 19, 2024

This comes from fa2faa1, so the situation is that there"s already a Dynamic in the code that was unified with the monomorph. If the code in question used Any instead we wouldn"t run into this problem in the first place.

@kLabz
Copy link
Contributor

kLabz commented Dec 19, 2024

I guess my suggestion was to use Any instead of Dynamic when closing a monomorph that has no constraints, but maybe that wouldn"t happen there anyway because of expected type?

@Simn
Copy link
Member Author

Simn commented Dec 19, 2024

It doesn"t have no constraints, its only constraint is Dynamic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@Simn @kLabz