Refactor import resolution to use a global store #204
Conversation
To make sure we don't let ids escape and we don't mix scopes.
We aren't supposed to inspect anything before alternatives are chosen
There was a problem hiding this comment.
Nice work! 😄
This looks pretty involved 😅 I was wondering if it wouldn't be simpler if the global import store was kept in a global mutable data structure. You can get something pretty good with lazy_static and a Mutex. I believe this would remove the need for lifetimes everywhere, and you wouldn't have to thread the context to all the functions that need it.
About issue dhall-lang/dhall-lang#1113, while I still think it's worth thinking about whether parse errors and typecheck errors should influence alternative imports, I am now less sure that it will make things much easier for resolving imports concurrently.
After talking with @ocharles on Slack about it, it became more clear to me that typechecking still needs to be done on a per file basis, and that we cannot simply inline the result of an import into the expression where it is imported without typechecking it first. He pointed out this is particularly important when dealing with free variables resolution, where they obviously should not leak between imports. You probably already understood this, and I didn't think it through. 😅
So is the main benefit for you to have fewer files to typecheck by being able to eliminate some alternatives earlier? Even if we don't get this issue resolved, we should be able to typecheck all alternatives concurrently (which on non-wasm targets could actually be done in parallel), no?
| pub struct StoredImportAlternative<'cx> { | ||
| pub left_imports: Box<[ImportNode<'cx>]>, | ||
| pub right_imports: Box<[ImportNode<'cx>]>, | ||
| /// `true` for left, `false` for right. | ||
| selected: OnceCell<bool>, | ||
| } |
There was a problem hiding this comment.
How does this work? I'm not sure it can support nested alternatives, can it?
There was a problem hiding this comment.
It can :) An ImportNode can be either a single import or another alternative
|
Yeah, I considered the global mutable store but it made me sad :(. The pain of lifetimes is mostly adding them, but once they're there they're not too painful I find. |
|
EDIt: I confused myself, for a moment I thought there were no dependencies between different files when we typecheck :( |
|
Eh, I want to run with this and see where it leads. I think we really want some sort of store, we can always make it static later if the lifetimes-based one is too painful |
This is the first step towards concurrent/async import resolution, as needed for wasm support but also more generally would be cool.
This is an intense PR. The idea is that instead of storing the result of an import in the
Hirdirectly, instead we store and index into a global store. Which means I had to make the global store accessible to any code that would need to read an import. I ended up adding a lot of lifetimes x).So now import resolution runs in two phases: first traverse the ast and replace the imports by their indices, and then go through the imports and resolve them. By making that second step smaller, we'll make it possible to add async and to batch imports.
I got excited with the global store. There's a few more cool things I want to do with it. I hope this is not making the code too much more complicated.
Among other things, once non-import errors don't count for import alternatives (dhall-lang/dhall-lang#1113), we can decouple entirely import resolution from the rest of the code. That would not have been possible before this PR without introducing lots of duplicate work.
cc @basile-henry