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

Improve make_static_call #11347

Merged
merged 4 commits into from
Dec 4, 2024
Merged

Improve make_static_call #11347

merged 4 commits into from
Dec 4, 2024

Conversation

Simn
Copy link
Member

@Simn Simn commented Oct 26, 2023

Opening a PR so that I have a single place to put my conclusions.

The problem with make_static_call is that it currently spawns a bunch of monomorphs that are never unified with anything. We have a mechanism now to unify typed call arguments and we should use it here.

However, we're running into problems with some tests related to @:multiType, in particular situations where we have Map<NotString, X> with NotString NOT being assignable to String but still supposed to end up as StringMap. This means a proper unification fails.

If we want to keep this behavior (related issues: #4777, #9712, #10098) we have to add a special case for it "somewhere". I'm having a hard time understanding why it should be justifiable to have a type that is not assignable to another type except when used as a key to Map. However, multiple people whose opinions I usually value highly are somehow totally fine with that idea, so maybe I just have to find the right mindset...

@:multiType is broken, let's check what else
@Simn
Copy link
Member Author

Simn commented Oct 26, 2023

@kLabz Could you check how badly this messes up Shiro things? #4777 mentions CastleDB so I expect apocalypse.

@kLabz
Copy link
Contributor

kLabz commented Oct 26, 2023

Yeah there are some failures:

src/Console.hx:1117: characters 18-27 : error: RegionKind should be String
src/Console.hx:1117: characters 18-27 : ... have: haxe.IMap<RegionKind, ...>
src/Console.hx:1117: characters 18-27 : ... want: haxe.IMap<String, ...>

@kLabz
Copy link
Contributor

kLabz commented Oct 26, 2023

Hide breaks too. Need HeapsIO/hide#230 for module resolution, and then we get

hrt/prefab/l3d/HeightMap.hx:805: characters 47-56 : error: hrt.prefab.l3d.HeightMaPTexturePathKind should be String
hrt/prefab/l3d/HeightMap.hx:805: characters 47-56 : ... have: haxe.IMap<hrt.prefab.l3d.HeightMaPTexturePathKind, ...>
hrt/prefab/l3d/HeightMap.hx:805: characters 47-56 : ... want: haxe.IMap<String, ...>

@Simn
Copy link
Member Author

Simn commented Oct 26, 2023

Alright, thanks... so we need more hacks.

@skial skial mentioned this pull request Oct 27, 2023
1 task
@Simn
Copy link
Member Author

Simn commented Dec 3, 2024

@yuxiaomao Could you check if Shiro survives this?

@yuxiaomao
Copy link
Contributor

All projects survived with this PR.

@Simn Simn merged commit 3813914 into development Dec 4, 2024
99 checks passed
@Simn Simn deleted the make_static_call_better branch December 4, 2024 05:24
@skial skial mentioned this pull request Dec 9, 2024
1 task
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.

3 participants