Refactor import resolver to allow for dynamic loading of modules.

[kustosz]

Pull Request Description

Fixes #1569
Closes #1768

Important Notes

Checklist

Please include the following checklist in your PR:

• The documentation has been updated if necessary.
• All code conforms to the Scala, Java, and Rust style guides.
• All documentation and configuration conforms to the markdown and YAML style guides.
• All code has been tested where possible.

[radeusgd]

Looks good to me.

I think error handling of failed module load should be more granular as indicated below, but I guess that can be further extended in future PRs.
Maybe this PR could at least change the return type of ensurePackageIsLoaded into an Either[PackageLoadingError, Unit] with only one variant defined for PackageLoadingError which can then be later extended.

[radeusgd]

I'd suggest renaming to getPreloadedPackages or something like that and indicating in the doc comment that this will be removed in the next iteration. Just to avoid someone using it in a wrong place by mistake.

[radeusgd]

I would suggest extending this signature a bit, although I guess it is ok if it happens in future PRs.

I think we should here somehow differentiate various kinds of errors - for display purposes there should be a different error saying "package is not defined in the edition that you are using" versus "package could not be downloaded due to network problems" or "could not load the package due to insufficient filesystem permissions", as these are completely different error, so thay cannot be merged into a single false return value.

[kustosz]

Fair!

[radeusgd]

After having a second look at the spec I have some doubts if everything is done - technically I guess all features are there, but I think there are things that could be done in this PR instead of the next one.

For example, the getPackages method of the context may be misleading as it only returns the preloaded packages - but as it is only used in the temporary PackageRepository, maybe its logic could be moved into the PackageRepository itself?
Currently the PackageRepository delegates a lot back to the Context, so they are actually quite tightly coupled while the whole purpose of this task was to delegate as much as possible of module handling to the PackageRepository, so that the next task will require less modifications.

I'm also not sure if we should keep modules in Context. I mean I think this is a correct solution, and I appreciate that we only explicitly try to load new packages by calling the ensurePackageIsLoaded method. On the other hand, I feel like having modules there may result in some other code, perhaps added later, getting modules directly from this field and not calling ensurePackageIsLoaded, which could result in forgetting to preload some packages. But maybe it is indeed better to ask to load packages explicitly, rather then implicitly with each getModuleByName request? I'm not sure, just wanted to discuss the alternatives. I would however suggest to rename these methods to indicate that they only refer to loaded modules and will not work if a package was not yet loaded.

[kustosz]

@radeusgd honestly this is just an unblocker for "things in compiler use this, it is obvious where it should be replaced in case the API changes", such that further changes to the packaging logic don't need to propagate far into the compiler. Regarding moving the preloading logic into package repo – I decided against it on the basis of "what's the point moving code that will be removed by the next task?". There will be no preloading, so this whole logic will be removed. Regarding "module access should be mediated through the package repo" – interesting thought, it does have some problems, but may be more elegant. I'll think about it.

[radeusgd]

Regarding moving the preloading logic into package repo – I decided against it on the basis of "what's the point moving code that will be removed by the next task?"

Fair

Regarding "module access should be mediated through the package repo" – interesting thought, it does have some problems, but may be more elegant. I'll think about it.

That was my initial idea, I think that it reduces the chance of erroneously forgetting to call ensurePackageIsLoaded. But also being explicit about loading also may have some advantages, so yeah it would be good to think through which one will be better overall.