Rework module resolution #11168
Rework module resolution #11168
Conversation
|
This should be ready, or at least as ready as it will be for the time being. I have updated the initial post to explain what's happening. Now waiting for CI to behave so that I can benchmark. @kLabz Could you try this branch with Shiro codebases, and whatever else you find that might be interesting? |
|
I will try to find some time this week-end (worst case I'll do it on Monday) |
# Conflicts: # src/typing/typer.ml
|
There's a strange case in heap's Quat.hx module: At the top of the file it has However, inside the I'm currently in a state of confusion. |
|
We're working towards merging this. Some libraries are going to need updates because they rely on unintended importing behavior. However, all fixes required can be made in a way that is compatible with both Haxe 4 and 5. |
|
Unfortunately, the benchmarks for this don't look good: 
While it's not massively slower, the graph suggests that this doesn't scale well in larger projects. It's quite difficult to determine the cause of this accurately, but we'll have to look into that. Edit: Actually, these results are not recent. But I doubt the more recent results will look much different. |
|
Good news: After running the benchmarks again with the current version, the results look much better: 
|
|
@kLabz Could you check Shiro codebases with https://github.com/HaxeFoundation/haxe/tree/module_resolution_no_weirdness? I still want to make that change too (for #9150), but if it causes too much friction right now we can wait a bit. |
for now
* start on m.module_resolution * support static inits * finish module_globals removal * remove wildcard_packages * get cursed resolution tests to pass * add some debug and a test of the current behavior (see HaxeFoundation#9197) * remove early import resolving and fix python test * turn into class * add RLazy * (finally) remove context_init * don't expand for type lookups * fix 2/3 of broken display test * rename some things * make immutable * add own_resolution * avoid some duplicate lookuppery * distinguish class and abstract statics * move enum expansion to resolution_list * move to own module * add test for alias conflict * add test #closes 2729 * add actual resolve method and try some caching * change expected enum typing * try something different * merge aliases into resolution_kind * remove add_l * add some timers and a type import cache * deal with weirdness * remove weird import lookup see HaxeFoundation#9150 * meh * asdfg * keep weird handling for now to have CI green * investigate * add timer for flush_pass * add absurd amount of timers * Revert "add absurd amount of timers" This reverts commit 1c49717. * Revert "add timer for flush_pass" This reverts commit 935946b. * Revert "investigate" This reverts commit de52786. * fix test * Remove unused open * remove timers for now --------- Co-authored-by: Rudy Ges <[email protected]>
This changes resolution behavior to consistently consider lexical appearance instead of always preferring enum constructors. Previously, the typer would keep 3 separate lists:
This PR merges those into a single list, in which they are distinguished by a variant type. Identifier resolution then walks through that list and compares all imports in-order. This means that this changes the specified resolution behavior and is a breaking change.
Additionally, resolution now treats its own types and statics as if they were imported, and caches this information in
ctx.m.own_resolution. This fixes #9197.Aliasing
The implementation of
import X as ywas changed by giving all imports an implicit alias name, which is what resolution compares against. For types, it's the type name and for fields it's the field name. As a consequence, we no longer introduce special typedefs to make this work, which caused one display test to fail. I will track this in #8660.Field imports
One problem with field imports is that by the time we're processing imports initially, we don't know a module's structure yet. To deal with that, the current implementation passed a
context_initobject around which manages the delays for such fields. I have removed that completely and instead introduced the resolution kindRLazy.You can think of
RLazyas a placeholder in the resolution list, which then expands itself when it's time to resolve a field. So a resolution list like[Type1, Type2, RLazy, Type3]then becomes[Type1, Type2, Field1, Field2, Type3]. This ensures that the lexical order is maintained.This functionality is also used to deal with enum and enum abstract constructors: When we add a type import, we also add an
RLazyin front of it which might resolve to a list of constructors. This gives us the same behavior as before.Breaking...
I'm not sure how much this will break in practice, but it's definitely a specification change. Once we merge this, I'll consider the path to Haxe 5 open. Before that, I'd appreciate if people could test their codebases on this branch and report problems.
I'll also make sure to run this on https://benchs.haxe.org/ before merging. There's a chance that this linear kind of lookup performs worse than the previous implementation, although that one also wasn't exactly optimized. If this turns out to be a problem, we can look into adding a proper lookup cache, which might be a good idea anyway.
Also closes third-oldest open issue #2729.